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

[ebiederm@xmission.com (Eric W. Biederman)]

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

> On Tue, May 26, 2020 at 11:42 AM Eric W. Biederman
> <ebiederm@xmission.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