diff mbox

[V4,2/3] IMA: Use consistent creds

Message ID 20180103012017.7022-2-mjg59@google.com
State New, archived
Headers show

Commit Message

Matthew Garrett Jan. 3, 2018, 1:20 a.m. UTC
Right now most of the IMA code is using current->creds, but the LSM
checks are using security_task_getsecid() which ends up looking at
real_creds. Switch to using security_cred_getsecid() in order to make
this consistent.

Signed-off-by: Matthew Garrett <mjg59@google.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Eric Paris <eparis@parisplace.org>
Cc: selinux@tycho.nsa.gov
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-security-module@vger.kernel.org
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: linux-integrity@vger.kernel.org
---
 security/integrity/ima/ima_policy.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Casey Schaufler Jan. 3, 2018, 3:54 p.m. UTC | #1
On 1/2/2018 5:20 PM, Matthew Garrett wrote:
> Right now most of the IMA code is using current->creds, but the LSM
> checks are using security_task_getsecid() which ends up looking at
> real_creds. Switch to using security_cred_getsecid() in order to make
> this consistent.
>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: Eric Paris <eparis@parisplace.org>
> Cc: selinux@tycho.nsa.gov
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-security-module@vger.kernel.org
> Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> Cc: linux-integrity@vger.kernel.org
> ---
>  security/integrity/ima/ima_policy.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index ee4613fa5840..52951ac445ea 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -249,7 +249,6 @@ static void ima_lsm_update_rules(void)
>  static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>  			    enum ima_hooks func, int mask)
>  {
> -	struct task_struct *tsk = current;
>  	const struct cred *cred = current_cred();
>  	int i;
>  
> @@ -305,7 +304,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>  		case LSM_SUBJ_USER:
>  		case LSM_SUBJ_ROLE:
>  		case LSM_SUBJ_TYPE:
> -			security_task_getsecid(tsk, &sid);
> +			security_cred_getsecid(cred, &sid);
>  			rc = security_filter_rule_match(sid,

security_filter_rule_match() is security_audit_rule_match() in
sheep's clothing. Using the cred secid in this case, where the
task secid is used elsewhere is going to lead to tears. It's
going to make *me* cry as I work on untangling secids for
stacking/namespaces. I can't predict how else it's going to
bite us, but I'm betting on it.

 

>  							rule->lsm[i].type,
>  							Audit_equal,

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett Jan. 3, 2018, 6:11 p.m. UTC | #2
On Wed, Jan 3, 2018 at 7:54 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 1/2/2018 5:20 PM, Matthew Garrett wrote:
>> Right now most of the IMA code is using current->creds, but the LSM
>> checks are using security_task_getsecid() which ends up looking at
>> real_creds. Switch to using security_cred_getsecid() in order to make
>> this consistent.
> security_filter_rule_match() is security_audit_rule_match() in
> sheep's clothing. Using the cred secid in this case, where the
> task secid is used elsewhere is going to lead to tears. It's
> going to make *me* cry as I work on untangling secids for
> stacking/namespaces. I can't predict how else it's going to
> bite us, but I'm betting on it.

The problem here is that we don't *have* the task secid for one of the
cases I care about. Validating the task secid at execution time gives
us the security context of the spawning process, rather than the
spawned one - by the time it's committed to the task structure, it's
too late to block execution, so all we have is the secid associated
with the creds in the bprm structure. Obviously fixing this in a way
that doesn't break your work is important, so any suggestions on how I
should be fixing this? :)
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Casey Schaufler Jan. 3, 2018, 7:32 p.m. UTC | #3
On 1/3/2018 10:11 AM, Matthew Garrett wrote:
> On Wed, Jan 3, 2018 at 7:54 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 1/2/2018 5:20 PM, Matthew Garrett wrote:
>>> Right now most of the IMA code is using current->creds, but the LSM
>>> checks are using security_task_getsecid() which ends up looking at
>>> real_creds. Switch to using security_cred_getsecid() in order to make
>>> this consistent.
>> security_filter_rule_match() is security_audit_rule_match() in
>> sheep's clothing. Using the cred secid in this case, where the
>> task secid is used elsewhere is going to lead to tears. It's
>> going to make *me* cry as I work on untangling secids for
>> stacking/namespaces. I can't predict how else it's going to
>> bite us, but I'm betting on it.
> The problem here is that we don't *have* the task secid for one of the
> cases I care about. Validating the task secid at execution time gives
> us the security context of the spawning process, rather than the
> spawned one - by the time it's committed to the task structure, it's
> too late to block execution, so all we have is the secid associated
> with the creds in the bprm structure. Obviously fixing this in a way
> that doesn't break your work is important, so any suggestions on how I
> should be fixing this? :)

