* Re: [PATCH v3 4/5] LSM: Define SELinux function to measure security state
From: Lakshmi Ramasubramanian @ 2020-07-20 15:17 UTC (permalink / raw)
To: Stephen Smalley
Cc: Mimi Zohar, Casey Schaufler, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <CAEjxPJ7xQtZToF4d2w_o8SXFKG9kPZaWTWTFqyC-7GwBWnQa0A@mail.gmail.com>
On 7/20/20 7:31 AM, Stephen Smalley wrote:
>> +void __init selinux_init_measurement(void)
>> +{
>> + int i;
>> +
>> + /*
>> + * enabled
>> + * enforcing
>> + * checkreqport
>
> checkreqprot (spelling)
:( - will fix that.
>
> What about initialized? Or do you consider that to be implicitly
> true/1 else we wouldn't be taking a measurement? Only caveat there is
> that it provides one more means of disabling measurements (at the same
> time as disabling enforcement) by setting it to false/0 via kernel
> write flaw.
Yes - I was thinking measuring SELinux state would be meaningful only
when initialized is set to true/1.
I can include "initialized" as well in the measurement.
>
>> + * All policy capability flags
>> + */
>> + selinux_state_count = 3 + __POLICYDB_CAPABILITY_MAX;
>> +
>> + selinux_state_string_len = snprintf(NULL, 0, str_format,
>> + "enabled", 0);
>> + selinux_state_string_len += snprintf(NULL, 0, str_format,
>> + "enforcing", 0);
>> + selinux_state_string_len += snprintf(NULL, 0, str_format,
>> + "checkreqprot", 0);
>> + for (i = 3; i < selinux_state_count; i++) {
>> + selinux_state_string_len +=
>> + snprintf(NULL, 0, str_format,
>> + selinux_policycap_names[i-3], 0);
>> + }
>
> What's the benefit of this pattern versus just making the loop go from
> 0 to __POLICYDB_CAPABILITY_MAX and using selinux_policycap_names[i]?
No real benefit - I was just trying to use selinux_state_count.
I'll change the loop to go from 0 to POLICY_CAP_MAX
>
>> +void selinux_measure_state(struct selinux_state *selinux_state)
>> +{
>> + void *policy = NULL;
>> + void *policy_hash = NULL;
>> + size_t curr, buflen;
>> + int i, policy_hash_len, rc = 0;
>> +
>> + if (!selinux_initialized(selinux_state)) {
>> + pr_warn("%s: SELinux not yet initialized.\n", __func__);
>> + return;
>> + }
>
> We could measure the global state variables before full SELinux
> initialization (i.e. policy load).
> Only the policy hash depends on having loaded the policy.
Thanks for the information. I'll measure the state variables always and
measure policy only if "initialized" is true/1.
>
>> +
>> + if (!selinux_state_string) {
>> + pr_warn("%s: Buffer for state not allocated.\n", __func__);
>> + return;
>> + }
>> +
>> + curr = snprintf(selinux_state_string, selinux_state_string_len,
>> + str_format, "enabled",
>> + !selinux_disabled(selinux_state));
>> + curr += snprintf((selinux_state_string + curr),
>> + (selinux_state_string_len - curr),
>> + str_format, "enforcing",
>> + enforcing_enabled(selinux_state));
>> + curr += snprintf((selinux_state_string + curr),
>> + (selinux_state_string_len - curr),
>> + str_format, "checkreqprot",
>> + selinux_checkreqprot(selinux_state));
>> +
>> + for (i = 3; i < selinux_state_count; i++) {
>> + curr += snprintf((selinux_state_string + curr),
>> + (selinux_state_string_len - curr),
>> + str_format,
>> + selinux_policycap_names[i - 3],
>> + selinux_state->policycap[i - 3]);
>> + }
>
> Same question here as for the previous loop; seems cleaner to go from
> 0 to __POLICYDB_CAPABILITY_MAX and use [i].
Will change it.
>
> What public git tree / branch would you recommend trying to use your
> patches against? Didn't seem to apply to any of the obvious ones.
>
Please try it on Mimi's next-integrity branch
https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git/log/?h=next-integrity
You can try it on Linus's mainline as well if you apply the following
patch first (have mentioned that in the Cover letter as well)
https://patchwork.kernel.org/patch/11612989/
Thanks for trying out the changes. Please let me know the defects you find.
Just to let you know - I am making the following change (will update in
the next patch):
=> Save the last policy hash and state string in selinux_state struct.
=> Measure policy and hash only if it has changed since the last
measurement.
=> Also, suffix the IMA event name used with time stamp. For example,
10 e32e...5ac3 ima-buf sha256:86e8...4594
selinux-state-1595257807:874963248
656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574706565723d313b6f70656e7065726d3d313b657874736f636b636c6173733d313b616c776179736e6574776f726b3d303b6367726f75707365636c6162656c3d313b6e6e706e6f737569647472616e736974696f6e3d313b67656e66737365636c6162656c73796d6c696e6b3d303b
10 f4a7...9408 ima-buf sha256:4941...68fc
selinux-policy-hash-1595257807:874963248
8d1d...1834
The above will ensure the following sequence will be measured:
#1 State A - Measured
#2 Change from State A to State B - Measured
#3 Change from State B back to State A - Since the measured data is
same as in #1, the change will be measured only if the event name is
different between #1 and #3
thanks,
-lakshmi
^ permalink raw reply
* Re: [PATCH 12/15] Manual pages: cap_get_file.3: NOTES: note the effect of the Ambient set
From: Andrew G. Morgan @ 2020-07-20 15:36 UTC (permalink / raw)
To: Michael Kerrisk (man-pages); +Cc: LSM List
In-Reply-To: <20200720091328.290336-13-mtk.manpages@gmail.com>
I've applied all but this one. This one seems to imply that if the
effective bit is lowered, but the permitted bits are raised, the
ambient will have some sort of effect. This isn't how it works. Any
file caps (even an empty set) suppresses any effect of the ambient
vector.
Cheers
Andrew
On Mon, Jul 20, 2020 at 2:14 AM Michael Kerrisk (man-pages)
<mtk.manpages@gmail.com> wrote:
>
> The addition of Ambient capabilities in Linux 4.3 rendered the text on
> the effect of the Effective bit during execve(2) out-of-date. Fix that.
> Also add a couple of paragraph breaks to improve readability.
>
> Signed-off-by: Michael Kerrisk (man-pages) <mtk.manpages@gmail.com>
> ---
> doc/cap_get_file.3 | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/doc/cap_get_file.3 b/doc/cap_get_file.3
> index ceacbaf..dc7b571 100644
> --- a/doc/cap_get_file.3
> +++ b/doc/cap_get_file.3
> @@ -103,13 +103,18 @@ or
> These functions are specified by withdrawn POSIX.1e draft specification.
> .SH NOTES
> Support for file capabilities is provided on Linux since version 2.6.24.
> -
> +.PP
> On Linux, the file Effective set is a single bit.
> If it is enabled, then all Permitted capabilities are enabled
> in the Effective set of the calling process when the file is executed;
> -otherwise, no capabilities are enabled in the process's Effective set
> +otherwise, the process's Ambient capabilities
> +(or, before the Linux 4.3 addition of Ambient capabilities, no capabilities)
> +are enabled in the process's Effective set
> following an
> -.BR execve (2).
> +.BR execve (2)
> +(see
> +.BR capabilities (7)).
> +.PP
> Because the file Effective set is a single bit,
> if any capability is enabled in the Effective set of the
> .I cap_t
> --
> 2.26.2
>
^ permalink raw reply
* Re: [RFC PATCH v4 05/12] fs: add security blob and hooks for block_device
From: Deven Bowers @ 2020-07-20 16:42 UTC (permalink / raw)
To: Casey Schaufler, agk, axboe, snitzer, jmorris, serge, zohar, viro,
paul, eparis, jannh, dm-devel, linux-integrity,
linux-security-module, linux-fsdevel, linux-block, linux-audit
Cc: tyhicks, linux-kernel, corbet, sashal, jaskarankhurana, mdsakib,
nramas
In-Reply-To: <1843d707-c62e-fa13-c663-c123ea1205a0@schaufler-ca.com>
On 7/17/2020 5:14 PM, Casey Schaufler wrote:
[...snip]
>> +EXPORT_SYMBOL(security_bdev_free);
>> +
>> +int security_bdev_setsecurity(struct block_device *bdev,
>> + const char *name, const void *value,
>> + size_t size)
>> +{
>> + return call_int_hook(bdev_setsecurity, 0, bdev, name, value, size);
>> +}
>
> What is your expectation regarding multiple security modules using the
> same @name? What do you expect a security module to do if it does not
> support a particular @name? You may have a case where SELinux supports
> a @name that AppArmor (or KSRI) doesn't. -ENOSYS may be you friend here.
>
I expect that some security modules may want to use the same @name / use
the data contained with @name. I cannot speak to the future cases of
other LSMs, but I expect if they want the raw @value, they'll copy it
into their security blob, or interpret @value to a field defined by
their security blob.
Originally, I expected a security module that does not implement a
particular @name no-op with return 0, not -ENOSYS, but I recognize that
error codes are valuable, and it's a trivial change - I'll switch the
security hook to call the hooks while allowing -ENOSYS or 0 in the next
iteration.
>> +EXPORT_SYMBOL(security_bdev_setsecurity);
>> +
>> #ifdef CONFIG_PERF_EVENTS
>> int security_perf_event_open(struct perf_event_attr *attr, int type)
>> {
^ permalink raw reply
* Re: [RFC PATCH v4 02/12] security: add ipe lsm evaluation loop and audit system
From: Deven Bowers @ 2020-07-20 16:44 UTC (permalink / raw)
To: Randy Dunlap, agk, axboe, snitzer, jmorris, serge, zohar, viro,
paul, eparis, jannh, dm-devel, linux-integrity,
linux-security-module, linux-fsdevel, linux-block, linux-audit
Cc: tyhicks, linux-kernel, corbet, sashal, jaskarankhurana, mdsakib,
nramas, pasha.tatshin
In-Reply-To: <4b0c9925-d163-46a2-bbcb-74deb7446540@infradead.org>
On 7/17/2020 4:16 PM, Randy Dunlap wrote:
> On 7/17/20 4:09 PM, Deven Bowers wrote:
>> +config SECURITY_IPE_PERMISSIVE_SWITCH
>> + bool "Enable the ability to switch IPE to permissive mode"
>> + default y
>> + help
>> + This option enables two ways of switching IPE to permissive mode,
>> + a sysctl (if enabled), `ipe.enforce`, or a kernel command line
>> + parameter, `ipe.enforce`. If either of these are set to 0, files
>
> is set
Thanks, I'll change it in the next iteration.
>
>> + will be subject to IPE's policy, audit messages will be logged, but
>> + the policy will not be enforced.
>> +
>> + If unsure, answer Y.
>
>
^ permalink raw reply
* Re: [PATCH v3 07/12] ima: Fail rule parsing when appraise_flag=blacklist is unsupportable
From: Nayna @ 2020-07-20 17:02 UTC (permalink / raw)
To: Tyler Hicks, Mimi Zohar
Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
linux-integrity, linux-security-module, Nayna Jain
In-Reply-To: <20200717181133.GM3673@sequoia>
On 7/17/20 2:11 PM, Tyler Hicks wrote:
> On 2020-07-17 13:40:22, Nayna wrote:
>> On 7/9/20 2:19 AM, Tyler Hicks wrote:
>>> The "appraise_flag" option is only appropriate for appraise actions
>>> and its "blacklist" value is only appropriate when
>>> CONFIG_IMA_APPRAISE_MODSIG is enabled and "appraise_flag=blacklist" is
>>> only appropriate when "appraise_type=imasig|modsig" is also present.
>>> Make this clear at policy load so that IMA policy authors don't assume
>>> that other uses of "appraise_flag=blacklist" are supported.
>>>
>>> Fixes: 273df864cf74 ("ima: Check against blacklisted hashes for files with modsig")
>>> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
>>> Cc: Nayna Jain <nayna@linux.ibm.com>
>>> ---
>>>
>>> * v3
>>> - New patch
>>>
>>> security/integrity/ima/ima_policy.c | 13 ++++++++++++-
>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>>> index 81da02071d41..9842e2e0bc6d 100644
>>> --- a/security/integrity/ima/ima_policy.c
>>> +++ b/security/integrity/ima/ima_policy.c
>>> @@ -1035,6 +1035,11 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>>> return false;
>>> }
>>> + /* Ensure that combinations of flags are compatible with each other */
>>> + if (entry->flags & IMA_CHECK_BLACKLIST &&
>>> + !(entry->flags & IMA_MODSIG_ALLOWED))
>>> + return false;
>>> +
>>> return true;
>>> }
>>> @@ -1371,8 +1376,14 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>>> result = -EINVAL;
>>> break;
>>> case Opt_appraise_flag:
>>> + if (entry->action != APPRAISE) {
>>> + result = -EINVAL;
>>> + break;
>>> + }
>>> +
>>> ima_log_string(ab, "appraise_flag", args[0].from);
>>> - if (strstr(args[0].from, "blacklist"))
>>> + if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
>>> + strstr(args[0].from, "blacklist"))
>>> entry->flags |= IMA_CHECK_BLACKLIST;
>> If IMA_APPRAISE_MODSIG is disabled, it will allow the following rule to
>> load, which is not as expected.
>>
>> "appraise func=xxx_CHECK appraise_flag=blacklist appraise_type=imasig"
>>
>> Missing is the "else" condition to immediately reject the policy rule.
> Thanks for the review. You're right. This change is needed:
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 9842e2e0bc6d..cf3ddb38dfa8 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1385,6 +1385,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
> strstr(args[0].from, "blacklist"))
> entry->flags |= IMA_CHECK_BLACKLIST;
> + else
> + result = -EINVAL;
> break;
> case Opt_permit_directio:
> entry->flags |= IMA_PERMIT_DIRECTIO;
>
Reviewed-by: Nayna Jain<nayna@linux.ibm.com>
Tested-by: Nayna Jain<nayna@linux.ibm.com>
Thanks & Regards,
- Nayna
^ permalink raw reply
* Re: [PATCH v3 4/5] LSM: Define SELinux function to measure security state
From: Stephen Smalley @ 2020-07-20 17:06 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Mimi Zohar, Casey Schaufler, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <c0fbfcf3-ec36-872a-c389-b3fea214848c@linux.microsoft.com>
On Mon, Jul 20, 2020 at 11:17 AM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
> Thanks for trying out the changes. Please let me know the defects you find.
>
> Just to let you know - I am making the following change (will update in
> the next patch):
>
> => Save the last policy hash and state string in selinux_state struct.
> => Measure policy and hash only if it has changed since the last
> measurement.
> => Also, suffix the IMA event name used with time stamp. For example,
>
> 10 e32e...5ac3 ima-buf sha256:86e8...4594
> selinux-state-1595257807:874963248
> 656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574706565723d313b6f70656e7065726d3d313b657874736f636b636c6173733d313b616c776179736e6574776f726b3d303b6367726f75707365636c6162656c3d313b6e6e706e6f737569647472616e736974696f6e3d313b67656e66737365636c6162656c73796d6c696e6b3d303b
>
> 10 f4a7...9408 ima-buf sha256:4941...68fc
> selinux-policy-hash-1595257807:874963248
> 8d1d...1834
>
> The above will ensure the following sequence will be measured:
> #1 State A - Measured
> #2 Change from State A to State B - Measured
> #3 Change from State B back to State A - Since the measured data is
> same as in #1, the change will be measured only if the event name is
> different between #1 and #3
Perhaps the timestamp / sequence number should be part of the hashed
data instead of the event name?
I can see the appraiser wanting to know two things:
1) The current state of the system (e.g. is it enforcing, is the
currently loaded policy the expected one?).
2) Has the system ever been in an unexpected state (e.g. was it
temporarily switched to permissive or had an unexpected policy
loaded?)
I applied the patch series on top of the next-integrity branch, added
measure func=LSM_STATE to ima-policy, and booted that kernel. I get
the following entries in ascii_runtime_measurements, but seemingly
missing the final field:
10 8a09c48af4f8a817f59b495bd82971e096e2e367 ima-ng
sha256:21c3d7b09b62b4d0b3ed15ba990f816b94808f90b76787bfae755c4b3a44cd24
selinux-state
10 e610908931d70990a2855ddb33c16af2d82ce56a ima-ng
sha256:c8898652afd5527ef4eaf8d85f5fee1d91fcccee34bc97f6e55b96746bedb318
selinux-policy-hash
Thus, I cannot verify. What am I missing?
^ permalink raw reply
* Re: [PATCH v3 4/5] LSM: Define SELinux function to measure security state
From: Mimi Zohar @ 2020-07-20 17:26 UTC (permalink / raw)
To: Stephen Smalley, Lakshmi Ramasubramanian, Tyler Hicks,
Prakhar Srivastava
Cc: Casey Schaufler, James Morris, linux-integrity, SElinux list,
LSM List, linux-kernel
In-Reply-To: <CAEjxPJ7VH18bEo6+U1GWrx=tHVGr=6XtF5_ygcfQYgdtZ74J+g@mail.gmail.com>
On Mon, 2020-07-20 at 13:06 -0400, Stephen Smalley wrote:
>
>
> I applied the patch series on top of the next-integrity branch, added
> measure func=LSM_STATE to ima-policy, and booted that kernel. I get
> the following entries in ascii_runtime_measurements, but seemingly
> missing the final field:
>
> 10 8a09c48af4f8a817f59b495bd82971e096e2e367 ima-ng
> sha256:21c3d7b09b62b4d0b3ed15ba990f816b94808f90b76787bfae755c4b3a44cd24
> selinux-state
> 10 e610908931d70990a2855ddb33c16af2d82ce56a ima-ng
> sha256:c8898652afd5527ef4eaf8d85f5fee1d91fcccee34bc97f6e55b96746bedb318
> selinux-policy-hash
>
> Thus, I cannot verify. What am I missing?
Missing is "template=ima-buf" on the policy rule.
Tyler's patch set just added some support for verifying the policy.
Refer to ima_validate_rule(). There are still some things missing.
For example, nayna noticed that making sure that asymmetric key
support is enabled. Another example is requiring "template=" for any
of the buffer measurements. Template names can be defined
dynamically, so it will need to support either format:
measure func=KEXEC_CMDLINE template=ima-buf
measure func=KEXEC_CMDLINE template=d-ng|n-ng|buf
Mimi
^ permalink raw reply
* Re: [PATCH v3 4/5] LSM: Define SELinux function to measure security state
From: Lakshmi Ramasubramanian @ 2020-07-20 17:34 UTC (permalink / raw)
To: Stephen Smalley
Cc: Mimi Zohar, Casey Schaufler, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <CAEjxPJ7VH18bEo6+U1GWrx=tHVGr=6XtF5_ygcfQYgdtZ74J+g@mail.gmail.com>
On 7/20/20 10:06 AM, Stephen Smalley wrote:
>> The above will ensure the following sequence will be measured:
>> #1 State A - Measured
>> #2 Change from State A to State B - Measured
>> #3 Change from State B back to State A - Since the measured data is
>> same as in #1, the change will be measured only if the event name is
>> different between #1 and #3
>
> Perhaps the timestamp / sequence number should be part of the hashed
> data instead of the event name?
If the timestamp/seqno is part of the hashed data, on every call to
measure IMA will add a new entry in the IMA log. This would fill up the
IMA log - even when there is no change in the measured data.
To avoid that I keep the last measurement in SELinux and measure only
when there is a change with the timestamp in the event name.
> I can see the appraiser wanting to know two things:
> 1) The current state of the system (e.g. is it enforcing, is the
> currently loaded policy the expected one?).
> 2) Has the system ever been in an unexpected state (e.g. was it
> temporarily switched to permissive or had an unexpected policy
> loaded?)
Yes - you are right.
The appraiser will have to look at the entire IMA log (and the
corresponding TPM PCR data) to know the above.
Time t0 => State of the system measured
Time tn => State changed and the new state measured
Time tm => State changed again and the new state measured.
Say, the measurement at "Time tn" was an illegal change, the appraiser
would know.
>
> I applied the patch series on top of the next-integrity branch, added
> measure func=LSM_STATE to ima-policy, and booted that kernel. I get
> the following entries in ascii_runtime_measurements, but seemingly
> missing the final field:
>
> 10 8a09c48af4f8a817f59b495bd82971e096e2e367 ima-ng
> sha256:21c3d7b09b62b4d0b3ed15ba990f816b94808f90b76787bfae755c4b3a44cd24
> selinux-state
> 10 e610908931d70990a2855ddb33c16af2d82ce56a ima-ng
> sha256:c8898652afd5527ef4eaf8d85f5fee1d91fcccee34bc97f6e55b96746bedb318
> selinux-policy-hash
>
> Thus, I cannot verify. What am I missing?
>
Looks like the template used is ima-ng which doesn't include the
measured buffer. Please set template to "ima-buf" in the policy.
For example,
measure func=LSM_STATE template=ima-buf
thanks,
-lakshmi
^ permalink raw reply
* Re: [PATCH v3 4/5] LSM: Define SELinux function to measure security state
From: Stephen Smalley @ 2020-07-20 17:40 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Mimi Zohar, Casey Schaufler, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <bea0cb52-2e13-fb14-b66c-b57287c23c3f@linux.microsoft.com>
On Mon, Jul 20, 2020 at 1:34 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> On 7/20/20 10:06 AM, Stephen Smalley wrote:
>
> >> The above will ensure the following sequence will be measured:
> >> #1 State A - Measured
> >> #2 Change from State A to State B - Measured
> >> #3 Change from State B back to State A - Since the measured data is
> >> same as in #1, the change will be measured only if the event name is
> >> different between #1 and #3
> >
> > Perhaps the timestamp / sequence number should be part of the hashed
> > data instead of the event name?
>
> If the timestamp/seqno is part of the hashed data, on every call to
> measure IMA will add a new entry in the IMA log. This would fill up the
> IMA log - even when there is no change in the measured data.
>
> To avoid that I keep the last measurement in SELinux and measure only
> when there is a change with the timestamp in the event name.
>
> > I can see the appraiser wanting to know two things:
> > 1) The current state of the system (e.g. is it enforcing, is the
> > currently loaded policy the expected one?).
> > 2) Has the system ever been in an unexpected state (e.g. was it
> > temporarily switched to permissive or had an unexpected policy
> > loaded?)
>
> Yes - you are right.
> The appraiser will have to look at the entire IMA log (and the
> corresponding TPM PCR data) to know the above.
>
> Time t0 => State of the system measured
> Time tn => State changed and the new state measured
> Time tm => State changed again and the new state measured.
>
> Say, the measurement at "Time tn" was an illegal change, the appraiser
> would know.
>
> >
> > I applied the patch series on top of the next-integrity branch, added
> > measure func=LSM_STATE to ima-policy, and booted that kernel. I get
> > the following entries in ascii_runtime_measurements, but seemingly
> > missing the final field:
> >
> > 10 8a09c48af4f8a817f59b495bd82971e096e2e367 ima-ng
> > sha256:21c3d7b09b62b4d0b3ed15ba990f816b94808f90b76787bfae755c4b3a44cd24
> > selinux-state
> > 10 e610908931d70990a2855ddb33c16af2d82ce56a ima-ng
> > sha256:c8898652afd5527ef4eaf8d85f5fee1d91fcccee34bc97f6e55b96746bedb318
> > selinux-policy-hash
> >
> > Thus, I cannot verify. What am I missing?
> >
>
> Looks like the template used is ima-ng which doesn't include the
> measured buffer. Please set template to "ima-buf" in the policy.
>
> For example,
> measure func=LSM_STATE template=ima-buf
It seems like one shouldn't need to manually specify it if it is the
only template that yields a useful result for the LSM_STATE function?
^ permalink raw reply
* Re: [PATCH v3 4/5] LSM: Define SELinux function to measure security state
From: Stephen Smalley @ 2020-07-20 17:49 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Mimi Zohar, Casey Schaufler, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <CAEjxPJ6Rt7u3shLbxoPRHgr-D=CD9d_eXRB07A9qN7RmJwZAwA@mail.gmail.com>
On Mon, Jul 20, 2020 at 1:40 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Mon, Jul 20, 2020 at 1:34 PM Lakshmi Ramasubramanian
> <nramas@linux.microsoft.com> wrote:
> >
> > On 7/20/20 10:06 AM, Stephen Smalley wrote:
> >
> > >> The above will ensure the following sequence will be measured:
> > >> #1 State A - Measured
> > >> #2 Change from State A to State B - Measured
> > >> #3 Change from State B back to State A - Since the measured data is
> > >> same as in #1, the change will be measured only if the event name is
> > >> different between #1 and #3
> > >
> > > Perhaps the timestamp / sequence number should be part of the hashed
> > > data instead of the event name?
> >
> > If the timestamp/seqno is part of the hashed data, on every call to
> > measure IMA will add a new entry in the IMA log. This would fill up the
> > IMA log - even when there is no change in the measured data.
> >
> > To avoid that I keep the last measurement in SELinux and measure only
> > when there is a change with the timestamp in the event name.
> >
> > > I can see the appraiser wanting to know two things:
> > > 1) The current state of the system (e.g. is it enforcing, is the
> > > currently loaded policy the expected one?).
> > > 2) Has the system ever been in an unexpected state (e.g. was it
> > > temporarily switched to permissive or had an unexpected policy
> > > loaded?)
> >
> > Yes - you are right.
> > The appraiser will have to look at the entire IMA log (and the
> > corresponding TPM PCR data) to know the above.
> >
> > Time t0 => State of the system measured
> > Time tn => State changed and the new state measured
> > Time tm => State changed again and the new state measured.
> >
> > Say, the measurement at "Time tn" was an illegal change, the appraiser
> > would know.
> >
> > >
> > > I applied the patch series on top of the next-integrity branch, added
> > > measure func=LSM_STATE to ima-policy, and booted that kernel. I get
> > > the following entries in ascii_runtime_measurements, but seemingly
> > > missing the final field:
> > >
> > > 10 8a09c48af4f8a817f59b495bd82971e096e2e367 ima-ng
> > > sha256:21c3d7b09b62b4d0b3ed15ba990f816b94808f90b76787bfae755c4b3a44cd24
> > > selinux-state
> > > 10 e610908931d70990a2855ddb33c16af2d82ce56a ima-ng
> > > sha256:c8898652afd5527ef4eaf8d85f5fee1d91fcccee34bc97f6e55b96746bedb318
> > > selinux-policy-hash
> > >
> > > Thus, I cannot verify. What am I missing?
> > >
> >
> > Looks like the template used is ima-ng which doesn't include the
> > measured buffer. Please set template to "ima-buf" in the policy.
> >
> > For example,
> > measure func=LSM_STATE template=ima-buf
>
> It seems like one shouldn't need to manually specify it if it is the
> only template that yields a useful result for the LSM_STATE function?
Actually, if we used ima-ng template for selinux-policy-hash, then
instead of needing to hash the policy
first and passing the hash to IMA, we could just pass the policy as
the buffer and IMA would take care of the hashing, right?
And we only need to use ima-buf for the selinux-state if we want the
measurement list to include the string value that
was hashed; if we just want to compare against a known-good, it would
suffice to use ima-ng for it as well, right?
^ permalink raw reply
* [PATCH 1/2] LSM: Signal to SafeSetID when in set*gid syscall
From: Micah Morton @ 2020-07-20 18:11 UTC (permalink / raw)
To: linux-security-module
Cc: keescook, casey, paul, stephen.smalley.work, serge, jmorris,
Thomas Cedeno, Micah Morton
From: Thomas Cedeno <thomascedeno@google.com>
For SafeSetID to properly gate set*gid() calls, it needs to know whether
ns_capable() is being called from within a sys_set*gid() function or is
being called from elsewhere in the kernel. This allows SafeSetID to deny
CAP_SETGID to restricted groups when they are attempting to use the
capability for code paths other than updating GIDs (e.g. setting up
userns GID mappings). This is the identical approach to what is
currently done for CAP_SETUID.
Signed-off-by: Thomas Cedeno <thomascedeno@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
kernel/sys.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/kernel/sys.c b/kernel/sys.c
index 00a96746e28a..55e0c86772ab 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -373,7 +373,7 @@ long __sys_setregid(gid_t rgid, gid_t egid)
if (rgid != (gid_t) -1) {
if (gid_eq(old->gid, krgid) ||
gid_eq(old->egid, krgid) ||
- ns_capable(old->user_ns, CAP_SETGID))
+ ns_capable_setid(old->user_ns, CAP_SETGID))
new->gid = krgid;
else
goto error;
@@ -382,7 +382,7 @@ long __sys_setregid(gid_t rgid, gid_t egid)
if (gid_eq(old->gid, kegid) ||
gid_eq(old->egid, kegid) ||
gid_eq(old->sgid, kegid) ||
- ns_capable(old->user_ns, CAP_SETGID))
+ ns_capable_setid(old->user_ns, CAP_SETGID))
new->egid = kegid;
else
goto error;
@@ -432,7 +432,7 @@ long __sys_setgid(gid_t gid)
old = current_cred();
retval = -EPERM;
- if (ns_capable(old->user_ns, CAP_SETGID))
+ if (ns_capable_setid(old->user_ns, CAP_SETGID))
new->gid = new->egid = new->sgid = new->fsgid = kgid;
else if (gid_eq(kgid, old->gid) || gid_eq(kgid, old->sgid))
new->egid = new->fsgid = kgid;
@@ -744,7 +744,7 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid)
old = current_cred();
retval = -EPERM;
- if (!ns_capable(old->user_ns, CAP_SETGID)) {
+ if (!ns_capable_setid(old->user_ns, CAP_SETGID)) {
if (rgid != (gid_t) -1 && !gid_eq(krgid, old->gid) &&
!gid_eq(krgid, old->egid) && !gid_eq(krgid, old->sgid))
goto error;
@@ -871,7 +871,7 @@ long __sys_setfsgid(gid_t gid)
if (gid_eq(kgid, old->gid) || gid_eq(kgid, old->egid) ||
gid_eq(kgid, old->sgid) || gid_eq(kgid, old->fsgid) ||
- ns_capable(old->user_ns, CAP_SETGID)) {
+ ns_capable_setid(old->user_ns, CAP_SETGID)) {
if (!gid_eq(kgid, old->fsgid)) {
new->fsgid = kgid;
if (security_task_fix_setgid(new,old,LSM_SETID_FS) == 0)
--
2.28.0.rc0.105.gf9edc3c819-goog
^ permalink raw reply related
* [PATCH 2/2] LSM: SafeSetID: Add GID security policy handling
From: Micah Morton @ 2020-07-20 18:12 UTC (permalink / raw)
To: linux-security-module
Cc: keescook, casey, paul, stephen.smalley.work, serge, jmorris,
Thomas Cedeno, Micah Morton
From: Thomas Cedeno <thomascedeno@google.com>
The SafeSetID LSM has functionality for restricting setuid() calls based
on its configured security policies. This patch adds the analogous
functionality for setgid() calls. This is mostly a copy-and-paste change
with some code deduplication, plus slight modifications/name changes to
the policy-rule-related structs (now contain GID rules in addition to
the UID ones) and some type generalization since SafeSetID now needs to
deal with kgid_t and kuid_t types.
Signed-off-by: Thomas Cedeno <thomascedeno@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
NOTE: Looks like some userns-related lines in the selftest for SafeSetID
recently had some kind of regression. We won't be sending a patch to
update the selftest until we can get to the bottom of that. However, we
have a WIP (due to the userns regression) update to the selftest which
tests the GID stuff and we have used it to ensure this patch is correct.
security/safesetid/lsm.c | 178 +++++++++++++++++++++-------
security/safesetid/lsm.h | 38 ++++--
security/safesetid/securityfs.c | 198 +++++++++++++++++++++++---------
3 files changed, 309 insertions(+), 105 deletions(-)
diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
index 7760019ad35d..787a98e82f1e 100644
--- a/security/safesetid/lsm.c
+++ b/security/safesetid/lsm.c
@@ -24,20 +24,36 @@
/* Flag indicating whether initialization completed */
int safesetid_initialized;
-struct setuid_ruleset __rcu *safesetid_setuid_rules;
+struct setid_ruleset __rcu *safesetid_setuid_rules;
+struct setid_ruleset __rcu *safesetid_setgid_rules;
+
/* Compute a decision for a transition from @src to @dst under @policy. */
-enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy,
- kuid_t src, kuid_t dst)
+enum sid_policy_type _setid_policy_lookup(struct setid_ruleset *policy,
+ kid_t src, kid_t dst)
{
- struct setuid_rule *rule;
+ struct setid_rule *rule;
enum sid_policy_type result = SIDPOL_DEFAULT;
- hash_for_each_possible(policy->rules, rule, next, __kuid_val(src)) {
- if (!uid_eq(rule->src_uid, src))
- continue;
- if (uid_eq(rule->dst_uid, dst))
- return SIDPOL_ALLOWED;
+ if (policy->type == UID) {
+ hash_for_each_possible(policy->rules, rule, next, __kuid_val(src.uid)) {
+ if (!uid_eq(rule->src_id.uid, src.uid))
+ continue;
+ if (uid_eq(rule->dst_id.uid, dst.uid))
+ return SIDPOL_ALLOWED;
+ result = SIDPOL_CONSTRAINED;
+ }
+ } else if (policy->type == GID) {
+ hash_for_each_possible(policy->rules, rule, next, __kgid_val(src.gid)) {
+ if (!gid_eq(rule->src_id.gid, src.gid))
+ continue;
+ if (gid_eq(rule->dst_id.gid, dst.gid)){
+ return SIDPOL_ALLOWED;
+ }
+ result = SIDPOL_CONSTRAINED;
+ }
+ } else {
+ /* Should not reach here, report the ID as contrainsted */
result = SIDPOL_CONSTRAINED;
}
return result;
@@ -47,15 +63,26 @@ enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy,
* Compute a decision for a transition from @src to @dst under the active
* policy.
*/
-static enum sid_policy_type setuid_policy_lookup(kuid_t src, kuid_t dst)
+static enum sid_policy_type setid_policy_lookup(kid_t src, kid_t dst, enum setid_type new_type)
{
enum sid_policy_type result = SIDPOL_DEFAULT;
- struct setuid_ruleset *pol;
+ struct setid_ruleset *pol;
rcu_read_lock();
- pol = rcu_dereference(safesetid_setuid_rules);
- if (pol)
- result = _setuid_policy_lookup(pol, src, dst);
+ if (new_type == UID)
+ pol = rcu_dereference(safesetid_setuid_rules);
+ else if (new_type == GID)
+ pol = rcu_dereference(safesetid_setgid_rules);
+ else { /* Should not reach here */
+ result = SIDPOL_CONSTRAINED;
+ rcu_read_unlock();
+ return result;
+ }
+
+ if (pol) {
+ pol->type = new_type;
+ result = _setid_policy_lookup(pol, src, dst);
+ }
rcu_read_unlock();
return result;
}
@@ -65,8 +92,8 @@ static int safesetid_security_capable(const struct cred *cred,
int cap,
unsigned int opts)
{
- /* We're only interested in CAP_SETUID. */
- if (cap != CAP_SETUID)
+ /* We're only interested in CAP_SETUID and CAP_SETGID. */
+ if (cap != CAP_SETUID && cap != CAP_SETGID)
return 0;
/*
@@ -77,45 +104,83 @@ static int safesetid_security_capable(const struct cred *cred,
if ((opts & CAP_OPT_INSETID) != 0)
return 0;
- /*
- * If no policy applies to this task, allow the use of CAP_SETUID for
- * other purposes.
- */
- if (setuid_policy_lookup(cred->uid, INVALID_UID) == SIDPOL_DEFAULT)
+ switch (cap) {
+ case CAP_SETUID:
+ /*
+ * If no policy applies to this task, allow the use of CAP_SETUID for
+ * other purposes.
+ */
+ if (setid_policy_lookup((kid_t)cred->uid, INVALID_ID, UID) == SIDPOL_DEFAULT)
+ return 0;
+ /*
+ * Reject use of CAP_SETUID for functionality other than calling
+ * set*uid() (e.g. setting up userns uid mappings).
+ */
+ pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
+ __kuid_val(cred->uid));
+ return -EPERM;
+ break;
+ case CAP_SETGID:
+ /*
+ * If no policy applies to this task, allow the use of CAP_SETGID for
+ * other purposes.
+ */
+ if (setid_policy_lookup((kid_t)cred->gid, INVALID_ID, GID) == SIDPOL_DEFAULT)
+ return 0;
+ /*
+ * Reject use of CAP_SETUID for functionality other than calling
+ * set*gid() (e.g. setting up userns gid mappings).
+ */
+ pr_warn("Operation requires CAP_SETGID, which is not available to GID %u for operations besides approved set*gid transitions\n",
+ __kuid_val(cred->uid));
+ return -EPERM;
+ break;
+ default:
+ /* Error, the only capabilities were checking for is CAP_SETUID/GID */
return 0;
-
- /*
- * Reject use of CAP_SETUID for functionality other than calling
- * set*uid() (e.g. setting up userns uid mappings).
- */
- pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
- __kuid_val(cred->uid));
- return -EPERM;
+ break;
+ }
+ return 0;
}
/*
* Check whether a caller with old credentials @old is allowed to switch to
- * credentials that contain @new_uid.
+ * credentials that contain @new_id.
*/
-static bool uid_permitted_for_cred(const struct cred *old, kuid_t new_uid)
+static bool id_permitted_for_cred(const struct cred *old, kid_t new_id, enum setid_type new_type)
{
bool permitted;
- /* If our old creds already had this UID in it, it's fine. */
- if (uid_eq(new_uid, old->uid) || uid_eq(new_uid, old->euid) ||
- uid_eq(new_uid, old->suid))
- return true;
+ /* If our old creds already had this ID in it, it's fine. */
+ if (new_type == UID) {
+ if (uid_eq(new_id.uid, old->uid) || uid_eq(new_id.uid, old->euid) ||
+ uid_eq(new_id.uid, old->suid))
+ return true;
+ } else if (new_type == GID){
+ if (gid_eq(new_id.gid, old->gid) || gid_eq(new_id.gid, old->egid) ||
+ gid_eq(new_id.gid, old->sgid))
+ return true;
+ } else /* Error, new_type is an invalid type */
+ return false;
/*
* Transitions to new UIDs require a check against the policy of the old
* RUID.
*/
permitted =
- setuid_policy_lookup(old->uid, new_uid) != SIDPOL_CONSTRAINED;
+ setid_policy_lookup((kid_t)old->uid, new_id, new_type) != SIDPOL_CONSTRAINED;
+
if (!permitted) {
- pr_warn("UID transition ((%d,%d,%d) -> %d) blocked\n",
- __kuid_val(old->uid), __kuid_val(old->euid),
- __kuid_val(old->suid), __kuid_val(new_uid));
+ if (new_type == UID) {
+ pr_warn("UID transition ((%d,%d,%d) -> %d) blocked\n",
+ __kuid_val(old->uid), __kuid_val(old->euid),
+ __kuid_val(old->suid), __kuid_val(new_id.uid));
+ } else if (new_type == GID) {
+ pr_warn("GID transition ((%d,%d,%d) -> %d) blocked\n",
+ __kgid_val(old->gid), __kgid_val(old->egid),
+ __kgid_val(old->sgid), __kgid_val(new_id.gid));
+ } else /* Error, new_type is an invalid type */
+ return false;
}
return permitted;
}
@@ -131,13 +196,37 @@ static int safesetid_task_fix_setuid(struct cred *new,
{
/* Do nothing if there are no setuid restrictions for our old RUID. */
- if (setuid_policy_lookup(old->uid, INVALID_UID) == SIDPOL_DEFAULT)
+ if (setid_policy_lookup((kid_t)old->uid, INVALID_ID, UID) == SIDPOL_DEFAULT)
+ return 0;
+
+ if (id_permitted_for_cred(old, (kid_t)new->uid, UID) &&
+ id_permitted_for_cred(old, (kid_t)new->euid, UID) &&
+ id_permitted_for_cred(old, (kid_t)new->suid, UID) &&
+ id_permitted_for_cred(old, (kid_t)new->fsuid, UID))
+ return 0;
+
+ /*
+ * Kill this process to avoid potential security vulnerabilities
+ * that could arise from a missing whitelist entry preventing a
+ * privileged process from dropping to a lesser-privileged one.
+ */
+ force_sig(SIGKILL);
+ return -EACCES;
+}
+
+static int safesetid_task_fix_setgid(struct cred *new,
+ const struct cred *old,
+ int flags)
+{
+
+ /* Do nothing if there are no setgid restrictions for our old RGID. */
+ if (setid_policy_lookup((kid_t)old->gid, INVALID_ID, GID) == SIDPOL_DEFAULT)
return 0;
- if (uid_permitted_for_cred(old, new->uid) &&
- uid_permitted_for_cred(old, new->euid) &&
- uid_permitted_for_cred(old, new->suid) &&
- uid_permitted_for_cred(old, new->fsuid))
+ if (id_permitted_for_cred(old, (kid_t)new->gid, GID) &&
+ id_permitted_for_cred(old, (kid_t)new->egid, GID) &&
+ id_permitted_for_cred(old, (kid_t)new->sgid, GID) &&
+ id_permitted_for_cred(old, (kid_t)new->fsgid, GID))
return 0;
/*
@@ -151,6 +240,7 @@ static int safesetid_task_fix_setuid(struct cred *new,
static struct security_hook_list safesetid_security_hooks[] = {
LSM_HOOK_INIT(task_fix_setuid, safesetid_task_fix_setuid),
+ LSM_HOOK_INIT(task_fix_setgid, safesetid_task_fix_setgid),
LSM_HOOK_INIT(capable, safesetid_security_capable)
};
diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
index db6d16e6bbc3..bde8c43a3767 100644
--- a/security/safesetid/lsm.h
+++ b/security/safesetid/lsm.h
@@ -27,27 +27,47 @@ enum sid_policy_type {
SIDPOL_ALLOWED /* target ID explicitly allowed */
};
+typedef union {
+ kuid_t uid;
+ kgid_t gid;
+} kid_t;
+
+enum setid_type {
+ UID,
+ GID
+};
+
/*
- * Hash table entry to store safesetid policy signifying that 'src_uid'
- * can setuid to 'dst_uid'.
+ * Hash table entry to store safesetid policy signifying that 'src_id'
+ * can set*id to 'dst_id'.
*/
-struct setuid_rule {
+struct setid_rule {
struct hlist_node next;
- kuid_t src_uid;
- kuid_t dst_uid;
+ kid_t src_id;
+ kid_t dst_id;
+
+ /* Flag to signal if rule is for UID's or GID's */
+ enum setid_type type;
};
#define SETID_HASH_BITS 8 /* 256 buckets in hash table */
-struct setuid_ruleset {
+/* Extension of INVALID_UID/INVALID_GID for kid_t type */
+#define INVALID_ID (kid_t){.uid = INVALID_UID}
+
+struct setid_ruleset {
DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
char *policy_str;
struct rcu_head rcu;
+
+ //Flag to signal if ruleset is for UID's or GID's
+ enum setid_type type;
};
-enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy,
- kuid_t src, kuid_t dst);
+enum sid_policy_type _setid_policy_lookup(struct setid_ruleset *policy,
+ kid_t src, kid_t dst);
-extern struct setuid_ruleset __rcu *safesetid_setuid_rules;
+extern struct setid_ruleset __rcu *safesetid_setuid_rules;
+extern struct setid_ruleset __rcu *safesetid_setgid_rules;
#endif /* _SAFESETID_H */
diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index f8bc574cea9c..211050d0a922 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -19,22 +19,23 @@
#include "lsm.h"
-static DEFINE_MUTEX(policy_update_lock);
+static DEFINE_MUTEX(uid_policy_update_lock);
+static DEFINE_MUTEX(gid_policy_update_lock);
/*
- * In the case the input buffer contains one or more invalid UIDs, the kuid_t
+ * In the case the input buffer contains one or more invalid IDs, the kid_t
* variables pointed to by @parent and @child will get updated but this
* function will return an error.
* Contents of @buf may be modified.
*/
static int parse_policy_line(struct file *file, char *buf,
- struct setuid_rule *rule)
+ struct setid_rule *rule)
{
char *child_str;
int ret;
u32 parsed_parent, parsed_child;
- /* Format of |buf| string should be <UID>:<UID>. */
+ /* Format of |buf| string should be <UID>:<UID> or <GID>:<GID> */
child_str = strchr(buf, ':');
if (child_str == NULL)
return -EINVAL;
@@ -49,20 +50,36 @@ static int parse_policy_line(struct file *file, char *buf,
if (ret)
return ret;
- rule->src_uid = make_kuid(file->f_cred->user_ns, parsed_parent);
- rule->dst_uid = make_kuid(file->f_cred->user_ns, parsed_child);
- if (!uid_valid(rule->src_uid) || !uid_valid(rule->dst_uid))
+ if (rule->type == UID){
+ rule->src_id.uid = make_kuid(file->f_cred->user_ns, parsed_parent);
+ rule->dst_id.uid = make_kuid(file->f_cred->user_ns, parsed_child);
+ } else if (rule->type == GID){
+ rule->src_id.gid = make_kgid(file->f_cred->user_ns, parsed_parent);
+ rule->dst_id.gid = make_kgid(file->f_cred->user_ns, parsed_child);
+ } else {
+ /* Error, rule->type is an invalid type */
return -EINVAL;
+ }
+ if (rule->type == UID) {
+ if (!uid_valid(rule->src_id.uid) || !uid_valid(rule->dst_id.uid))
+ return -EINVAL;
+ } else if (rule->type == GID) {
+ if (!gid_valid(rule->src_id.gid) || !gid_valid(rule->dst_id.gid))
+ return -EINVAL;
+ } else {
+ /* Error, rule->type is an invalid type */
+ return -EINVAL;
+ }
return 0;
}
static void __release_ruleset(struct rcu_head *rcu)
{
- struct setuid_ruleset *pol =
- container_of(rcu, struct setuid_ruleset, rcu);
+ struct setid_ruleset *pol =
+ container_of(rcu, struct setid_ruleset, rcu);
int bucket;
- struct setuid_rule *rule;
+ struct setid_rule *rule;
struct hlist_node *tmp;
hash_for_each_safe(pol->rules, bucket, tmp, rule, next)
@@ -71,36 +88,58 @@ static void __release_ruleset(struct rcu_head *rcu)
kfree(pol);
}
-static void release_ruleset(struct setuid_ruleset *pol)
-{
+static void release_ruleset(struct setid_ruleset *pol){
call_rcu(&pol->rcu, __release_ruleset);
}
-static void insert_rule(struct setuid_ruleset *pol, struct setuid_rule *rule)
+static void insert_rule(struct setid_ruleset *pol, struct setid_rule *rule)
{
- hash_add(pol->rules, &rule->next, __kuid_val(rule->src_uid));
+ if (pol->type == UID)
+ hash_add(pol->rules, &rule->next, __kuid_val(rule->src_id.uid));
+ else if (pol->type == GID)
+ hash_add(pol->rules, &rule->next, __kgid_val(rule->src_id.gid));
+ else /* Error, pol->type is neither UID or GID */
+ return;
}
-static int verify_ruleset(struct setuid_ruleset *pol)
+static int verify_ruleset(struct setid_ruleset *pol)
{
int bucket;
- struct setuid_rule *rule, *nrule;
+ struct setid_rule *rule, *nrule;
int res = 0;
hash_for_each(pol->rules, bucket, rule, next) {
- if (_setuid_policy_lookup(pol, rule->dst_uid, INVALID_UID) ==
- SIDPOL_DEFAULT) {
- pr_warn("insecure policy detected: uid %d is constrained but transitively unconstrained through uid %d\n",
- __kuid_val(rule->src_uid),
- __kuid_val(rule->dst_uid));
+ if (_setid_policy_lookup(pol, rule->dst_id, INVALID_ID) == SIDPOL_DEFAULT) {
+ if (pol->type == UID) {
+ pr_warn("insecure policy detected: uid %d is constrained but transitively unconstrained through uid %d\n",
+ __kuid_val(rule->src_id.uid),
+ __kuid_val(rule->dst_id.uid));
+ } else if (pol->type == GID) {
+ pr_warn("insecure policy detected: gid %d is constrained but transitively unconstrained through gid %d\n",
+ __kgid_val(rule->src_id.gid),
+ __kgid_val(rule->dst_id.gid));
+ } else { /* pol->type is an invalid type */
+ res = -EINVAL;
+ return res;
+ }
res = -EINVAL;
/* fix it up */
- nrule = kmalloc(sizeof(struct setuid_rule), GFP_KERNEL);
+ nrule = kmalloc(sizeof(struct setid_rule), GFP_KERNEL);
if (!nrule)
return -ENOMEM;
- nrule->src_uid = rule->dst_uid;
- nrule->dst_uid = rule->dst_uid;
+ if (pol->type == UID){
+ nrule->src_id.uid = rule->dst_id.uid;
+ nrule->dst_id.uid = rule->dst_id.uid;
+ nrule->type = UID;
+ } else if (pol->type == GID){
+ nrule->src_id.gid = rule->dst_id.gid;
+ nrule->dst_id.gid = rule->dst_id.gid;
+ nrule->type = GID;
+ } else { /* Error, pol->type is neither UID or GID */
+ kfree(nrule);
+ return res;
+ }
insert_rule(pol, nrule);
}
}
@@ -108,16 +147,17 @@ static int verify_ruleset(struct setuid_ruleset *pol)
}
static ssize_t handle_policy_update(struct file *file,
- const char __user *ubuf, size_t len)
+ const char __user *ubuf, size_t len, enum setid_type policy_type)
{
- struct setuid_ruleset *pol;
+ struct setid_ruleset *pol;
char *buf, *p, *end;
int err;
- pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL);
+ pol = kmalloc(sizeof(struct setid_ruleset), GFP_KERNEL);
if (!pol)
return -ENOMEM;
pol->policy_str = NULL;
+ pol->type = policy_type;
hash_init(pol->rules);
p = buf = memdup_user_nul(ubuf, len);
@@ -133,7 +173,7 @@ static ssize_t handle_policy_update(struct file *file,
/* policy lines, including the last one, end with \n */
while (*p != '\0') {
- struct setuid_rule *rule;
+ struct setid_rule *rule;
end = strchr(p, '\n');
if (end == NULL) {
@@ -142,18 +182,18 @@ static ssize_t handle_policy_update(struct file *file,
}
*end = '\0';
- rule = kmalloc(sizeof(struct setuid_rule), GFP_KERNEL);
+ rule = kmalloc(sizeof(struct setid_rule), GFP_KERNEL);
if (!rule) {
err = -ENOMEM;
goto out_free_buf;
}
+ rule->type = policy_type;
err = parse_policy_line(file, p, rule);
if (err)
goto out_free_rule;
- if (_setuid_policy_lookup(pol, rule->src_uid, rule->dst_uid) ==
- SIDPOL_ALLOWED) {
+ if (_setid_policy_lookup(pol, rule->src_id, rule->dst_id) == SIDPOL_ALLOWED) {
pr_warn("bad policy: duplicate entry\n");
err = -EEXIST;
goto out_free_rule;
@@ -178,21 +218,45 @@ static ssize_t handle_policy_update(struct file *file,
* What we really want here is an xchg() wrapper for RCU, but since that
* doesn't currently exist, just use a spinlock for now.
*/
- mutex_lock(&policy_update_lock);
- pol = rcu_replace_pointer(safesetid_setuid_rules, pol,
- lockdep_is_held(&policy_update_lock));
- mutex_unlock(&policy_update_lock);
+ if (policy_type == UID) {
+ mutex_lock(&uid_policy_update_lock);
+ pol = rcu_replace_pointer(safesetid_setuid_rules, pol,
+ lockdep_is_held(&uid_policy_update_lock));
+ mutex_unlock(&uid_policy_update_lock);
+ } else if (policy_type == GID) {
+ mutex_lock(&gid_policy_update_lock);
+ pol = rcu_replace_pointer(safesetid_setgid_rules, pol,
+ lockdep_is_held(&gid_policy_update_lock));
+ mutex_unlock(&gid_policy_update_lock);
+ } else {
+ /* Error, policy type is neither UID or GID */
+ pr_warn("error: bad policy type");
+ }
err = len;
out_free_buf:
kfree(buf);
out_free_pol:
if (pol)
- release_ruleset(pol);
+ release_ruleset(pol);
return err;
}
-static ssize_t safesetid_file_write(struct file *file,
+static ssize_t safesetid_uid_file_write(struct file *file,
+ const char __user *buf,
+ size_t len,
+ loff_t *ppos)
+{
+ if (!file_ns_capable(file, &init_user_ns, CAP_MAC_ADMIN))
+ return -EPERM;
+
+ if (*ppos != 0)
+ return -EINVAL;
+
+ return handle_policy_update(file, buf, len, UID);
+}
+
+static ssize_t safesetid_gid_file_write(struct file *file,
const char __user *buf,
size_t len,
loff_t *ppos)
@@ -203,38 +267,60 @@ static ssize_t safesetid_file_write(struct file *file,
if (*ppos != 0)
return -EINVAL;
- return handle_policy_update(file, buf, len);
+ return handle_policy_update(file, buf, len, GID);
}
static ssize_t safesetid_file_read(struct file *file, char __user *buf,
- size_t len, loff_t *ppos)
+ size_t len, loff_t *ppos, struct mutex *policy_update_lock, struct setid_ruleset* ruleset)
{
ssize_t res = 0;
- struct setuid_ruleset *pol;
+ struct setid_ruleset *pol;
const char *kbuf;
- mutex_lock(&policy_update_lock);
- pol = rcu_dereference_protected(safesetid_setuid_rules,
- lockdep_is_held(&policy_update_lock));
+ mutex_lock(policy_update_lock);
+ pol = rcu_dereference_protected(ruleset, lockdep_is_held(&policy_update_lock));
if (pol) {
kbuf = pol->policy_str;
res = simple_read_from_buffer(buf, len, ppos,
kbuf, strlen(kbuf));
}
- mutex_unlock(&policy_update_lock);
+ mutex_unlock(policy_update_lock);
+
return res;
}
-static const struct file_operations safesetid_file_fops = {
- .read = safesetid_file_read,
- .write = safesetid_file_write,
+static ssize_t safesetid_uid_file_read(struct file *file, char __user *buf,
+ size_t len, loff_t *ppos)
+{
+ return safesetid_file_read(file, buf, len, ppos,
+ &uid_policy_update_lock, safesetid_setuid_rules);
+}
+
+static ssize_t safesetid_gid_file_read(struct file *file, char __user *buf,
+ size_t len, loff_t *ppos)
+{
+ return safesetid_file_read(file, buf, len, ppos,
+ &gid_policy_update_lock, safesetid_setgid_rules);
+}
+
+
+
+static const struct file_operations safesetid_uid_file_fops = {
+ .read = safesetid_uid_file_read,
+ .write = safesetid_uid_file_write,
+};
+
+static const struct file_operations safesetid_gid_file_fops = {
+ .read = safesetid_gid_file_read,
+ .write = safesetid_gid_file_write,
};
static int __init safesetid_init_securityfs(void)
{
int ret;
struct dentry *policy_dir;
- struct dentry *policy_file;
+ struct dentry *uid_policy_file;
+ struct dentry *gid_policy_file;
if (!safesetid_initialized)
return 0;
@@ -245,13 +331,21 @@ static int __init safesetid_init_securityfs(void)
goto error;
}
- policy_file = securityfs_create_file("whitelist_policy", 0600,
- policy_dir, NULL, &safesetid_file_fops);
- if (IS_ERR(policy_file)) {
- ret = PTR_ERR(policy_file);
+ uid_policy_file = securityfs_create_file("uid_whitelist_policy", 0600,
+ policy_dir, NULL, &safesetid_uid_file_fops);
+ if (IS_ERR(uid_policy_file)) {
+ ret = PTR_ERR(uid_policy_file);
goto error;
}
+ gid_policy_file = securityfs_create_file("gid_whitelist_policy", 0600,
+ policy_dir, NULL, &safesetid_gid_file_fops);
+ if (IS_ERR(gid_policy_file)) {
+ ret = PTR_ERR(gid_policy_file);
+ goto error;
+ }
+
+
return 0;
error:
--
2.28.0.rc0.105.gf9edc3c819-goog
^ permalink raw reply related
* Re: [PATCH v3 4/5] LSM: Define SELinux function to measure security state
From: Lakshmi Ramasubramanian @ 2020-07-20 18:27 UTC (permalink / raw)
To: Stephen Smalley
Cc: Mimi Zohar, Casey Schaufler, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <CAEjxPJ6-jHha+CeqSdQ2O0bpyQe_9buj2ENZz6FNj6S87XSSfg@mail.gmail.com>
On 7/20/20 10:49 AM, Stephen Smalley wrote:
>>>
>>> Looks like the template used is ima-ng which doesn't include the
>>> measured buffer. Please set template to "ima-buf" in the policy.
>>>
>>> For example,
>>> measure func=LSM_STATE template=ima-buf
>>
>> It seems like one shouldn't need to manually specify it if it is the
>> only template that yields a useful result for the LSM_STATE function?
>
> Actually, if we used ima-ng template for selinux-policy-hash, then
> instead of needing to hash the policy
> first and passing the hash to IMA, we could just pass the policy as
> the buffer and IMA would take care of the hashing, right?
That is correct.
The IMA hook I've added to measure LSM structures is a generic one that
can be used by any security module (SM). I feel it would be better to
not have policy or state or any such SM specific logic in IMA, but leave
that to the individual SM to handle.
What do you think?
> And we only need to use ima-buf for the selinux-state if we want the
> measurement list to include the string value that
> was hashed; if we just want to compare against a known-good, it would
> suffice to use ima-ng for it as well, right?
>
-lakshmi
^ permalink raw reply
* Re: [PATCH v3 4/5] LSM: Define SELinux function to measure security state
From: Stephen Smalley @ 2020-07-20 18:44 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Mimi Zohar, Casey Schaufler, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <dccfc56b-c3ab-327e-19b2-7a70d15afe5b@linux.microsoft.com>
On Mon, Jul 20, 2020 at 2:27 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> On 7/20/20 10:49 AM, Stephen Smalley wrote:
>
> >>>
> >>> Looks like the template used is ima-ng which doesn't include the
> >>> measured buffer. Please set template to "ima-buf" in the policy.
> >>>
> >>> For example,
> >>> measure func=LSM_STATE template=ima-buf
> >>
> >> It seems like one shouldn't need to manually specify it if it is the
> >> only template that yields a useful result for the LSM_STATE function?
> >
> > Actually, if we used ima-ng template for selinux-policy-hash, then
> > instead of needing to hash the policy
> > first and passing the hash to IMA, we could just pass the policy as
> > the buffer and IMA would take care of the hashing, right?
>
> That is correct.
>
> The IMA hook I've added to measure LSM structures is a generic one that
> can be used by any security module (SM). I feel it would be better to
> not have policy or state or any such SM specific logic in IMA, but leave
> that to the individual SM to handle.
>
> What do you think?
It is correct to remain security module agnostic. However, I think
you can remain LSM-neutral while still avoiding the double hashing of
the policy here. Can't you just pass in the policy itself as the
buffer and let IMA hash it? Then you can let the policy author decide
on the template to be used (ima-buf versus ima-ng). If you want to
support the use of different templates for different "kinds" of LSM
state (e.g. state versus policy) you could either provide two funcs
(LSM_STATE, LSM_POLICY) or otherwise support selection based on some
other attribute.
^ permalink raw reply
* Re: [PATCH v3 4/5] LSM: Define SELinux function to measure security state
From: Lakshmi Ramasubramanian @ 2020-07-20 18:59 UTC (permalink / raw)
To: Stephen Smalley
Cc: Mimi Zohar, Casey Schaufler, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <CAEjxPJ6_pxEh6HG9F3r=4B5ZgEpNPkgLHHfJp6ze=F1wKt4wCw@mail.gmail.com>
On 7/20/20 11:44 AM, Stephen Smalley wrote:
>>>
>>> Actually, if we used ima-ng template for selinux-policy-hash, then
>>> instead of needing to hash the policy
>>> first and passing the hash to IMA, we could just pass the policy as
>>> the buffer and IMA would take care of the hashing, right?
>>
>> That is correct.
>>
>> The IMA hook I've added to measure LSM structures is a generic one that
>> can be used by any security module (SM). I feel it would be better to
>> not have policy or state or any such SM specific logic in IMA, but leave
>> that to the individual SM to handle.
>>
>> What do you think?
>
> It is correct to remain security module agnostic. However, I think
> you can remain LSM-neutral while still avoiding the double hashing of
> the policy here. Can't you just pass in the policy itself as the
> buffer and let IMA hash it?
Yes - that is an option. If I do that then, as you have stated below,
we'll need to two funcs -
one that will only add the hash but not the entire data payload in the
IMA log (i.e., "ima-ng")
and, the other that handles hashing and including date payload (i.e.,
"ima-buf").
Then you can let the policy author decide
> on the template to be used (ima-buf versus ima-ng). If you want to
> support the use of different templates for different "kinds" of LSM
> state (e.g. state versus policy) you could either provide two funcs
> (LSM_STATE, LSM_POLICY) or otherwise support selection based on some
> other attribute.
>
I can do the above.
-lakshmi
^ permalink raw reply
* ANN: libseccomp v2.5.0 released
From: Paul Moore @ 2020-07-20 19:49 UTC (permalink / raw)
To: libseccomp, linux-security-module; +Cc: Tom Hromatka
On behalf of the libseccomp project I would like to announce libseccomp v2.5.0!
* https://github.com/seccomp/libseccomp/releases/tag/v2.5.0
The libseccomp v2.5.0 release is backwards compatible with previous
v2.x releases and is a drop-in replacement; no recompilation of
applications is required. Applications will need to be restarted to
take advantage of the new libseccomp release. While the v2.4.x
release stream will be supported for at least one more maintenance
release, all users and distributions are encouraged to upgrade to
libseccomp v2.5.0.
The core libseccomp library is the work of 56 contributors, and this
release is a significant upgrade over the libseccomp v2.4.x release
stream. The v2.5.0 release brings new support for RISC-V and seccomp
user notifications along with a number of bug fixes and performance
improvements. A more detailed list of changes can be seen below:
- Add support for the seccomp user notifications, see the
seccomp_notify_alloc(3), seccomp_notify_receive(3),
seccomp_notify_respond(3) manpages for more information
- Add support for new filter optimization approaches, including a
balanced tree optimization, see the SCMP_FLTATR_CTL_OPTIMIZE filter
attribute for more information
- Add support for the 64-bit RISC-V architecture
- Performance improvements when adding new rules to a filter thanks to
the use of internal shadow transactions and improved syscall lookup
tables
- Properly document the libseccomp API return values and include them
in the stable API promise
- Improvements to the s390 and s390x multiplexed syscall handling
- Multiple fixes and improvements to the libseccomp manpages
- Moved from manually maintained syscall tables to an automatically
generated syscall table in CSV format
- Update the syscall tables to Linux v5.8.0-rc5
- Python bindings and build now default to Python 3.x
- Improvements to the tests have boosted code coverage to over 93%
- Enable Travis CI testing on the aarch64 and ppc64le architectures
- Add code inspection via lgtm.com
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 12/15] Manual pages: cap_get_file.3: NOTES: note the effect of the Ambient set
From: Michael Kerrisk (man-pages) @ 2020-07-20 20:21 UTC (permalink / raw)
To: Andrew G. Morgan; +Cc: LSM List
In-Reply-To: <CALQRfL5-aL3h6M+CYqkVHSpPKQ-i3s+RWD8060AicrGPP3vSfw@mail.gmail.com>
Hi Andrew,
On Mon, 20 Jul 2020 at 17:36, Andrew G. Morgan <morgan@kernel.org> wrote:
>
> I've applied all but this one. This one seems to imply that if the
> effective bit is lowered, but the permitted bits are raised, the
> ambient will have some sort of effect. This isn't how it works. Any
> file caps (even an empty set) suppresses any effect of the ambient
> vector.
Thanks for catching that. I was trying to capture this piece of the
execve() transformation rules:
P'(effective) = F(effective) ? P'(permitted) : P'(ambient)
But of course, I failed to capture the detail that it is the process's
*new* ambient set (which, as you note, is cleared if the file has any
attached capabilities) that is assigned to the effective set. Perhaps
the text is best left as is. If I have some better idea, I'll come
back to you.
Thanks,
Michael
> On Mon, Jul 20, 2020 at 2:14 AM Michael Kerrisk (man-pages)
> <mtk.manpages@gmail.com> wrote:
> >
> > The addition of Ambient capabilities in Linux 4.3 rendered the text on
> > the effect of the Effective bit during execve(2) out-of-date. Fix that.
> > Also add a couple of paragraph breaks to improve readability.
> >
> > Signed-off-by: Michael Kerrisk (man-pages) <mtk.manpages@gmail.com>
> > ---
> > doc/cap_get_file.3 | 11 ++++++++---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/doc/cap_get_file.3 b/doc/cap_get_file.3
> > index ceacbaf..dc7b571 100644
> > --- a/doc/cap_get_file.3
> > +++ b/doc/cap_get_file.3
> > @@ -103,13 +103,18 @@ or
> > These functions are specified by withdrawn POSIX.1e draft specification.
> > .SH NOTES
> > Support for file capabilities is provided on Linux since version 2.6.24.
> > -
> > +.PP
> > On Linux, the file Effective set is a single bit.
> > If it is enabled, then all Permitted capabilities are enabled
> > in the Effective set of the calling process when the file is executed;
> > -otherwise, no capabilities are enabled in the process's Effective set
> > +otherwise, the process's Ambient capabilities
> > +(or, before the Linux 4.3 addition of Ambient capabilities, no capabilities)
> > +are enabled in the process's Effective set
> > following an
> > -.BR execve (2).
> > +.BR execve (2)
> > +(see
> > +.BR capabilities (7)).
> > +.PP
> > Because the file Effective set is a single bit,
> > if any capability is enabled in the Effective set of the
> > .I cap_t
> > --
> > 2.26.2
> >
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support
From: Mimi Zohar @ 2020-07-20 21:38 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin, Sasha Levin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module, Janne Karhunen, Eric Biederman, kexec,
Casey Schaufler, Nayna Jain
In-Reply-To: <20200709061911.954326-1-tyhicks@linux.microsoft.com>
[Cc'ing Sasha]
On Thu, 2020-07-09 at 01:18 -0500, Tyler Hicks wrote:
> I envision patches 1-7 going to stable. The series is ordered in a way
> that has all the fixes up front, followed by cleanups, followed by the
> feature patch. The breakdown of patches looks like so:
>
> Memory leak fixes: 1-3
> Parser strictness fixes: 4-7
> Code cleanups made possible by the fixes: 8-11
> Extend KEXEC_CMDLINE rule support: 12
I agree they should be backported, but they don't apply cleanly before
linux-5.6. The changes aren't that major. Some patch hunks apply
cleanly, but won't compile, while others patch hunks need to be
dropped based on when the feature was upstreamed. For these reasons,
I'm not Cc'ing stable.
Feature upstreamed:
- LSM policy update: linux 5.3
- key command line: linux 5.3
- blacklist: linux 5.5
- keyrings: linux 5.6
For Linux 5.3:
- Dependency on backporting commit 483ec26eed42 ("ima: ima/lsm policy
rule loading logic bug fixes") to apply " ima: Free the entire rule if
it fails to parse".
Mimi
^ permalink raw reply
* Re: [PATCH bpf-next v4 2/4] bpf: Implement bpf_local_storage for inodes
From: KP Singh @ 2020-07-20 22:44 UTC (permalink / raw)
To: KP Singh
Cc: Martin KaFai Lau, linux-kernel, bpf, linux-security-module,
Alexei Starovoitov, Daniel Borkmann, Paul Turner, Jann Horn,
Florent Revest
In-Reply-To: <20200715225911.GA1194150@google.com>
On 16-Jul 00:59, KP Singh wrote:
> On 15-Jul 14:57, Martin KaFai Lau wrote:
> > On Thu, Jul 09, 2020 at 12:12:37PM +0200, KP Singh wrote:
> > > From: KP Singh <kpsingh@google.com>
> > >
> > > Similar to bpf_local_storage for sockets, add local storage for inodes.
> > > The life-cycle of storage is managed with the life-cycle of the inode.
> > > i.e. the storage is destroyed along with the owning inode.
> > >
> > > The BPF LSM allocates an __rcu pointer to the bpf_local_storage in the
> > > security blob which are now stackable and can co-exist with other LSMs.
> > >
> > > Signed-off-by: KP Singh <kpsingh@google.com>
> >
> > [ ... ]
> >
> >
> > > +static void *bpf_inode_storage_lookup_elem(struct bpf_map *map, void *key)
> > > +{
> > > + struct bpf_local_storage_data *sdata;
> > > + struct inode *inode;
> > > + int err = -EINVAL;
> > > +
> > > + if (key) {
> > > + inode = *(struct inode **)(key);
> > The bpf_inode_storage_lookup_elem() here and the (update|delete)_elem() below
> > are called from the userspace syscall. How the userspace may provide this key?
>
> I realized this when I replied about the _fd_ name in the sk helpers.
> I am going to mark them as unsupported for now for inodes.
>
> We could, probably and separately, use a combination of the device
> and inode number as a key from userspace.
I actually implemented these as:
static int bpf_fd_inode_storage_delete_elem(struct bpf_map *map, void *key)
{
struct file *f;
int fd;
fd = *(int *)key;
f = fcheck(fd);
if (!f)
return -EINVAL;
return inode_storage_delete(f->f_inode, map);
}
This keeps it similar to sk_storage and the userspace can just pass an
fd.
- KP
>
> - KP
>
> >
> > > + sdata = inode_storage_lookup(inode, map, true);
> > > + return sdata ? sdata->data : NULL;
> > > + }
> > > +
> > > + return ERR_PTR(err);
> > > +}
> > > +
> > > +static int bpf_inode_storage_update_elem(struct bpf_map *map, void *key,
> > > + void *value, u64 map_flags)
> > > +{
> > > + struct bpf_local_storage_data *sdata;
> > > + struct inode *inode;
> > > + int err = -EINVAL;
> > > +
> > > + if (key) {
> > > + inode = *(struct inode **)(key);
> > > + sdata = map->ops->map_local_storage_update(inode, map, value,
> > > + map_flags);
> > > + return PTR_ERR_OR_ZERO(sdata);
> > > + }
> > > + return err;
> > > +}
> > > +
> > > +static int inode_storage_delete(struct inode *inode, struct bpf_map *map)
> > > +{
> > > + struct bpf_local_storage_data *sdata;
> > > +
> > > + sdata = inode_storage_lookup(inode, map, false);
> > > + if (!sdata)
> > > + return -ENOENT;
> > > +
> > > + bpf_selem_unlink_map_elem(SELEM(sdata));
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int bpf_inode_storage_delete_elem(struct bpf_map *map, void *key)
> > > +{
> > > + struct inode *inode;
> > > + int err = -EINVAL;
> > > +
> > > + if (key) {
> > > + inode = *(struct inode **)(key);
> > > + err = inode_storage_delete(inode, map);
> > > + }
> > > +
> > > + return err;
> > > +}
> > > +
> >
> > [ ... ]
> >
> > > +static int inode_storage_map_btf_id;
> > > +const struct bpf_map_ops inode_storage_map_ops = {
> > > + .map_alloc_check = bpf_local_storage_map_alloc_check,
> > > + .map_alloc = inode_storage_map_alloc,
> > > + .map_free = inode_storage_map_free,
> > > + .map_get_next_key = notsupp_get_next_key,
> > > + .map_lookup_elem = bpf_inode_storage_lookup_elem,
> > > + .map_update_elem = bpf_inode_storage_update_elem,
> > > + .map_delete_elem = bpf_inode_storage_delete_elem,
> > > + .map_check_btf = bpf_local_storage_map_check_btf,
> > > + .map_btf_name = "bpf_local_storage_map",
> > > + .map_btf_id = &inode_storage_map_btf_id,
> > > + .map_local_storage_alloc = inode_storage_alloc,
> > > + .map_selem_alloc = inode_selem_alloc,
> > > + .map_local_storage_update = inode_storage_update,
> > > + .map_local_storage_unlink = unlink_inode_storage,
> > > +};
> > > +
^ permalink raw reply
* Re: [PATCH] Smack: fix use-after-free in smk_write_relabel_self()
From: Eric Biggers @ 2020-07-21 0:38 UTC (permalink / raw)
To: linux-security-module, Casey Schaufler, James Morris,
Serge E . Hallyn
Cc: syzkaller-bugs, linux-kernel, stable, syzbot+e6416dabb497a650da40
In-Reply-To: <20200708201520.140376-1-ebiggers@kernel.org>
On Wed, Jul 08, 2020 at 01:15:20PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> smk_write_relabel_self() frees memory from the task's credentials with
> no locking, which can easily cause a use-after-free because multiple
> tasks can share the same credentials structure.
>
> Fix this by using prepare_creds() and commit_creds() to correctly modify
> the task's credentials.
>
> Reproducer for "BUG: KASAN: use-after-free in smk_write_relabel_self":
>
> #include <fcntl.h>
> #include <pthread.h>
> #include <unistd.h>
>
> static void *thrproc(void *arg)
> {
> int fd = open("/sys/fs/smackfs/relabel-self", O_WRONLY);
> for (;;) write(fd, "foo", 3);
> }
>
> int main()
> {
> pthread_t t;
> pthread_create(&t, NULL, thrproc, NULL);
> thrproc(NULL);
> }
>
> Reported-by: syzbot+e6416dabb497a650da40@syzkaller.appspotmail.com
> Fixes: 38416e53936e ("Smack: limited capability for changing process label")
> Cc: <stable@vger.kernel.org> # v4.4+
> Signed-off-by: Eric Biggers <ebiggers@google.com>
Ping.
^ permalink raw reply
* Re: [PATCH] Smack: fix use-after-free in smk_write_relabel_self()
From: Casey Schaufler @ 2020-07-21 0:57 UTC (permalink / raw)
To: Eric Biggers, linux-security-module, James Morris,
Serge E . Hallyn
Cc: syzkaller-bugs, linux-kernel, stable, syzbot+e6416dabb497a650da40,
Casey Schaufler
In-Reply-To: <20200721003830.GC7464@sol.localdomain>
On 7/20/2020 5:38 PM, Eric Biggers wrote:
> On Wed, Jul 08, 2020 at 01:15:20PM -0700, Eric Biggers wrote:
>> From: Eric Biggers <ebiggers@google.com>
>>
>> smk_write_relabel_self() frees memory from the task's credentials with
>> no locking, which can easily cause a use-after-free because multiple
>> tasks can share the same credentials structure.
>>
>> Fix this by using prepare_creds() and commit_creds() to correctly modify
>> the task's credentials.
>>
>> Reproducer for "BUG: KASAN: use-after-free in smk_write_relabel_self":
>>
>> #include <fcntl.h>
>> #include <pthread.h>
>> #include <unistd.h>
>>
>> static void *thrproc(void *arg)
>> {
>> int fd = open("/sys/fs/smackfs/relabel-self", O_WRONLY);
>> for (;;) write(fd, "foo", 3);
>> }
>>
>> int main()
>> {
>> pthread_t t;
>> pthread_create(&t, NULL, thrproc, NULL);
>> thrproc(NULL);
>> }
>>
>> Reported-by: syzbot+e6416dabb497a650da40@syzkaller.appspotmail.com
>> Fixes: 38416e53936e ("Smack: limited capability for changing process label")
>> Cc: <stable@vger.kernel.org> # v4.4+
>> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Ping.
I have queued your patch and will be pushing it for next shortly.
^ permalink raw reply
* Re: [PATCH 1/2] LSM: Signal to SafeSetID when in set*gid syscall
From: Serge E. Hallyn @ 2020-07-21 2:11 UTC (permalink / raw)
To: Micah Morton
Cc: linux-security-module, keescook, casey, paul,
stephen.smalley.work, serge, jmorris, Thomas Cedeno
In-Reply-To: <20200720181156.1461461-1-mortonm@chromium.org>
On Mon, Jul 20, 2020 at 11:11:56AM -0700, Micah Morton wrote:
> From: Thomas Cedeno <thomascedeno@google.com>
>
> For SafeSetID to properly gate set*gid() calls, it needs to know whether
> ns_capable() is being called from within a sys_set*gid() function or is
> being called from elsewhere in the kernel. This allows SafeSetID to deny
> CAP_SETGID to restricted groups when they are attempting to use the
> capability for code paths other than updating GIDs (e.g. setting up
> userns GID mappings). This is the identical approach to what is
> currently done for CAP_SETUID.
>
> Signed-off-by: Thomas Cedeno <thomascedeno@google.com>
> Signed-off-by: Micah Morton <mortonm@chromium.org>
I see that only safesetid is using that now anyway.
Reviewed-by: Serge Hallyn <serge@hallyn.com>
> ---
> kernel/sys.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 00a96746e28a..55e0c86772ab 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -373,7 +373,7 @@ long __sys_setregid(gid_t rgid, gid_t egid)
> if (rgid != (gid_t) -1) {
> if (gid_eq(old->gid, krgid) ||
> gid_eq(old->egid, krgid) ||
> - ns_capable(old->user_ns, CAP_SETGID))
> + ns_capable_setid(old->user_ns, CAP_SETGID))
> new->gid = krgid;
> else
> goto error;
> @@ -382,7 +382,7 @@ long __sys_setregid(gid_t rgid, gid_t egid)
> if (gid_eq(old->gid, kegid) ||
> gid_eq(old->egid, kegid) ||
> gid_eq(old->sgid, kegid) ||
> - ns_capable(old->user_ns, CAP_SETGID))
> + ns_capable_setid(old->user_ns, CAP_SETGID))
> new->egid = kegid;
> else
> goto error;
> @@ -432,7 +432,7 @@ long __sys_setgid(gid_t gid)
> old = current_cred();
>
> retval = -EPERM;
> - if (ns_capable(old->user_ns, CAP_SETGID))
> + if (ns_capable_setid(old->user_ns, CAP_SETGID))
> new->gid = new->egid = new->sgid = new->fsgid = kgid;
> else if (gid_eq(kgid, old->gid) || gid_eq(kgid, old->sgid))
> new->egid = new->fsgid = kgid;
> @@ -744,7 +744,7 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid)
> old = current_cred();
>
> retval = -EPERM;
> - if (!ns_capable(old->user_ns, CAP_SETGID)) {
> + if (!ns_capable_setid(old->user_ns, CAP_SETGID)) {
> if (rgid != (gid_t) -1 && !gid_eq(krgid, old->gid) &&
> !gid_eq(krgid, old->egid) && !gid_eq(krgid, old->sgid))
> goto error;
> @@ -871,7 +871,7 @@ long __sys_setfsgid(gid_t gid)
>
> if (gid_eq(kgid, old->gid) || gid_eq(kgid, old->egid) ||
> gid_eq(kgid, old->sgid) || gid_eq(kgid, old->fsgid) ||
> - ns_capable(old->user_ns, CAP_SETGID)) {
> + ns_capable_setid(old->user_ns, CAP_SETGID)) {
> if (!gid_eq(kgid, old->fsgid)) {
> new->fsgid = kgid;
> if (security_task_fix_setgid(new,old,LSM_SETID_FS) == 0)
> --
> 2.28.0.rc0.105.gf9edc3c819-goog
^ permalink raw reply
* Re: [PATCH 2/2] LSM: SafeSetID: Add GID security policy handling
From: Serge E. Hallyn @ 2020-07-21 2:42 UTC (permalink / raw)
To: Micah Morton
Cc: linux-security-module, keescook, casey, paul,
stephen.smalley.work, serge, jmorris, Thomas Cedeno
In-Reply-To: <20200720181203.1461548-1-mortonm@chromium.org>
On Mon, Jul 20, 2020 at 11:12:03AM -0700, Micah Morton wrote:
> From: Thomas Cedeno <thomascedeno@google.com>
>
> The SafeSetID LSM has functionality for restricting setuid() calls based
> on its configured security policies. This patch adds the analogous
> functionality for setgid() calls. This is mostly a copy-and-paste change
> with some code deduplication, plus slight modifications/name changes to
> the policy-rule-related structs (now contain GID rules in addition to
> the UID ones) and some type generalization since SafeSetID now needs to
> deal with kgid_t and kuid_t types.
>
> Signed-off-by: Thomas Cedeno <thomascedeno@google.com>
> Signed-off-by: Micah Morton <mortonm@chromium.org>
Just one question and two little comments below.
> ---
> NOTE: Looks like some userns-related lines in the selftest for SafeSetID
> recently had some kind of regression. We won't be sending a patch to
> update the selftest until we can get to the bottom of that. However, we
> have a WIP (due to the userns regression) update to the selftest which
> tests the GID stuff and we have used it to ensure this patch is correct.
>
> security/safesetid/lsm.c | 178 +++++++++++++++++++++-------
> security/safesetid/lsm.h | 38 ++++--
> security/safesetid/securityfs.c | 198 +++++++++++++++++++++++---------
> 3 files changed, 309 insertions(+), 105 deletions(-)
>
> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> index 7760019ad35d..787a98e82f1e 100644
> --- a/security/safesetid/lsm.c
> +++ b/security/safesetid/lsm.c
> @@ -24,20 +24,36 @@
> /* Flag indicating whether initialization completed */
> int safesetid_initialized;
>
> -struct setuid_ruleset __rcu *safesetid_setuid_rules;
> +struct setid_ruleset __rcu *safesetid_setuid_rules;
> +struct setid_ruleset __rcu *safesetid_setgid_rules;
> +
>
> /* Compute a decision for a transition from @src to @dst under @policy. */
> -enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy,
> - kuid_t src, kuid_t dst)
> +enum sid_policy_type _setid_policy_lookup(struct setid_ruleset *policy,
> + kid_t src, kid_t dst)
> {
> - struct setuid_rule *rule;
> + struct setid_rule *rule;
> enum sid_policy_type result = SIDPOL_DEFAULT;
>
> - hash_for_each_possible(policy->rules, rule, next, __kuid_val(src)) {
> - if (!uid_eq(rule->src_uid, src))
> - continue;
> - if (uid_eq(rule->dst_uid, dst))
> - return SIDPOL_ALLOWED;
> + if (policy->type == UID) {
> + hash_for_each_possible(policy->rules, rule, next, __kuid_val(src.uid)) {
> + if (!uid_eq(rule->src_id.uid, src.uid))
> + continue;
> + if (uid_eq(rule->dst_id.uid, dst.uid))
> + return SIDPOL_ALLOWED;
> + result = SIDPOL_CONSTRAINED;
Can you describe precisely under which conditions SIDPOL_CONSTRAINED should
be returned vs. SIDPOL_DEFAULT?
> + }
> + } else if (policy->type == GID) {
> + hash_for_each_possible(policy->rules, rule, next, __kgid_val(src.gid)) {
> + if (!gid_eq(rule->src_id.gid, src.gid))
> + continue;
> + if (gid_eq(rule->dst_id.gid, dst.gid)){
> + return SIDPOL_ALLOWED;
> + }
> + result = SIDPOL_CONSTRAINED;
> + }
> + } else {
> + /* Should not reach here, report the ID as contrainsted */
> result = SIDPOL_CONSTRAINED;
> }
> return result;
> @@ -47,15 +63,26 @@ enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy,
> * Compute a decision for a transition from @src to @dst under the active
> * policy.
> */
> -static enum sid_policy_type setuid_policy_lookup(kuid_t src, kuid_t dst)
> +static enum sid_policy_type setid_policy_lookup(kid_t src, kid_t dst, enum setid_type new_type)
> {
> enum sid_policy_type result = SIDPOL_DEFAULT;
> - struct setuid_ruleset *pol;
> + struct setid_ruleset *pol;
>
> rcu_read_lock();
> - pol = rcu_dereference(safesetid_setuid_rules);
> - if (pol)
> - result = _setuid_policy_lookup(pol, src, dst);
> + if (new_type == UID)
> + pol = rcu_dereference(safesetid_setuid_rules);
> + else if (new_type == GID)
> + pol = rcu_dereference(safesetid_setgid_rules);
> + else { /* Should not reach here */
> + result = SIDPOL_CONSTRAINED;
> + rcu_read_unlock();
> + return result;
> + }
> +
> + if (pol) {
> + pol->type = new_type;
> + result = _setid_policy_lookup(pol, src, dst);
> + }
> rcu_read_unlock();
> return result;
> }
> @@ -65,8 +92,8 @@ static int safesetid_security_capable(const struct cred *cred,
> int cap,
> unsigned int opts)
> {
> - /* We're only interested in CAP_SETUID. */
> - if (cap != CAP_SETUID)
> + /* We're only interested in CAP_SETUID and CAP_SETGID. */
> + if (cap != CAP_SETUID && cap != CAP_SETGID)
> return 0;
>
> /*
> @@ -77,45 +104,83 @@ static int safesetid_security_capable(const struct cred *cred,
> if ((opts & CAP_OPT_INSETID) != 0)
> return 0;
>
> - /*
> - * If no policy applies to this task, allow the use of CAP_SETUID for
> - * other purposes.
> - */
> - if (setuid_policy_lookup(cred->uid, INVALID_UID) == SIDPOL_DEFAULT)
> + switch (cap) {
> + case CAP_SETUID:
> + /*
> + * If no policy applies to this task, allow the use of CAP_SETUID for
> + * other purposes.
> + */
> + if (setid_policy_lookup((kid_t)cred->uid, INVALID_ID, UID) == SIDPOL_DEFAULT)
> + return 0;
> + /*
> + * Reject use of CAP_SETUID for functionality other than calling
> + * set*uid() (e.g. setting up userns uid mappings).
> + */
> + pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> + __kuid_val(cred->uid));
> + return -EPERM;
> + break;
> + case CAP_SETGID:
> + /*
> + * If no policy applies to this task, allow the use of CAP_SETGID for
> + * other purposes.
> + */
> + if (setid_policy_lookup((kid_t)cred->gid, INVALID_ID, GID) == SIDPOL_DEFAULT)
> + return 0;
> + /*
> + * Reject use of CAP_SETUID for functionality other than calling
> + * set*gid() (e.g. setting up userns gid mappings).
> + */
> + pr_warn("Operation requires CAP_SETGID, which is not available to GID %u for operations besides approved set*gid transitions\n",
> + __kuid_val(cred->uid));
> + return -EPERM;
> + break;
> + default:
> + /* Error, the only capabilities were checking for is CAP_SETUID/GID */
> return 0;
> -
> - /*
> - * Reject use of CAP_SETUID for functionality other than calling
> - * set*uid() (e.g. setting up userns uid mappings).
> - */
> - pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> - __kuid_val(cred->uid));
> - return -EPERM;
> + break;
> + }
> + return 0;
> }
>
> /*
> * Check whether a caller with old credentials @old is allowed to switch to
> - * credentials that contain @new_uid.
> + * credentials that contain @new_id.
> */
> -static bool uid_permitted_for_cred(const struct cred *old, kuid_t new_uid)
> +static bool id_permitted_for_cred(const struct cred *old, kid_t new_id, enum setid_type new_type)
> {
> bool permitted;
>
> - /* If our old creds already had this UID in it, it's fine. */
> - if (uid_eq(new_uid, old->uid) || uid_eq(new_uid, old->euid) ||
> - uid_eq(new_uid, old->suid))
> - return true;
> + /* If our old creds already had this ID in it, it's fine. */
> + if (new_type == UID) {
> + if (uid_eq(new_id.uid, old->uid) || uid_eq(new_id.uid, old->euid) ||
> + uid_eq(new_id.uid, old->suid))
> + return true;
> + } else if (new_type == GID){
> + if (gid_eq(new_id.gid, old->gid) || gid_eq(new_id.gid, old->egid) ||
> + gid_eq(new_id.gid, old->sgid))
> + return true;
> + } else /* Error, new_type is an invalid type */
> + return false;
>
> /*
> * Transitions to new UIDs require a check against the policy of the old
> * RUID.
> */
> permitted =
> - setuid_policy_lookup(old->uid, new_uid) != SIDPOL_CONSTRAINED;
> + setid_policy_lookup((kid_t)old->uid, new_id, new_type) != SIDPOL_CONSTRAINED;
> +
> if (!permitted) {
> - pr_warn("UID transition ((%d,%d,%d) -> %d) blocked\n",
> - __kuid_val(old->uid), __kuid_val(old->euid),
> - __kuid_val(old->suid), __kuid_val(new_uid));
> + if (new_type == UID) {
> + pr_warn("UID transition ((%d,%d,%d) -> %d) blocked\n",
> + __kuid_val(old->uid), __kuid_val(old->euid),
> + __kuid_val(old->suid), __kuid_val(new_id.uid));
> + } else if (new_type == GID) {
> + pr_warn("GID transition ((%d,%d,%d) -> %d) blocked\n",
> + __kgid_val(old->gid), __kgid_val(old->egid),
> + __kgid_val(old->sgid), __kgid_val(new_id.gid));
> + } else /* Error, new_type is an invalid type */
> + return false;
> }
> return permitted;
> }
> @@ -131,13 +196,37 @@ static int safesetid_task_fix_setuid(struct cred *new,
> {
>
> /* Do nothing if there are no setuid restrictions for our old RUID. */
> - if (setuid_policy_lookup(old->uid, INVALID_UID) == SIDPOL_DEFAULT)
> + if (setid_policy_lookup((kid_t)old->uid, INVALID_ID, UID) == SIDPOL_DEFAULT)
> + return 0;
> +
> + if (id_permitted_for_cred(old, (kid_t)new->uid, UID) &&
> + id_permitted_for_cred(old, (kid_t)new->euid, UID) &&
> + id_permitted_for_cred(old, (kid_t)new->suid, UID) &&
> + id_permitted_for_cred(old, (kid_t)new->fsuid, UID))
> + return 0;
> +
> + /*
> + * Kill this process to avoid potential security vulnerabilities
> + * that could arise from a missing whitelist entry preventing a
> + * privileged process from dropping to a lesser-privileged one.
> + */
> + force_sig(SIGKILL);
> + return -EACCES;
> +}
> +
> +static int safesetid_task_fix_setgid(struct cred *new,
> + const struct cred *old,
> + int flags)
> +{
> +
> + /* Do nothing if there are no setgid restrictions for our old RGID. */
> + if (setid_policy_lookup((kid_t)old->gid, INVALID_ID, GID) == SIDPOL_DEFAULT)
> return 0;
>
> - if (uid_permitted_for_cred(old, new->uid) &&
> - uid_permitted_for_cred(old, new->euid) &&
> - uid_permitted_for_cred(old, new->suid) &&
> - uid_permitted_for_cred(old, new->fsuid))
> + if (id_permitted_for_cred(old, (kid_t)new->gid, GID) &&
> + id_permitted_for_cred(old, (kid_t)new->egid, GID) &&
> + id_permitted_for_cred(old, (kid_t)new->sgid, GID) &&
> + id_permitted_for_cred(old, (kid_t)new->fsgid, GID))
> return 0;
>
> /*
> @@ -151,6 +240,7 @@ static int safesetid_task_fix_setuid(struct cred *new,
>
> static struct security_hook_list safesetid_security_hooks[] = {
> LSM_HOOK_INIT(task_fix_setuid, safesetid_task_fix_setuid),
> + LSM_HOOK_INIT(task_fix_setgid, safesetid_task_fix_setgid),
> LSM_HOOK_INIT(capable, safesetid_security_capable)
> };
>
> diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> index db6d16e6bbc3..bde8c43a3767 100644
> --- a/security/safesetid/lsm.h
> +++ b/security/safesetid/lsm.h
> @@ -27,27 +27,47 @@ enum sid_policy_type {
> SIDPOL_ALLOWED /* target ID explicitly allowed */
> };
>
> +typedef union {
> + kuid_t uid;
> + kgid_t gid;
> +} kid_t;
> +
> +enum setid_type {
> + UID,
> + GID
> +};
> +
> /*
> - * Hash table entry to store safesetid policy signifying that 'src_uid'
> - * can setuid to 'dst_uid'.
> + * Hash table entry to store safesetid policy signifying that 'src_id'
> + * can set*id to 'dst_id'.
> */
> -struct setuid_rule {
> +struct setid_rule {
> struct hlist_node next;
> - kuid_t src_uid;
> - kuid_t dst_uid;
> + kid_t src_id;
> + kid_t dst_id;
> +
> + /* Flag to signal if rule is for UID's or GID's */
> + enum setid_type type;
> };
>
> #define SETID_HASH_BITS 8 /* 256 buckets in hash table */
>
> -struct setuid_ruleset {
> +/* Extension of INVALID_UID/INVALID_GID for kid_t type */
> +#define INVALID_ID (kid_t){.uid = INVALID_UID}
> +
> +struct setid_ruleset {
> DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
> char *policy_str;
> struct rcu_head rcu;
> +
> + //Flag to signal if ruleset is for UID's or GID's
> + enum setid_type type;
> };
>
> -enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy,
> - kuid_t src, kuid_t dst);
> +enum sid_policy_type _setid_policy_lookup(struct setid_ruleset *policy,
> + kid_t src, kid_t dst);
>
> -extern struct setuid_ruleset __rcu *safesetid_setuid_rules;
> +extern struct setid_ruleset __rcu *safesetid_setuid_rules;
> +extern struct setid_ruleset __rcu *safesetid_setgid_rules;
>
> #endif /* _SAFESETID_H */
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index f8bc574cea9c..211050d0a922 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -19,22 +19,23 @@
>
> #include "lsm.h"
>
> -static DEFINE_MUTEX(policy_update_lock);
> +static DEFINE_MUTEX(uid_policy_update_lock);
> +static DEFINE_MUTEX(gid_policy_update_lock);
>
> /*
> - * In the case the input buffer contains one or more invalid UIDs, the kuid_t
> + * In the case the input buffer contains one or more invalid IDs, the kid_t
> * variables pointed to by @parent and @child will get updated but this
> * function will return an error.
> * Contents of @buf may be modified.
> */
> static int parse_policy_line(struct file *file, char *buf,
> - struct setuid_rule *rule)
> + struct setid_rule *rule)
> {
> char *child_str;
> int ret;
> u32 parsed_parent, parsed_child;
>
> - /* Format of |buf| string should be <UID>:<UID>. */
> + /* Format of |buf| string should be <UID>:<UID> or <GID>:<GID> */
> child_str = strchr(buf, ':');
> if (child_str == NULL)
> return -EINVAL;
> @@ -49,20 +50,36 @@ static int parse_policy_line(struct file *file, char *buf,
> if (ret)
> return ret;
>
> - rule->src_uid = make_kuid(file->f_cred->user_ns, parsed_parent);
> - rule->dst_uid = make_kuid(file->f_cred->user_ns, parsed_child);
> - if (!uid_valid(rule->src_uid) || !uid_valid(rule->dst_uid))
> + if (rule->type == UID){
> + rule->src_id.uid = make_kuid(file->f_cred->user_ns, parsed_parent);
> + rule->dst_id.uid = make_kuid(file->f_cred->user_ns, parsed_child);
> + } else if (rule->type == GID){
> + rule->src_id.gid = make_kgid(file->f_cred->user_ns, parsed_parent);
> + rule->dst_id.gid = make_kgid(file->f_cred->user_ns, parsed_child);
> + } else {
> + /* Error, rule->type is an invalid type */
> return -EINVAL;
> + }
Is there any reason to have these the below if/else actions go into the above
if/else block?
> + if (rule->type == UID) {
> + if (!uid_valid(rule->src_id.uid) || !uid_valid(rule->dst_id.uid))
> + return -EINVAL;
> + } else if (rule->type == GID) {
> + if (!gid_valid(rule->src_id.gid) || !gid_valid(rule->dst_id.gid))
> + return -EINVAL;
> + } else {
> + /* Error, rule->type is an invalid type */
> + return -EINVAL;
> + }
> return 0;
> }
>
> static void __release_ruleset(struct rcu_head *rcu)
> {
> - struct setuid_ruleset *pol =
> - container_of(rcu, struct setuid_ruleset, rcu);
> + struct setid_ruleset *pol =
> + container_of(rcu, struct setid_ruleset, rcu);
> int bucket;
> - struct setuid_rule *rule;
> + struct setid_rule *rule;
> struct hlist_node *tmp;
>
> hash_for_each_safe(pol->rules, bucket, tmp, rule, next)
> @@ -71,36 +88,58 @@ static void __release_ruleset(struct rcu_head *rcu)
> kfree(pol);
> }
>
> -static void release_ruleset(struct setuid_ruleset *pol)
> -{
> +static void release_ruleset(struct setid_ruleset *pol){
> call_rcu(&pol->rcu, __release_ruleset);
> }
>
> -static void insert_rule(struct setuid_ruleset *pol, struct setuid_rule *rule)
> +static void insert_rule(struct setid_ruleset *pol, struct setid_rule *rule)
> {
> - hash_add(pol->rules, &rule->next, __kuid_val(rule->src_uid));
> + if (pol->type == UID)
> + hash_add(pol->rules, &rule->next, __kuid_val(rule->src_id.uid));
> + else if (pol->type == GID)
> + hash_add(pol->rules, &rule->next, __kgid_val(rule->src_id.gid));
> + else /* Error, pol->type is neither UID or GID */
> + return;
> }
>
> -static int verify_ruleset(struct setuid_ruleset *pol)
> +static int verify_ruleset(struct setid_ruleset *pol)
> {
> int bucket;
> - struct setuid_rule *rule, *nrule;
> + struct setid_rule *rule, *nrule;
> int res = 0;
>
> hash_for_each(pol->rules, bucket, rule, next) {
> - if (_setuid_policy_lookup(pol, rule->dst_uid, INVALID_UID) ==
> - SIDPOL_DEFAULT) {
> - pr_warn("insecure policy detected: uid %d is constrained but transitively unconstrained through uid %d\n",
> - __kuid_val(rule->src_uid),
> - __kuid_val(rule->dst_uid));
> + if (_setid_policy_lookup(pol, rule->dst_id, INVALID_ID) == SIDPOL_DEFAULT) {
> + if (pol->type == UID) {
> + pr_warn("insecure policy detected: uid %d is constrained but transitively unconstrained through uid %d\n",
> + __kuid_val(rule->src_id.uid),
> + __kuid_val(rule->dst_id.uid));
> + } else if (pol->type == GID) {
> + pr_warn("insecure policy detected: gid %d is constrained but transitively unconstrained through gid %d\n",
> + __kgid_val(rule->src_id.gid),
> + __kgid_val(rule->dst_id.gid));
> + } else { /* pol->type is an invalid type */
> + res = -EINVAL;
> + return res;
> + }
> res = -EINVAL;
>
> /* fix it up */
> - nrule = kmalloc(sizeof(struct setuid_rule), GFP_KERNEL);
> + nrule = kmalloc(sizeof(struct setid_rule), GFP_KERNEL);
> if (!nrule)
> return -ENOMEM;
> - nrule->src_uid = rule->dst_uid;
> - nrule->dst_uid = rule->dst_uid;
> + if (pol->type == UID){
> + nrule->src_id.uid = rule->dst_id.uid;
> + nrule->dst_id.uid = rule->dst_id.uid;
> + nrule->type = UID;
> + } else if (pol->type == GID){
> + nrule->src_id.gid = rule->dst_id.gid;
> + nrule->dst_id.gid = rule->dst_id.gid;
> + nrule->type = GID;
> + } else { /* Error, pol->type is neither UID or GID */
> + kfree(nrule);
Why not check this before the kmalloc?
> + return res;
> + }
> insert_rule(pol, nrule);
> }
> }
> @@ -108,16 +147,17 @@ static int verify_ruleset(struct setuid_ruleset *pol)
> }
>
> static ssize_t handle_policy_update(struct file *file,
> - const char __user *ubuf, size_t len)
> + const char __user *ubuf, size_t len, enum setid_type policy_type)
> {
> - struct setuid_ruleset *pol;
> + struct setid_ruleset *pol;
> char *buf, *p, *end;
> int err;
>
> - pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL);
> + pol = kmalloc(sizeof(struct setid_ruleset), GFP_KERNEL);
> if (!pol)
> return -ENOMEM;
> pol->policy_str = NULL;
> + pol->type = policy_type;
> hash_init(pol->rules);
>
> p = buf = memdup_user_nul(ubuf, len);
> @@ -133,7 +173,7 @@ static ssize_t handle_policy_update(struct file *file,
>
> /* policy lines, including the last one, end with \n */
> while (*p != '\0') {
> - struct setuid_rule *rule;
> + struct setid_rule *rule;
>
> end = strchr(p, '\n');
> if (end == NULL) {
> @@ -142,18 +182,18 @@ static ssize_t handle_policy_update(struct file *file,
> }
> *end = '\0';
>
> - rule = kmalloc(sizeof(struct setuid_rule), GFP_KERNEL);
> + rule = kmalloc(sizeof(struct setid_rule), GFP_KERNEL);
> if (!rule) {
> err = -ENOMEM;
> goto out_free_buf;
> }
>
> + rule->type = policy_type;
> err = parse_policy_line(file, p, rule);
> if (err)
> goto out_free_rule;
>
> - if (_setuid_policy_lookup(pol, rule->src_uid, rule->dst_uid) ==
> - SIDPOL_ALLOWED) {
> + if (_setid_policy_lookup(pol, rule->src_id, rule->dst_id) == SIDPOL_ALLOWED) {
> pr_warn("bad policy: duplicate entry\n");
> err = -EEXIST;
> goto out_free_rule;
> @@ -178,21 +218,45 @@ static ssize_t handle_policy_update(struct file *file,
> * What we really want here is an xchg() wrapper for RCU, but since that
> * doesn't currently exist, just use a spinlock for now.
> */
> - mutex_lock(&policy_update_lock);
> - pol = rcu_replace_pointer(safesetid_setuid_rules, pol,
> - lockdep_is_held(&policy_update_lock));
> - mutex_unlock(&policy_update_lock);
> + if (policy_type == UID) {
> + mutex_lock(&uid_policy_update_lock);
> + pol = rcu_replace_pointer(safesetid_setuid_rules, pol,
> + lockdep_is_held(&uid_policy_update_lock));
> + mutex_unlock(&uid_policy_update_lock);
> + } else if (policy_type == GID) {
> + mutex_lock(&gid_policy_update_lock);
> + pol = rcu_replace_pointer(safesetid_setgid_rules, pol,
> + lockdep_is_held(&gid_policy_update_lock));
> + mutex_unlock(&gid_policy_update_lock);
> + } else {
> + /* Error, policy type is neither UID or GID */
> + pr_warn("error: bad policy type");
> + }
> err = len;
>
> out_free_buf:
> kfree(buf);
> out_free_pol:
> if (pol)
> - release_ruleset(pol);
> + release_ruleset(pol);
> return err;
> }
>
> -static ssize_t safesetid_file_write(struct file *file,
> +static ssize_t safesetid_uid_file_write(struct file *file,
> + const char __user *buf,
> + size_t len,
> + loff_t *ppos)
> +{
> + if (!file_ns_capable(file, &init_user_ns, CAP_MAC_ADMIN))
> + return -EPERM;
> +
> + if (*ppos != 0)
> + return -EINVAL;
> +
> + return handle_policy_update(file, buf, len, UID);
> +}
> +
> +static ssize_t safesetid_gid_file_write(struct file *file,
> const char __user *buf,
> size_t len,
> loff_t *ppos)
> @@ -203,38 +267,60 @@ static ssize_t safesetid_file_write(struct file *file,
> if (*ppos != 0)
> return -EINVAL;
>
> - return handle_policy_update(file, buf, len);
> + return handle_policy_update(file, buf, len, GID);
> }
>
> static ssize_t safesetid_file_read(struct file *file, char __user *buf,
> - size_t len, loff_t *ppos)
> + size_t len, loff_t *ppos, struct mutex *policy_update_lock, struct setid_ruleset* ruleset)
> {
> ssize_t res = 0;
> - struct setuid_ruleset *pol;
> + struct setid_ruleset *pol;
> const char *kbuf;
>
> - mutex_lock(&policy_update_lock);
> - pol = rcu_dereference_protected(safesetid_setuid_rules,
> - lockdep_is_held(&policy_update_lock));
> + mutex_lock(policy_update_lock);
> + pol = rcu_dereference_protected(ruleset, lockdep_is_held(&policy_update_lock));
> if (pol) {
> kbuf = pol->policy_str;
> res = simple_read_from_buffer(buf, len, ppos,
> kbuf, strlen(kbuf));
> }
> - mutex_unlock(&policy_update_lock);
> + mutex_unlock(policy_update_lock);
> +
> return res;
> }
>
> -static const struct file_operations safesetid_file_fops = {
> - .read = safesetid_file_read,
> - .write = safesetid_file_write,
> +static ssize_t safesetid_uid_file_read(struct file *file, char __user *buf,
> + size_t len, loff_t *ppos)
> +{
> + return safesetid_file_read(file, buf, len, ppos,
> + &uid_policy_update_lock, safesetid_setuid_rules);
> +}
> +
> +static ssize_t safesetid_gid_file_read(struct file *file, char __user *buf,
> + size_t len, loff_t *ppos)
> +{
> + return safesetid_file_read(file, buf, len, ppos,
> + &gid_policy_update_lock, safesetid_setgid_rules);
> +}
> +
> +
> +
> +static const struct file_operations safesetid_uid_file_fops = {
> + .read = safesetid_uid_file_read,
> + .write = safesetid_uid_file_write,
> +};
> +
> +static const struct file_operations safesetid_gid_file_fops = {
> + .read = safesetid_gid_file_read,
> + .write = safesetid_gid_file_write,
> };
>
> static int __init safesetid_init_securityfs(void)
> {
> int ret;
> struct dentry *policy_dir;
> - struct dentry *policy_file;
> + struct dentry *uid_policy_file;
> + struct dentry *gid_policy_file;
>
> if (!safesetid_initialized)
> return 0;
> @@ -245,13 +331,21 @@ static int __init safesetid_init_securityfs(void)
> goto error;
> }
>
> - policy_file = securityfs_create_file("whitelist_policy", 0600,
> - policy_dir, NULL, &safesetid_file_fops);
> - if (IS_ERR(policy_file)) {
> - ret = PTR_ERR(policy_file);
> + uid_policy_file = securityfs_create_file("uid_whitelist_policy", 0600,
> + policy_dir, NULL, &safesetid_uid_file_fops);
> + if (IS_ERR(uid_policy_file)) {
> + ret = PTR_ERR(uid_policy_file);
> goto error;
> }
>
> + gid_policy_file = securityfs_create_file("gid_whitelist_policy", 0600,
> + policy_dir, NULL, &safesetid_gid_file_fops);
> + if (IS_ERR(gid_policy_file)) {
> + ret = PTR_ERR(gid_policy_file);
> + goto error;
> + }
> +
> +
> return 0;
>
> error:
> --
> 2.28.0.rc0.105.gf9edc3c819-goog
^ permalink raw reply
* Re: [PATCH v8 00/12] Introduce CAP_PERFMON to secure system performance monitoring and observability
From: Alexey Budankov @ 2020-07-21 13:06 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Ravi Bangoria, Alexei Starovoitov, Ingo Molnar,
James Morris, Namhyung Kim, Serge Hallyn, Jiri Olsa, Song Liu,
Andi Kleen, Stephane Eranian, Igor Lubashev, Thomas Gleixner,
linux-kernel, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
linux-doc@vger.kernel.org, linux-man
In-Reply-To: <20200713185152.GA18094@kernel.org>
On 13.07.2020 21:51, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jul 13, 2020 at 03:37:51PM +0300, Alexey Budankov escreveu:
>>
>> On 13.07.2020 15:17, Arnaldo Carvalho de Melo wrote:
>>> Em Mon, Jul 13, 2020 at 12:48:25PM +0300, Alexey Budankov escreveu:
>>>>
>>>> On 10.07.2020 20:09, Arnaldo Carvalho de Melo wrote:
>>>>> Em Fri, Jul 10, 2020 at 05:30:50PM +0300, Alexey Budankov escreveu:
>>>>>> On 10.07.2020 16:31, Ravi Bangoria wrote:
>>>>>>>> Currently access to perf_events, i915_perf and other performance
>>>>>>>> monitoring and observability subsystems of the kernel is open only for
>>>>>>>> a privileged process [1] with CAP_SYS_ADMIN capability enabled in the
>>>>>>>> process effective set [2].
>>>
>>>>>>>> This patch set introduces CAP_PERFMON capability designed to secure
>>>>>>>> system performance monitoring and observability operations so that
>>>>>>>> CAP_PERFMON would assist CAP_SYS_ADMIN capability in its governing role
>>>>>>>> for performance monitoring and observability subsystems of the kernel.
>>>
>>>>>>> I'm seeing an issue with CAP_PERFMON when I try to record data for a
>>>>>>> specific target. I don't know whether this is sort of a regression or
>>>>>>> an expected behavior.
>>>
>>>>>> Thanks for reporting and root causing this case. The behavior looks like
>>>>>> kind of expected since currently CAP_PERFMON takes over the related part
>>>>>> of CAP_SYS_ADMIN credentials only. Actually Perf security docs [1] say
>>>>>> that access control is also subject to CAP_SYS_PTRACE credentials.
>>>
>>>>> I think that stating that in the error message would be helpful, after
>>>>> all, who reads docs? 8-)
>>>
>>>> At least those who write it :D ...
>>>
>>> Everybody should read it, sure :-)
>>>
>>>>> I.e., this:
>>>>>
>>>>> $ ./perf stat ls
>>>>> Error:
>>>>> Access to performance monitoring and observability operations is limited.
>>>>> $
>>>>>
>>>>> Could become:
>>>>>
>>>>> $ ./perf stat ls
>>>>> Error:
>>>>> Access to performance monitoring and observability operations is limited.
>>>>> Right now only CAP_PERFMON is granted, you may need CAP_SYS_PTRACE.
>>>>> $
>>>>
>>>> It would better provide reference to perf security docs in the tool output.
>>>
>>> So add a 3rd line:
>>>
>>> $ ./perf stat ls
>>> Error:
>>> Access to performance monitoring and observability operations is limited.
>>> Right now only CAP_PERFMON is granted, you may need CAP_SYS_PTRACE.
>>> Please read the 'Perf events and tool security' document:
>>> https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html
>
>> If it had that patch below then message change would not be required.
>
> Sure, but the tool should continue to work and provide useful messages
> when running on kernels without that change. Pointing to the document is
> valid and should be done, that is an agreed point. But the tool can do
> some checks, narrow down the possible causes for the error message and
> provide something that in most cases will make the user make progress.
>
>> However this two sentences in the end of whole message would still add up:
>> "Please read the 'Perf events and tool security' document:
>> https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html"
>
> We're in violent agreement here. :-)
Here is the message draft mentioning a) CAP_SYS_PTRACE, for kernels prior
v5.8, and b) Perf security document link. The plan is to send a patch extending
perf_events with CAP_PERFMON check [1] for ptrace_may_access() and extending
the tool with this message.
"Access to performance monitoring and observability operations is limited.
Enforced MAC policy settings (SELinux) can limit access to performance
monitoring and observability operations. Inspect system audit records for
more perf_event access control information and adjusting the policy.
Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open
access to performance monitoring and observability operations for processes
without CAP_PERFMON, CAP_SYS_PTRACE or CAP_SYS_ADMIN Linux capability.
More information can be found at 'Perf events and tool security' document:
https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html
perf_event_paranoid setting is -1:
-1: Allow use of (almost) all events by all users
Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
>= 0: Disallow raw and ftrace function tracepoint access
>= 1: Disallow CPU event access
>= 2: Disallow kernel profiling
To make the adjusted perf_event_paranoid setting permanent preserve it
in /etc/sysctl.conf (e.g. kernel.perf_event_paranoid = <setting>)"
Alexei
[1] https://lore.kernel.org/lkml/20200713121746.GA7029@kernel.org/
>
>>>
>>>> Looks like extending ptrace_may_access() check for perf_events with CAP_PERFMON
>>>
>>> You mean the following?
>>
>> Exactly that.
>
> Sure, lets then wait for others to chime in and then you can go ahead
> and submit that patch.
>
> Peter?
>
> - Arnaldo
>
>>>
>>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>>> index 856d98c36f56..a2397f724c10 100644
>>> --- a/kernel/events/core.c
>>> +++ b/kernel/events/core.c
>>> @@ -11595,7 +11595,7 @@ SYSCALL_DEFINE5(perf_event_open,
>>> * perf_event_exit_task() that could imply).
>>> */
>>> err = -EACCES;
>>> - if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
>>> + if (!perfmon_capable() && !ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
>>> goto err_cred;
>>> }
>>>
>>>> makes monitoring simpler and even more secure to use since Perf tool need
>>>> not to start/stop/single-step and read/write registers and memory and so on
>>>> like a debugger or strace-like tool. What do you think?
>>>
>>> I tend to agree, Peter?
>>>
>>>> Alexei
>>>>
>>>>>
>>>>> - Arnaldo
>>
>> Alexei
>
^ permalink raw reply
* [PATCH] KEYS: remove redundant memsets
From: trix @ 2020-07-21 14:15 UTC (permalink / raw)
To: dhowells, jarkko.sakkinen, jmorris, serge, denkenz, marcel
Cc: keyrings, linux-security-module, linux-kernel, Tom Rix
From: Tom Rix <trix@redhat.com>
Reviewing use of memset in keyctrl_pkey.c
keyctl_pkey_params_get prologue code to set params up
memset(params, 0, sizeof(*params));
params->encoding = "raw";
keyctl_pkey_params_get_2 and keyctl_pkey_query have the same
prologue and they call keyctl_pkey_params_get.
So remove the prologue from the callers.
In keyctl_pkey_params_get_2, reorder the copy_from_user
of uparams to closer to it's use to ensure that
the keyctrl_pkey_params_get is called first.
Fixes: 00d60fd3b932 ("KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver #2]")
Signed-off-by: Tom Rix <trix@redhat.com>
---
security/keys/keyctl_pkey.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
index 931d8dfb4a7f..60b504681388 100644
--- a/security/keys/keyctl_pkey.c
+++ b/security/keys/keyctl_pkey.c
@@ -119,12 +119,6 @@ static int keyctl_pkey_params_get_2(const struct keyctl_pkey_params __user *_par
struct kernel_pkey_query info;
int ret;
- memset(params, 0, sizeof(*params));
- params->encoding = "raw";
-
- if (copy_from_user(&uparams, _params, sizeof(uparams)) != 0)
- return -EFAULT;
-
ret = keyctl_pkey_params_get(uparams.key_id, _info, params);
if (ret < 0)
return ret;
@@ -133,6 +127,9 @@ static int keyctl_pkey_params_get_2(const struct keyctl_pkey_params __user *_par
if (ret < 0)
return ret;
+ if (copy_from_user(&uparams, _params, sizeof(uparams)) != 0)
+ return -EFAULT;
+
switch (op) {
case KEYCTL_PKEY_ENCRYPT:
case KEYCTL_PKEY_DECRYPT:
@@ -166,8 +163,6 @@ long keyctl_pkey_query(key_serial_t id,
struct kernel_pkey_query res;
long ret;
- memset(¶ms, 0, sizeof(params));
-
ret = keyctl_pkey_params_get(id, _info, ¶ms);
if (ret < 0)
goto error;
--
2.18.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox