Re: [PATCH v3 02/15] exec: Rename bprm->cred_prepared to called_set_creds
On Tue, Jul 18, 2017 at 6:25 PM, Kees Cook wrote: > The cred_prepared bprm flag has a misleading name. It has nothing to do > with the bprm_prepare_cred hook, and actually tracks if bprm_set_creds has > been called. Rename this flag and improve its comment. > > Cc: David Howells > Cc: John Johansen > Cc: Paul Moore > Cc: Stephen Smalley > Cc: Casey Schaufler > Cc: James Morris > Signed-off-by: Kees Cook > --- > fs/binfmt_flat.c | 2 +- > fs/exec.c | 2 +- > include/linux/binfmts.h| 8 ++-- > security/apparmor/domain.c | 2 +- > security/selinux/hooks.c | 2 +- > security/smack/smack_lsm.c | 2 +- > security/tomoyo/tomoyo.c | 2 +- > 7 files changed, 12 insertions(+), 8 deletions(-) Acked-by: Paul Moore -- paul moore www.paul-moore.com
Re: [PATCH v3 02/15] exec: Rename bprm->cred_prepared to called_set_creds
On Tue, 18 Jul 2017, Kees Cook wrote: > The cred_prepared bprm flag has a misleading name. It has nothing to do > with the bprm_prepare_cred hook, and actually tracks if bprm_set_creds has > been called. Rename this flag and improve its comment. > > Cc: David Howells > Cc: John Johansen > Cc: Paul Moore > Cc: Stephen Smalley > Cc: Casey Schaufler > Cc: James Morris > Signed-off-by: Kees Cook Acked-by: James Morris -- James Morris
Re: [PATCH v3 02/15] exec: Rename bprm->cred_prepared to called_set_creds
On Tue, Jul 18, 2017 at 6:06 PM, Andy Lutomirski wrote: > On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook wrote: >> The cred_prepared bprm flag has a misleading name. It has nothing to do >> with the bprm_prepare_cred hook, and actually tracks if bprm_set_creds has >> been called. Rename this flag and improve its comment. >> >> Cc: David Howells >> Cc: John Johansen >> Cc: Paul Moore >> Cc: Stephen Smalley >> Cc: Casey Schaufler >> Cc: James Morris >> Signed-off-by: Kees Cook >> --- >> fs/binfmt_flat.c | 2 +- >> fs/exec.c | 2 +- >> include/linux/binfmts.h| 8 ++-- >> security/apparmor/domain.c | 2 +- >> security/selinux/hooks.c | 2 +- >> security/smack/smack_lsm.c | 2 +- >> security/tomoyo/tomoyo.c | 2 +- >> 7 files changed, 12 insertions(+), 8 deletions(-) >> >> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c >> index 2edcefc0a294..a722530cc468 100644 >> --- a/fs/binfmt_flat.c >> +++ b/fs/binfmt_flat.c >> @@ -885,7 +885,7 @@ static int load_flat_shared_library(int id, struct >> lib_info *libs) >> * as we're past the point of no return and are dealing with shared >> * libraries. >> */ >> - bprm.cred_prepared = 1; >> + bprm.called_set_creds = 1; > > WTF is this? It's not, strictly speaking, a bug in this patch, but > it's nonsensical. Is it fixed (presuably deleted) later? binfmt_flat looks crazy, but I haven't seen any distros that enable it. > Otherwise looks good. Thanks! -Kees -- Kees Cook Pixel Security
Re: [PATCH v3 02/15] exec: Rename bprm->cred_prepared to called_set_creds
On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook wrote: > The cred_prepared bprm flag has a misleading name. It has nothing to do > with the bprm_prepare_cred hook, and actually tracks if bprm_set_creds has > been called. Rename this flag and improve its comment. > > Cc: David Howells > Cc: John Johansen > Cc: Paul Moore > Cc: Stephen Smalley > Cc: Casey Schaufler > Cc: James Morris > Signed-off-by: Kees Cook > --- > fs/binfmt_flat.c | 2 +- > fs/exec.c | 2 +- > include/linux/binfmts.h| 8 ++-- > security/apparmor/domain.c | 2 +- > security/selinux/hooks.c | 2 +- > security/smack/smack_lsm.c | 2 +- > security/tomoyo/tomoyo.c | 2 +- > 7 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c > index 2edcefc0a294..a722530cc468 100644 > --- a/fs/binfmt_flat.c > +++ b/fs/binfmt_flat.c > @@ -885,7 +885,7 @@ static int load_flat_shared_library(int id, struct > lib_info *libs) > * as we're past the point of no return and are dealing with shared > * libraries. > */ > - bprm.cred_prepared = 1; > + bprm.called_set_creds = 1; WTF is this? It's not, strictly speaking, a bug in this patch, but it's nonsensical. Is it fixed (presuably deleted) later? Otherwise looks good.
Re: [PATCH v3 02/15] exec: Rename bprm->cred_prepared to called_set_creds
On 07/18/2017 03:25 PM, Kees Cook wrote:
> The cred_prepared bprm flag has a misleading name. It has nothing to do
> with the bprm_prepare_cred hook, and actually tracks if bprm_set_creds has
> been called. Rename this flag and improve its comment.
>
> Cc: David Howells
> Cc: John Johansen
> Cc: Paul Moore
> Cc: Stephen Smalley
> Cc: Casey Schaufler
> Cc: James Morris
> Signed-off-by: Kees Cook
looks good
Acked-by: John Johansen
> ---
> fs/binfmt_flat.c | 2 +-
> fs/exec.c | 2 +-
> include/linux/binfmts.h| 8 ++--
> security/apparmor/domain.c | 2 +-
> security/selinux/hooks.c | 2 +-
> security/smack/smack_lsm.c | 2 +-
> security/tomoyo/tomoyo.c | 2 +-
> 7 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
> index 2edcefc0a294..a722530cc468 100644
> --- a/fs/binfmt_flat.c
> +++ b/fs/binfmt_flat.c
> @@ -885,7 +885,7 @@ static int load_flat_shared_library(int id, struct
> lib_info *libs)
>* as we're past the point of no return and are dealing with shared
>* libraries.
>*/
> - bprm.cred_prepared = 1;
> + bprm.called_set_creds = 1;
>
> res = prepare_binprm(&bprm);
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 904199086490..925c85a45d97 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1547,7 +1547,7 @@ int prepare_binprm(struct linux_binprm *bprm)
> retval = security_bprm_set_creds(bprm);
> if (retval)
> return retval;
> - bprm->cred_prepared = 1;
> + bprm->called_set_creds = 1;
>
> memset(bprm->buf, 0, BINPRM_BUF_SIZE);
> return kernel_read(bprm->file, 0, bprm->buf, BINPRM_BUF_SIZE);
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index 9508b5f83c7e..36be5a67517a 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -25,8 +25,12 @@ struct linux_binprm {
> struct mm_struct *mm;
> unsigned long p; /* current top of mem */
> unsigned int
> - cred_prepared:1,/* true if creds already prepared (multiple
> - * preps happen for interpreters) */
> + /*
> + * True after the bprm_set_creds hook has been called once
> + * (multiple calls can be made via prepare_binprm() for
> + * binfmt_script/misc).
> + */
> + called_set_creds:1,
> cap_effective:1,/* true if has elevated effective capabilities,
>* false if not; except for init which inherits
>* its parent's caps anyway */
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 001e133a3c8c..878407e023e3 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -350,7 +350,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
> const char *name = NULL, *info = NULL;
> int error = 0;
>
> - if (bprm->cred_prepared)
> + if (bprm->called_set_creds)
> return 0;
>
> ctx = cred_ctx(bprm->cred);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 819fd6858b49..0f1450a06b02 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2327,7 +2327,7 @@ static int selinux_bprm_set_creds(struct linux_binprm
> *bprm)
>
> /* SELinux context only depends on initial program or script and not
>* the script interpreter */
> - if (bprm->cred_prepared)
> + if (bprm->called_set_creds)
> return 0;
>
> old_tsec = current_security();
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 658f5d8c7e76..7d4b2e221124 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -917,7 +917,7 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
> struct superblock_smack *sbsp;
> int rc;
>
> - if (bprm->cred_prepared)
> + if (bprm->called_set_creds)
> return 0;
>
> isp = inode->i_security;
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 130b4fa4f65f..d25b705360e0 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -76,7 +76,7 @@ static int tomoyo_bprm_set_creds(struct linux_binprm *bprm)
>* Do only if this function is called for the first time of an execve
>* operation.
>*/
> - if (bprm->cred_prepared)
> + if (bprm->called_set_creds)
> return 0;
> #ifndef CONFIG_SECURITY_TOMOYO_OMIT_USERSPACE_LOADER
> /*
>
[PATCH v3 02/15] exec: Rename bprm->cred_prepared to called_set_creds
The cred_prepared bprm flag has a misleading name. It has nothing to do
with the bprm_prepare_cred hook, and actually tracks if bprm_set_creds has
been called. Rename this flag and improve its comment.
Cc: David Howells
Cc: John Johansen
Cc: Paul Moore
Cc: Stephen Smalley
Cc: Casey Schaufler
Cc: James Morris
Signed-off-by: Kees Cook
---
fs/binfmt_flat.c | 2 +-
fs/exec.c | 2 +-
include/linux/binfmts.h| 8 ++--
security/apparmor/domain.c | 2 +-
security/selinux/hooks.c | 2 +-
security/smack/smack_lsm.c | 2 +-
security/tomoyo/tomoyo.c | 2 +-
7 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index 2edcefc0a294..a722530cc468 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -885,7 +885,7 @@ static int load_flat_shared_library(int id, struct lib_info
*libs)
* as we're past the point of no return and are dealing with shared
* libraries.
*/
- bprm.cred_prepared = 1;
+ bprm.called_set_creds = 1;
res = prepare_binprm(&bprm);
diff --git a/fs/exec.c b/fs/exec.c
index 904199086490..925c85a45d97 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1547,7 +1547,7 @@ int prepare_binprm(struct linux_binprm *bprm)
retval = security_bprm_set_creds(bprm);
if (retval)
return retval;
- bprm->cred_prepared = 1;
+ bprm->called_set_creds = 1;
memset(bprm->buf, 0, BINPRM_BUF_SIZE);
return kernel_read(bprm->file, 0, bprm->buf, BINPRM_BUF_SIZE);
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 9508b5f83c7e..36be5a67517a 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -25,8 +25,12 @@ struct linux_binprm {
struct mm_struct *mm;
unsigned long p; /* current top of mem */
unsigned int
- cred_prepared:1,/* true if creds already prepared (multiple
-* preps happen for interpreters) */
+ /*
+* True after the bprm_set_creds hook has been called once
+* (multiple calls can be made via prepare_binprm() for
+* binfmt_script/misc).
+*/
+ called_set_creds:1,
cap_effective:1,/* true if has elevated effective capabilities,
* false if not; except for init which inherits
* its parent's caps anyway */
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 001e133a3c8c..878407e023e3 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -350,7 +350,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
const char *name = NULL, *info = NULL;
int error = 0;
- if (bprm->cred_prepared)
+ if (bprm->called_set_creds)
return 0;
ctx = cred_ctx(bprm->cred);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 819fd6858b49..0f1450a06b02 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2327,7 +2327,7 @@ static int selinux_bprm_set_creds(struct linux_binprm
*bprm)
/* SELinux context only depends on initial program or script and not
* the script interpreter */
- if (bprm->cred_prepared)
+ if (bprm->called_set_creds)
return 0;
old_tsec = current_security();
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 658f5d8c7e76..7d4b2e221124 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -917,7 +917,7 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
struct superblock_smack *sbsp;
int rc;
- if (bprm->cred_prepared)
+ if (bprm->called_set_creds)
return 0;
isp = inode->i_security;
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 130b4fa4f65f..d25b705360e0 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -76,7 +76,7 @@ static int tomoyo_bprm_set_creds(struct linux_binprm *bprm)
* Do only if this function is called for the first time of an execve
* operation.
*/
- if (bprm->cred_prepared)
+ if (bprm->called_set_creds)
return 0;
#ifndef CONFIG_SECURITY_TOMOYO_OMIT_USERSPACE_LOADER
/*
--
2.7.4
