Re: [d-kernel] [PATCH] AltHa: handle setcap binaries in the same way as setuid ones

[Vitaly Chikunov <vt@altlinux.org>]

Vladimir,

On Thu, May 19, 2022 at 04:24:17PM +0300, Vladimir D. Seleznev wrote:
> On Thu, May 19, 2022 at 03:09:23AM +0300, Vitaly Chikunov wrote:
> > >  	/* when it's not a shebang issued script interpreter */
> > >  	if (rstrscript_enabled && bprm->executable == bprm->interpreter) {
> > >  		char *path_p;
> > > @@ -267,11 +277,30 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
> > >  		up_read(&interpreters_sem);
> > >  		kfree(path_buffer);
> > >  	}
> > > -	if (unlikely(nosuid_enabled &&
> > > -		     !uid_eq(bprm->cred->uid, bprm->cred->euid))) {
> > > +	if (nosuid_enabled) {
> > >  		char *path_p;
> > >  		char *path_buffer;
> > > -		uid_t cur_uid;
> > > +		int is_setuid = 0, is_setcap = 0;
> > > +		uid_t cur_uid, cur_euid;
> > > +
> > > +		is_setuid = !uid_eq(bprm->cred->uid, bprm->cred->euid);
> > 
> > It seems we want to restrict root to suid into user too, because this
> > way of switching users is never used. Perhaps, this decision should be
> > documented in comments.
> 
> Or we can restrict only switching to superuser. What do you think would
> be a correct way?

I'm not that perfectionist.

> 
> > > +
> > > +		if (!is_setuid) {
> > > +			cur_euid = from_kuid(bprm->cred->user_ns, bprm->cred->euid);
> > > +			if (cur_euid != (uid_t) 0)
> > > +				is_setcap = has_any_caps(bprm->cred);
> > 
> > Perhaps, this should also be documented in comment why such complicated
> > logic of setting `is_setcap`. -- Because, exec by root always have
> > capabilities which does not imply setcap and you want to avoid this
> > situation and accidental drop of legitimate root capabilities.
> 
> Isn't that obvious?

It's obvious only for people who know well what is bprm->cred at the time of
this LSM hook.

> > > +		/* If no suid and no caps detected, exit. */
> > > +		if (!is_setuid && !is_setcap)
> > > +			return 0;
> > >   ...
> > > +		if (is_setuid)
> > > +			bprm->cred->euid = bprm->cred->uid;
> > > +		cap_clear(bprm->cred->cap_permitted);
> > > +		cap_clear(bprm->cred->cap_effective);
> > 
> > Any exec under root will drop privileges, is it intended? 
> 
> No, this code does not run if there is no either setuid or setcap.
> Everything is fine.

You are right.

Thanks,