linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] set_user: add capability check when rlimit(RLIMIT_NPROC) exceeds
@ 2021-07-28  7:26 cgel.zte
  2021-07-28 11:59 ` Christian Brauner
  0 siblings, 1 reply; 8+ messages in thread
From: cgel.zte @ 2021-07-28  7:26 UTC (permalink / raw)
  To: christian.brauner, peterz, tglx; +Cc: linux-kernel, Ran Xiaokai

From: Ran Xiaokai <ran.xiaokai@zte.com.cn>

in copy_process(): non root users but with capability CAP_SYS_RESOURCE
or CAP_SYS_ADMIN will clean PF_NPROC_EXCEEDED flag even
rlimit(RLIMIT_NPROC) exceeds. Add the same capability check logic here.

Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
---
 kernel/sys.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index ef1a78f5d71c..72c7639e3c98 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -480,7 +480,8 @@ static int set_user(struct cred *new)
 	 * failure to the execve() stage.
 	 */
 	if (is_ucounts_overlimit(new->ucounts, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC)) &&
-			new_user != INIT_USER)
+			new_user != INIT_USER &&
+			!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
 		current->flags |= PF_NPROC_EXCEEDED;
 	else
 		current->flags &= ~PF_NPROC_EXCEEDED;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] set_user: add capability check when rlimit(RLIMIT_NPROC) exceeds
  2021-07-28  7:26 [PATCH] set_user: add capability check when rlimit(RLIMIT_NPROC) exceeds cgel.zte
@ 2021-07-28 11:59 ` Christian Brauner
  2021-07-30  8:23   ` CGEL
  0 siblings, 1 reply; 8+ messages in thread
From: Christian Brauner @ 2021-07-28 11:59 UTC (permalink / raw)
  To: cgel.zte
  Cc: peterz, tglx, linux-kernel, Ran Xiaokai, James Morris,
	Linus Torvalds, Kees Cook, NeilBrown

[Ccing a few people that did the PF_NPROC_EXCEEDED changes]

On Wed, Jul 28, 2021 at 12:26:29AM -0700, cgel.zte@gmail.com wrote:
> From: Ran Xiaokai <ran.xiaokai@zte.com.cn>
> 
> in copy_process(): non root users but with capability CAP_SYS_RESOURCE
> or CAP_SYS_ADMIN will clean PF_NPROC_EXCEEDED flag even
> rlimit(RLIMIT_NPROC) exceeds. Add the same capability check logic here.
> 
> Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
> ---
>  kernel/sys.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sys.c b/kernel/sys.c
> index ef1a78f5d71c..72c7639e3c98 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -480,7 +480,8 @@ static int set_user(struct cred *new)
>  	 * failure to the execve() stage.
>  	 */
>  	if (is_ucounts_overlimit(new->ucounts, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC)) &&
> -			new_user != INIT_USER)
> +			new_user != INIT_USER &&
> +			!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
>  		current->flags |= PF_NPROC_EXCEEDED;
>  	else
>  		current->flags &= ~PF_NPROC_EXCEEDED;

Hey Cgel,
Hey Ran,

The gist seems to me that this code wants to make sure that a program
can't successfully exec if it has gone through a set*id() transition
while exceeding its RLIMIT_NPROC.

But I agree that the semantics here are a bit strange.

Iicu, a capable but non-INIT_USER caller getting PF_NPROC_EXCEEDED set
during a set*id() transition wouldn't be able to exec right away if they
still exceed their RLIMIT_NPROC at the time of exec. So their exec would
fail in fs/exec.c:

	if ((current->flags & PF_NPROC_EXCEEDED) &&
	    is_ucounts_overlimit(current_ucounts(), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
		retval = -EAGAIN;
		goto out_ret;
	}

However, if the caller were to fork() right after the set*id()
transition but before the exec while still exceeding their RLIMIT_NPROC
then they would get PF_NPROC_EXCEEDED cleared (while the child would
inherit it):

	retval = -EAGAIN;
	if (is_ucounts_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
		if (p->real_cred->user != INIT_USER &&
		    !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
			goto bad_fork_free;
	}
	current->flags &= ~PF_NPROC_EXCEEDED;

which means a subsequent exec by the capable caller would now succeed
even though they could still exceed their RLIMIT_NPROC limit.

So at first glance, it seems that set_user() should probably get the
same check as it can be circumvented today unless I misunderstand the
original motivation.

Christian

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] set_user: add capability check when rlimit(RLIMIT_NPROC) exceeds
  2021-07-28 11:59 ` Christian Brauner
