Messages in this thread | | | From | (Eric W. Biederman) | Date | Tue, 26 May 2020 15:32:10 -0500 | Subject | Re: [GIT PULL] Please pull exec fix for v5.7 |
| |
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
| |