linux-kernel - Re: [GIT PULL] Please pull exec fix for v5.7

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87pnaq5tf9.fsf@x220.int.ebiederm.org>
Date:   Tue, 26 May 2020 15:32:10 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Christoph Lameter <cl@...ux.com>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Kees Cook <keescook@...omium.org>,
        Andy Lutomirski <luto@...capital.net>
Subject: Re: [GIT PULL] Please pull exec fix for v5.7

Linus Torvalds <torvalds@...ux-foundation.org> writes:

> On Tue, May 26, 2020 at 11:42 AM Eric W. Biederman
> <ebiederm@...ssion.com> wrote:
>>
>> While working on my exec cleanups I found a bug in exec that winds
>> up miscomputing the ambient credentials during exec.  Andy appears
>> as to credentials are computed for both the script and the interpreter.
>

> Can you rephrase that?

yes.

The sentence should have read: 
"Andy appears to have to been confused as to why credentials are computed
for both the script and the interpreter."

If that is not enough ask and I will rewrite and resend the pull
request.

> I tried to figure out what you were trying to say, and I can't. I
> suspect a whole line or two is missing, or you were re-writing that
> thing and stopped in the middle or something.
>
> I'm also somewhat confused by your placement of that
>
>         new->cap_ambient = old->cap_ambient;

I am restoring the work usually done by prepare_exec_creds, that
happens to get messed up when cap_bprm_set_creds is called multiple
times.

Since that happens before cap_brpm_set_creds is ever called I figured
doing it at the top of the function in case there is something subtle
is the path to safety and reliability, especially if the code will
be backported.

I don't see us touching cap_ambient anywhere except the line that does:

	/* File caps or setid cancels ambient. */
	if (has_fcap || is_setid)
		cap_clear(new->cap_ambient);

But I am human and miss things occasionally.

> which doesn't seem to make a lot of sense. It's before the code even
> checks that the old ambient is valid, which I guess doesn't really
> matter (an error is an error, and the newly set state will not be used
> in that case), but aside from that it's just in an odd place.
>
> It's not near any other code that affects the new capabilities.
> Wouldn't it have made more sense to do this where we then clear
> cap_ambient if it's a setid binary?

That was my first thought but then I got defensive.

I think setting new->cap_ambient unconditionally at the top of the
function is the most robust way to code it. (see above).

The distance for other code clearing variables is also a mirage.  The
get_file_caps function 3 lines down in it's first line clears
new->cap_permitted.

> So this pull just confuses me for a couple of reasons - I'm not saying
> it's wrong, but at a minimum I'd like to get a merge message that
> makes more sense..

I am going to dash to get my allergy injection today, and then come
back and address whatever concerns you might have.

Eric