@ 2021-07-30  8:23   ` CGEL
  2021-08-03 10:03     ` CGEL
  0 siblings, 1 reply; 8+ messages in thread
From: CGEL @ 2021-07-30  8:23 UTC (permalink / raw)
  To: Christian Brauner
  Cc: peterz, tglx, linux-kernel, Ran Xiaokai, James Morris,
	Linus Torvalds, Kees Cook, NeilBrown

On Wed, Jul 28, 2021 at 01:59:30PM +0200, Christian Brauner wrote:
> [Ccing a few people that did the PF_NPROC_EXCEEDED changes]
> 
> 
> Hey Cgel,
> Hey Ran,
> 
> The gist seems to me that this code wants to make sure that a program
> can't successfully exec if it has gone through a set*id() transition
> while exceeding its RLIMIT_NPROC.
> 
> But I agree that the semantics here are a bit strange.
> 
> Iicu, a capable but non-INIT_USER caller getting PF_NPROC_EXCEEDED set
> during a set*id() transition wouldn't be able to exec right away if they
> still exceed their RLIMIT_NPROC at the time of exec. So their exec would
> fail in fs/exec.c:
> 
> 	if ((current->flags & PF_NPROC_EXCEEDED) &&
> 	    is_ucounts_overlimit(current_ucounts(), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> 		retval = -EAGAIN;
> 		goto out_ret;
> 	}
> 
> However, if the caller were to fork() right after the set*id()
> transition but before the exec while still exceeding their RLIMIT_NPROC
> then they would get PF_NPROC_EXCEEDED cleared (while the child would
> inherit it):
> 
> 	retval = -EAGAIN;
> 	if (is_ucounts_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> 		if (p->real_cred->user != INIT_USER &&
> 		    !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> 			goto bad_fork_free;
> 	}
> 	current->flags &= ~PF_NPROC_EXCEEDED;
> 
> which means a subsequent exec by the capable caller would now succeed
> even though they could still exceed their RLIMIT_NPROC limit.
> 
> So at first glance, it seems that set_user() should probably get the
> same check as it can be circumvented today unless I misunderstand the
> original motivation.
> 
> Christian

Hi Christian,

I think i didn't give enough information in the commit message.
When switch to a capable but non-INIT_SUER and the RLIMIT_NPROC limit already exceeded,
and calls these funcs:
1. set_xxuid()->exec() 
             ---> fail
2. set_xxuid()->fork()->exec()
             ---> success
Kernel should have the same behavior to uer space.
Also i think non init_user CAN exceed the limit when with proper capability,
so in the patch, set_user() clear PF_NPROC_EXCEEDED flag if capable()
returns true.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] set_user: add capability check when rlimit(RLIMIT_NPROC) exceeds
  2021-07-30  8:23   ` CGEL
@ 2021-08-03 10:03     ` CGEL
  2021-08-03 14:07       ` Christian Brauner
  0 siblings, 1 reply; 8+ messages in thread
From: CGEL @ 2021-08-03 10:03 UTC (permalink / raw)
  To: Christian Brauner
  Cc: peterz, tglx, linux-kernel, Ran Xiaokai, James Morris,
	Linus Torvalds, Kees Cook, NeilBrown

On Fri, Jul 30, 2021 at 01:23:31AM -0700, CGEL wrote:
> On Wed, Jul 28, 2021 at 01:59:30PM +0200, Christian Brauner wrote:
> > [Ccing a few people that did the PF_NPROC_EXCEEDED changes]
> > 
> > 
> > Hey Cgel,
> > Hey Ran,
> > 
> > The gist seems to me that this code wants to make sure that a program
> > can't successfully exec if it has gone through a set*id() transition
> > while exceeding its RLIMIT_NPROC.
> > 
> > But I agree that the semantics here are a bit strange.
> > 
> > Iicu, a capable but non-INIT_USER caller getting PF_NPROC_EXCEEDED set
> > during a set*id() transition wouldn't be able to exec right away if they
> > still exceed their RLIMIT_NPROC at the time of exec. So their exec would
> > fail in fs/exec.c:
> > 
> > 	if ((current->flags & PF_NPROC_EXCEEDED) &&
> > 	    is_ucounts_overlimit(current_ucounts(), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> > 		retval = -EAGAIN;
> > 		goto out_ret;
> > 	}
> > 
> > However, if the caller were to fork() right after the set*id()
> > transition but before the exec while still exceeding their RLIMIT_NPROC
> > then they would get PF_NPROC_EXCEEDED cleared (while the child would
> > inherit it):
> > 
> > 	retval = -EAGAIN;
> > 	if (is_ucounts_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> > 		if (p->real_cred->user != INIT_USER &&
> > 		    !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> > 			goto bad_fork_free;
> > 	}
> > 	current->flags &= ~PF_NPROC_EXCEEDED;
> > 
> > which means a subsequent exec by the capable caller would now succeed
> > even though they could still exceed their RLIMIT_NPROC limit.
> > 
> > So at first glance, it seems that set_user() should probably get the
> > same check as it can be circumvented today unless I misunderstand the
> > original motivation.
> > 
> > Christian
> 
> Hi Christian,
> 
> I think i didn't give enough information in the commit message.
> When switch to a capable but non-INIT_SUER and the RLIMIT_NPROC limit already exceeded,
> and calls these funcs:
> 1. set_xxuid()->exec() 
>              ---> fail
> 2. set_xxuid()->fork()->exec()
>              ---> success
> Kernel should have the same behavior to uer space.
> Also i think non init_user CAN exceed the limit when with proper capability,
> so in the patch, set_user() clear PF_NPROC_EXCEEDED flag if capable()
> returns true.

Hi, Christian

Do you have any further comments on this patch?
is there anything i did not give enough infomation ?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] set_user: add capability check when rlimit(RLIMIT_NPROC) exceeds
  2021-08-03 10:03     ` CGEL
@ 2021-08-03 14:07       ` Christian Brauner
  2021-09-07 21:30         ` Solar Designer
  0 siblings, 1 reply; 8+ messages in thread
From: Christian Brauner @ 2021-08-03 14:07 UTC (permalink / raw)
  To: CGEL
  Cc: peterz, tglx, linux-kernel, Ran Xiaokai, James Morris,
	Linus Torvalds, Kees Cook, NeilBrown

On Tue, Aug 03, 2021 at 03:03:54AM -0700, CGEL wrote:
> On Fri, Jul 30, 2021 at 01:23:31AM -0700, CGEL wrote:
> > On Wed, Jul 28, 2021 at 01:59:30PM +0200, Christian Brauner wrote:
> > > [Ccing a few people that did the PF_NPROC_EXCEEDED changes]
> > > 
> > > 
> > > Hey Cgel,
> > > Hey Ran,
> > > 
> > > The gist seems to me that this code wants to make sure that a program
> > > can't successfully exec if it has gone through a set*id() transition
> > > while exceeding its RLIMIT_NPROC.
> > > 
> > > But I agree that the semantics here are a bit strange.
> > > 
> > > Iicu, a capable but non-INIT_USER caller getting PF_NPROC_EXCEEDED set
> > > during a set*id() transition wouldn't be able to exec right away if they
> > > still exceed their RLIMIT_NPROC at the time of exec. So their exec would
> > > fail in fs/exec.c:
> > > 
> > > 	if ((current->flags & PF_NPROC_EXCEEDED) &&
> > > 	    is_ucounts_overlimit(current_ucounts(), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> > > 		retval = -EAGAIN;
> > > 		goto out_ret;
> > > 	}
> > > 
> > > However, if the caller were to fork() right after the set*id()
> > > transition but before the exec while still exceeding their RLIMIT_NPROC
> > > then they would get PF_NPROC_EXCEEDED cleared (while the child would
> > > inherit it):
> > > 
> > > 	retval = -EAGAIN;
> > > 	if (is_ucounts_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> > > 		if (p->real_cred->user != INIT_USER &&
> > > 		    !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> > > 			goto bad_fork_free;
> > > 	}
> > > 	current->flags &= ~PF_NPROC_EXCEEDED;
> > > 
> > > which means a subsequent exec by the capable caller would now succeed
> > > even though they could still exceed their RLIMIT_NPROC limit.
> > > 
> > > So at first glance, it seems that set_user() should probably get the
> > > same check as it can be circumvented today unless I misunderstand the
> > > original motivation.
> > > 
> > > Christian
> > 
> > Hi Christian,
> > 
> > I think i didn't give enough information in the commit message.
> > When switch to a capable but non-INIT_SUER and the RLIMIT_NPROC limit already exceeded,
> > and calls these funcs:
> > 1. set_xxuid()->exec() 
> >              ---> fail
> > 2. set_xxuid()->fork()->exec()
> >              ---> success
> > Kernel should have the same behavior to uer space.
> > Also i think non init_user CAN exceed the limit when with proper capability,
> > so in the patch, set_user() clear PF_NPROC_EXCEEDED flag if capable()
> > returns true.
> 
> Hi, Christian
> 
> Do you have any further comments on this patch?
> is there anything i did not give enough infomation ?

Yeah, this is fine and how I understood it too. I don't see anything
obviously wrong with it and the weird detour workaround via fork() seems
inconsistent. So if I don't here anyone come up with a good reason the
current behavior makes sense I'll pick this up.

Christian

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] set_user: add capability check when rlimit(RLIMIT_NPROC) exceeds
  2021-08-03 14:07       ` Christian Brauner
@ 2021-09-07 21:30         ` Solar Designer
  2021-09-08 10:24           ` Solar Designer
  0 siblings, 1 reply; 8+ messages in thread
From: Solar Designer @ 2021-09-07 21:30 UTC (permalink / raw)
  To: Christian Brauner
  Cc: CGEL, peterz, tglx, linux-kernel, Ran Xiaokai, James Morris,
	Linus Torvalds, Kees Cook, NeilBrown

Hi all,

Brad Spengler brought this to my attention on Twitter, and Christian
Brauner agreed I should follow up.  So here goes, below the quote:

On Tue, Aug 03, 2021 at 04:07:02PM +0200, Christian Brauner wrote:
> On Tue, Aug 03, 2021 at 03:03:54AM -0700, CGEL wrote:
> > On Fri, Jul 30, 2021 at 01:23:31AM -0700, CGEL wrote:
> > > On Wed, Jul 28, 2021 at 01:59:30PM +0200, Christian Brauner wrote:
> > > > [Ccing a few people that did the PF_NPROC_EXCEEDED changes]
> > > > 
> > > > 
> > > > Hey Cgel,
> > > > Hey Ran,
> > > > 
> > > > The gist seems to me that this code wants to make sure that a program
> > > > can't successfully exec if it has gone through a set*id() transition
> > > > while exceeding its RLIMIT_NPROC.
> > > > 
> > > > But I agree that the semantics here are a bit strange.
> > > > 
> > > > Iicu, a capable but non-INIT_USER caller getting PF_NPROC_EXCEEDED set
> > > > during a set*id() transition wouldn't be able to exec right away if they
> > > > still exceed their RLIMIT_NPROC at the time of exec. So their exec would
> > > > fail in fs/exec.c:
> > > > 
> > > > 	if ((current->flags & PF_NPROC_EXCEEDED) &&
> > > > 	    is_ucounts_overlimit(current_ucounts(), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> > > > 		retval = -EAGAIN;
> > > > 		goto out_ret;
> > > > 	}
> > > > 
> > > > However, if the caller were to fork() right after the set*id()
> > > > transition but before the exec while still exceeding their RLIMIT_NPROC
> > > > then they would get PF_NPROC_EXCEEDED cleared (while the child would
> > > > inherit it):
> > > > 
> > > > 	retval = -EAGAIN;
> > > > 	if (is_ucounts_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> > > > 		if (p->real_cred->user != INIT_USER &&
> > > > 		    !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> > > > 			goto bad_fork_free;
> > > > 	}
> > > > 	current->flags &= ~PF_NPROC_EXCEEDED;
> > > > 
> > > > which means a subsequent exec by the capable caller would now succeed
> > > > even though they could still exceed their RLIMIT_NPROC limit.
> > > > 
> > > > So at first glance, it seems that set_user() should probably get the
> > > > same check as it can be circumvented today unless I misunderstand the
> > > > original motivation.
> > > > 
> > > > Christian
> > > 
> > > Hi Christian,
> > > 
> > > I think i didn't give enough information in the commit message.
> > > When switch to a capable but non-INIT_SUER and the RLIMIT_NPROC limit already exceeded,
> > > and calls these funcs:
> > > 1. set_xxuid()->exec() 
> > >              ---> fail
> > > 2. set_xxuid()->fork()->exec()
> > >              ---> success
> > > Kernel should have the same behavior to uer space.
> > > Also i think non init_user CAN exceed the limit when with proper capability,
> > > so in the patch, set_user() clear PF_NPROC_EXCEEDED flag if capable()
> > > returns true.
> > 
> > Hi, Christian
> > 
> > Do you have any further comments on this patch?
> > is there anything i did not give enough infomation ?
> 
> Yeah, this is fine and how I understood it too. I don't see anything
> obviously wrong with it and the weird detour workaround via fork() seems
> inconsistent. So if I don't here anyone come up with a good reason the
> current behavior makes sense I'll pick this up.
> 
> Christian

As I understand, the resulting commit:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2863643fb8b92291a7e97ba46e342f1163595fa8

broke RLIMIT_NPROC support for Apache httpd suexec and likely similar.

Yes, I can see how having a detour via fork() was inconsistent, but
since the privileged process can be assumed non-malicious it was no big
deal.  suexec just doesn't have fork() in there.

Historically, the resetting on fork() appears to have been due to my
suggestion here:

https://www.openwall.com/lists/kernel-hardening/2011/07/25/4

"Perhaps also reset the flag on fork() because we have an RLIMIT_NPROC
check on fork() anyway."

Looks like I didn't consider the inconsistency for capable() processes
(or maybe that exception wasn't yet in there?)

Anyway, now I suggest that 2863643fb8b92291a7e97ba46e342f1163595fa8 be
reverted, and if there's any reason to make any change (what reason?
mere consistency or any real issue?) then I suggest that the flag
resetting on fork() be made conditional.  Something like this:

	if (atomic_read(&p->real_cred->user->processes) >=
			task_rlimit(p, RLIMIT_NPROC)) {
		if (p->real_cred->user != INIT_USER &&
		    !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
			goto bad_fork_free;
-	}
-	current->flags &= ~PF_NPROC_EXCEEDED;
+	} else
+		current->flags &= ~PF_NPROC_EXCEEDED;

Alexander

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] set_user: add capability check when rlimit(RLIMIT_NPROC) exceeds
  2021-09-07 21:30         ` Solar Designer
@ 2021-09-08 10:24           ` Solar Designer
  2021-09-13 10:01             ` Christian Brauner
  0 siblings, 1 reply; 8+ messages in thread
From: Solar Designer @ 2021-09-08 10:24 UTC (permalink / raw)
  To: Christian Brauner
  Cc: CGEL, peterz, tglx, linux-kernel, Ran Xiaokai, James Morris,
	Linus Torvalds, Kees Cook, NeilBrown

Here's a further observation:

On Tue, Sep 07, 2021 at 11:30:42PM +0200, Solar Designer wrote:
> As I understand, the resulting commit:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2863643fb8b92291a7e97ba46e342f1163595fa8
> 
> broke RLIMIT_NPROC support for Apache httpd suexec and likely similar.

The commit above tries to make things consistent by duplicating the
check from copy_process() also in set_user().  However, the check isn't
actually the same because set_user(new) is called _before_
security_task_fix_setuid(new, ...), whereas in the described detour via
fork() its check would be reached already as the new user.  So those
capable() calls just look the same, but are actually very different, and
that's the problem.  My current understanding is the commit actually
increases inconsistency.

The commit message starts with:

"in copy_process(): non root users but with capability CAP_SYS_RESOURCE
or CAP_SYS_ADMIN will clean PF_NPROC_EXCEEDED flag even
rlimit(RLIMIT_NPROC) exceeds. Add the same capability check logic here."

It talks about the obscure case of "non root users but with capability".
However, the capable() calls added by the commit actually also apply to
root, such as in suexec.

> Anyway, now I suggest that 2863643fb8b92291a7e97ba46e342f1163595fa8 be
> reverted, and if there's any reason to make any change (what reason?
> mere consistency or any real issue?) then I suggest that the flag
> resetting on fork() be made conditional.  Something like this:
> 
> 	if (atomic_read(&p->real_cred->user->processes) >=
> 			task_rlimit(p, RLIMIT_NPROC)) {
> 		if (p->real_cred->user != INIT_USER &&
> 		    !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> 			goto bad_fork_free;
> -	}
> -	current->flags &= ~PF_NPROC_EXCEEDED;
> +	} else
> +		current->flags &= ~PF_NPROC_EXCEEDED;

Alternatively, we could postpone the set_user() calls until we're
running with the new user's capabilities, but that's an invasive change
that's likely to create its own issues.  So my suggestion above holds.

Alexander

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] set_user: add capability check when rlimit(RLIMIT_NPROC) exceeds
  2021-09-08 10:24           ` Solar Designer
@ 2021-09-13 10:01             ` Christian Brauner
  0 siblings, 0 replies; 8+ messages in thread
