Skip to content
  • Kees Cook's avatar
    exec: Use secureexec for clearing pdeath_signal · a70423df
    Kees Cook authored
    Like dumpability, clearing pdeath_signal happens both in setup_new_exec()
    and later in commit_creds(). The test in setup_new_exec() is different
    from all other privilege comparisons, though: it is checking the new cred
    (bprm) uid vs the old cred (current) euid. This appears to be a bug,
    introduced by commit a6f76f23
    
     ("CRED: Make execve() take advantage of
    copy-on-write credentials"):
    
    -       if (bprm->e_uid != current_euid() ||
    -           bprm->e_gid != current_egid()) {
    -               set_dumpable(current->mm, suid_dumpable);
    +       if (bprm->cred->uid != current_euid() ||
    +           bprm->cred->gid != current_egid()) {
    
    It was bprm euid vs current euid (and egids), but the effective got
    dropped. Nothing in the exec flow changes bprm->cred->uid (nor gid).
    The call traces are:
    
    	prepare_bprm_creds()
    	    prepare_exec_creds()
    	        prepare_creds()
    	            memcpy(new_creds, old_creds, ...)
    	            security_prepare_creds() (unimplemented by commoncap)
    	...
    	prepare_binprm()
    	    bprm_fill_uid()
    	        resets euid/egid to current euid/egid
    	        sets euid/egid on bprm based on set*id file bits
    	    security_bprm_set_creds()
    		cap_bprm_set_creds()
    		        handle all caps-based manipulations
    
    so this test is effectively a test of current_uid() vs current_euid(),
    which is wrong, just like the prior dumpability tests were wrong.
    
    The commit log says "Clear pdeath_signal and set dumpable on
    certain circumstances that may not be covered by commit_creds()." This
    may be meaning the earlier old euid vs new euid (and egid) test that
    got changed.
    
    Luckily, as with dumpability, this is all masked by commit_creds()
    which performs old/new euid and egid tests and clears pdeath_signal.
    
    And again, like dumpability, we should include LSM secureexec logic for
    pdeath_signal clearing. For example, Smack goes out of its way to clear
    pdeath_signal when it finds a secureexec condition.
    
    Cc: David Howells <dhowells@redhat.com>
    Signed-off-by: default avatarKees Cook <keescook@chromium.org>
    Acked-by: default avatarSerge Hallyn <serge@hallyn.com>
    Reviewed-by: default avatarJames Morris <james.l.morris@oracle.com>
    a70423df