A security module is allowed to manage either or both of
task and cred blobs. How a security module uses secids is
completely up to the module. So far, everyone is using the
secid to be an alias for the secctx, and the task and cred
are treated as (roughly) the same kind of thing. But that's
not guaranteed going forward. I don't know what someone
might want to do that would cause a problem, but people are
amazingly creative.

I'm actually more concerned with the IMA code using the audit
rule matching. There's an assumption that the secid from a
cred and a secid from a task are both acceptable to the audit
system. What if they aren't? It's possible that I'm just
being paranoid, but we're getting too many permutations
(audit/IMA + task/cred) for my liking. 


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett Jan. 3, 2018, 7:44 p.m. UTC | #4
On Wed, Jan 3, 2018 at 11:32 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 1/3/2018 10:11 AM, Matthew Garrett wrote:
>> The problem here is that we don't *have* the task secid for one of the
>> cases I care about. Validating the task secid at execution time gives
>> us the security context of the spawning process, rather than the
>> spawned one - by the time it's committed to the task structure, it's
>> too late to block execution, so all we have is the secid associated
>> with the creds in the bprm structure. Obviously fixing this in a way
>> that doesn't break your work is important, so any suggestions on how I
>> should be fixing this? :)
>
> A security module is allowed to manage either or both of
> task and cred blobs. How a security module uses secids is
> completely up to the module. So far, everyone is using the
> secid to be an alias for the secctx, and the task and cred
> are treated as (roughly) the same kind of thing. But that's
> not guaranteed going forward. I don't know what someone
> might want to do that would cause a problem, but people are
> amazingly creative.
>
> I'm actually more concerned with the IMA code using the audit
> rule matching. There's an assumption that the secid from a
> cred and a secid from a task are both acceptable to the audit
> system. What if they aren't? It's possible that I'm just
> being paranoid, but we're getting too many permutations
> (audit/IMA + task/cred) for my liking.

The idea here is that we want to be able to trigger an IMA rule
conditionally based on the LSM context a process is running under at
exec time. The current code does so using the secid of current, which
means we're determining whether the new binary should be measured
based on the security context of the task that's executing it.
However, we want to be able to do so based on the security context of
the task that's being executed (usecase here is that I want to measure
anything that's executed in a privileged security context, but don't
care about anything that's running in an unprivileged context). The
child secid has been calculated and put in the creds that are present
in the bprm structure, but commit_creds() hasn't been called yet and
as a result they're not associated with the task struct. One of the
outcomes of measurement may be to block execution, and unfortunately
by the time commit_creds() has been called it's too late to do so.