From: Christian Brauner @ 2021-09-13 10:01 UTC (permalink / raw)
  To: Solar Designer
  Cc: CGEL, peterz, tglx, linux-kernel, Ran Xiaokai, James Morris,
	Linus Torvalds, Kees Cook, NeilBrown

On Wed, Sep 08, 2021 at 12:24:00PM +0200, Solar Designer wrote:
> Here's a further observation:
> 
> On Tue, Sep 07, 2021 at 11:30:42PM +0200, Solar Designer wrote:
> > As I understand, the resulting commit:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2863643fb8b92291a7e97ba46e342f1163595fa8
> > 
> > broke RLIMIT_NPROC support for Apache httpd suexec and likely similar.
> 
> The commit above tries to make things consistent by duplicating the
> check from copy_process() also in set_user().  However, the check isn't
> actually the same because set_user(new) is called _before_
> security_task_fix_setuid(new, ...), whereas in the described detour via
> fork() its check would be reached already as the new user.  So those
> capable() calls just look the same, but are actually very different, and
> that's the problem.  My current understanding is the commit actually
> increases inconsistency.
> 
> The commit message starts with:
> 
> "in copy_process(): non root users but with capability CAP_SYS_RESOURCE
> or CAP_SYS_ADMIN will clean PF_NPROC_EXCEEDED flag even
> rlimit(RLIMIT_NPROC) exceeds. Add the same capability check logic here."
> 
> It talks about the obscure case of "non root users but with capability".
> However, the capable() calls added by the commit actually also apply to
> root, such as in suexec.
> 
> > Anyway, now I suggest that 2863643fb8b92291a7e97ba46e342f1163595fa8 be
> > reverted, and if there's any reason to make any change (what reason?
> > mere consistency or any real issue?) then I suggest that the flag
> > resetting on fork() be made conditional.  Something like this:
> > 
> > 	if (atomic_read(&p->real_cred->user->processes) >=
> > 			task_rlimit(p, RLIMIT_NPROC)) {
> > 		if (p->real_cred->user != INIT_USER &&
> > 		    !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> > 			goto bad_fork_free;
> > -	}
> > -	current->flags &= ~PF_NPROC_EXCEEDED;
> > +	} else
> > +		current->flags &= ~PF_NPROC_EXCEEDED;
> 
> Alternatively, we could postpone the set_user() calls until we're
> running with the new user's capabilities, but that's an invasive change
> that's likely to create its own issues.  So my suggestion above holds.

Thanks for taking a look at this. We can surely revert this.  Fwiw,
given how non-obvious this whole thing turned out to be a few comments
in the code would've been helpful. I'll try to send a revert by the end
of this week with your explanations added in the revert message.

Christian

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-09-13 10:01 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-28  7:26 [PATCH] set_user: add capability check when rlimit(RLIMIT_NPROC) exceeds cgel.zte
2021-07-28 11:59 ` Christian Brauner
2021-07-30  8:23   ` CGEL
2021-08-03 10:03     ` CGEL
2021-08-03 14:07       ` Christian Brauner
2021-09-07 21:30         ` Solar Designer
2021-09-08 10:24           ` Solar Designer
2021-09-13 10:01             ` Christian Brauner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).