From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vt@altlinux.org>
Date: Fri, 20 May 2022 01:37:33 +0300
From: Vitaly Chikunov <vt@altlinux.org>
To: ALT Linux kernel packages development <devel-kernel@lists.altlinux.org>
Message-ID: <20220519223733.dbqtt3attdsq5zhb@altlinux.org>
References: <20220518152458.2326124-1-vseleznv@altlinux.org>
 <20220518152458.2326124-2-vseleznv@altlinux.org>
 <20220519000923.wguielow262jpryr@altlinux.org>
 <YoZFAUH3WO5WkhHq@portlab>
MIME-Version: 1.0
Content-Type: text/plain; charset=koi8-r
Content-Disposition: inline
In-Reply-To: <YoZFAUH3WO5WkhHq@portlab>
Subject: Re: [d-kernel] [PATCH] AltHa: handle setcap binaries in the same
 way as setuid ones
X-BeenThere: devel-kernel@lists.altlinux.org
X-Mailman-Version: 2.1.12
Precedence: list
Reply-To: ALT Linux kernel packages development
 <devel-kernel@lists.altlinux.org>
List-Id: ALT Linux kernel packages development
 <devel-kernel.lists.altlinux.org>
List-Unsubscribe: <https://lists.altlinux.org/mailman/options/devel-kernel>,
 <mailto:devel-kernel-request@lists.altlinux.org?subject=unsubscribe>
List-Archive: <http://lists.altlinux.org/pipermail/devel-kernel>
List-Post: <mailto:devel-kernel@lists.altlinux.org>
List-Help: <mailto:devel-kernel-request@lists.altlinux.org?subject=help>
List-Subscribe: <https://lists.altlinux.org/mailman/listinfo/devel-kernel>,
 <mailto:devel-kernel-request@lists.altlinux.org?subject=subscribe>
X-List-Received-Date: Thu, 19 May 2022 22:37:34 -0000
Archived-At: <http://lore.altlinux.org/devel-kernel/20220519223733.dbqtt3attdsq5zhb@altlinux.org/>
List-Archive: <http://lore.altlinux.org/devel-kernel/>
List-Post: <mailto:devel-kernel@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,