If we want to be able to do something conditional on the LSM context
that a process is going to be executed under, *before* commit_creds()
is called, is there an existing way to do so? I can rework this so we
use the task secid for all running processes and the cred secid for
the not-yet-running child process, but I don't know if that's
sufficient to avoid problems in future.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Casey Schaufler Jan. 3, 2018, 8:08 p.m. UTC | #5
On 1/3/2018 11:44 AM, Matthew Garrett wrote:
> On Wed, Jan 3, 2018 at 11:32 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 1/3/2018 10:11 AM, Matthew Garrett wrote:
>>> The problem here is that we don't *have* the task secid for one of the
>>> cases I care about. Validating the task secid at execution time gives
>>> us the security context of the spawning process, rather than the
>>> spawned one - by the time it's committed to the task structure, it's
>>> too late to block execution, so all we have is the secid associated
>>> with the creds in the bprm structure. Obviously fixing this in a way
>>> that doesn't break your work is important, so any suggestions on how I
>>> should be fixing this? :)
>> A security module is allowed to manage either or both of
>> task and cred blobs. How a security module uses secids is
>> completely up to the module. So far, everyone is using the
>> secid to be an alias for the secctx, and the task and cred
>> are treated as (roughly) the same kind of thing. But that's
>> not guaranteed going forward. I don't know what someone
>> might want to do that would cause a problem, but people are
>> amazingly creative.
>>
>> I'm actually more concerned with the IMA code using the audit
>> rule matching. There's an assumption that the secid from a
>> cred and a secid from a task are both acceptable to the audit
>> system. What if they aren't? It's possible that I'm just
>> being paranoid, but we're getting too many permutations
>> (audit/IMA + task/cred) for my liking.
> The idea here is that we want to be able to trigger an IMA rule
> conditionally based on the LSM context a process is running under at
> exec time. The current code does so using the secid of current, which
> means we're determining whether the new binary should be measured
> based on the security context of the task that's executing it.
> However, we want to be able to do so based on the security context of
> the task that's being executed (usecase here is that I want to measure
> anything that's executed in a privileged security context, but don't
> care about anything that's running in an unprivileged context). The
> child secid has been calculated and put in the creds that are present
> in the bprm structure, but commit_creds() hasn't been called yet and
> as a result they're not associated with the task struct. One of the
> outcomes of measurement may be to block execution, and unfortunately
> by the time commit_creds() has been called it's too late to do so.
>
> If we want to be able to do something conditional on the LSM context
> that a process is going to be executed under, *before* commit_creds()
> is called, is there an existing way to do so? I can rework this so we
> use the task secid for all running processes and the cred secid for
> the not-yet-running child process, but I don't know if that's
> sufficient to avoid problems in future.

It's possible that converting all the existing calls of
security_task_getsecid() to security_cred_getsecid() is the
safe approach. No one is using the task blob today, and this
would disambiguate the situation.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett Jan. 4, 2018, 7:17 p.m. UTC | #6
On Wed, Jan 3, 2018 at 12:08 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 1/3/2018 11:44 AM, Matthew Garrett wrote:
>> If we want to be able to do something conditional on the LSM context
>> that a process is going to be executed under, *before* commit_creds()
>> is called, is there an existing way to do so? I can rework this so we
>> use the task secid for all running processes and the cred secid for
>> the not-yet-running child process, but I don't know if that's
>> sufficient to avoid problems in future.
>
> It's possible that converting all the existing calls of
> security_task_getsecid() to security_cred_getsecid() is the
> safe approach. No one is using the task blob today, and this
> would disambiguate the situation.

Ok. Should we be looking at creds or real_creds?
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index ee4613fa5840..52951ac445ea 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -249,7 +249,6 @@  static void ima_lsm_update_rules(void)
 static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 			    enum ima_hooks func, int mask)
 {
-	struct task_struct *tsk = current;
 	const struct cred *cred = current_cred();
 	int i;
 
@@ -305,7 +304,7 @@  static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 		case LSM_SUBJ_USER:
 		case LSM_SUBJ_ROLE:
 		case LSM_SUBJ_TYPE:
-			security_task_getsecid(tsk, &sid);
+			security_cred_getsecid(cred, &sid);
 			rc = security_filter_rule_match(sid,
 							rule->lsm[i].type,
 							Audit_equal,