enforcing RLIMIT_NPROC in setuid() ?

One of my customers found an interesting way around
RLIMIT_NPROC the
other day - he was running a script from another site that
ssh'd in,
and due to bad coding in the script it began running an
infinite loop
of ssh calls, all of which stayed logged in at the same
time.

The result was that the customer was logged in hundreds of
times
on the same machine, with well over the default 160
processes set
by "ulimit -n".  He eventually reached MAXNPROC
and the system was
unable to do anything useful until we figured out what was
going on
and stopped his script from logging in.

I believe the reason this happened was because sshd starts
out as root, and once authentication is complete, demotes
itself
to run as the user logging in (presumably by using setuid()
and/or
seteuid() ).

Would it make sense to have setuid() check the process
limit,
and return an error if the user in question is over the
limit?
That way, programs that check the return value of setuid()
would
fail and prevent new processes from being created by login
loops
like the one described above.

It would probably be a good idea for setuid() to still carry
out
the UID change, just in case there's a poorly written
program
somewhere that doesn't check the return value.

	-- Ed

Ed Ravin          | "The law, in its majestic equality,
forbids the rich as
                  |  well as the poor to sleep under
bridges, to beg in the
 eravin          |  streets, and to steal bread."
 panix.com        |               --Anatole France, Le Lys
Rouge [1894]

On Thu, 10 Jan 2008, Ed Ravin wrote:

> Would it make sense to have setuid() check the process
limit,
> and return an error if the user in question is over the
limit?
> That way, programs that check the return value of
setuid() would
> fail and prevent new processes from being created by
login loops
> like the one described above.

I think so.

I have found that login, su, and cron could easily be used
to bypass these 
limits. I believe it is a security issue.

Have a look at 

http://mail-index.netbsd.org/tech-security/2006/0
6/13/0001.html

http://archive.netbsd.se/?ml=netbs
d-tech-userlevel&a=2006-06&t=2076384

http://archive.netbsd.se/?ml=netbsd
-tech-security&a=2006-06&t=2102044

> It would probably be a good idea for setuid() to still
carry out
> the UID change, just in case there's a poorly written
program
> somewhere that doesn't check the return value.

I am not sure about that.

  Jeremy C. Reed

christosastron.com (Christos Zoulas) writes:
> Yet another is to do the checking on exec, i.e. let
> root fork() and setuid(), but enforce the limit on
exec().

Clearly it is cleanest to enforce this in the kernel -- then
we don't
have to worry about every program that might conceivably run
something
as a user. One doesn't want to hand patch ssh and forget to
patch cron
or what have you.

The exec() trick would be good from this point of view. On
the other
hand, if you had a pre-existing process running as the user
and it
exec()ed (say the user typed "exec othershell" to
the shell), it would
fail because the process limit was exceeded. This may or may
not
matter in practice -- when you've hit the process limit,
something is
probably quite wrong already and you should expect ordinary
things to
start losing. kill is a builtin for most shells so it should
still
work, at least.

Do other Unixes have a different way of handling this in the
kernel?
What does solaris do, for example?

Regardless, I think that it should be done in the kernel
rather than
in the ssh code, so if nothing better comes up, I'd go with
your
exec() proposal.

Perry

On Thu, Jan 10, 2008 at 07:19:44PM +0000, Christos Zoulas
wrote:
> In article <20080110184357.GA24955panix.com>,
> Ed Ravin  <eravinpanix.com> wrote:
> >One of my customers found an interesting way around
RLIMIT_NPROC the
> >other day - he was running a script from another
site that ssh'd in,
> >and due to bad coding in the script it began
running an infinite loop
> >of ssh calls [...]

> >Would it make sense to have setuid() check the
process limit,
> >and return an error if the user in question is over
the limit?
> >That way, programs that check the return value of
setuid() would
> >fail and prevent new processes from being created
by login loops
> >like the one described above.
> >
> >It would probably be a good idea for setuid() to
still carry out
> >the UID change, just in case there's a poorly
written program
> >somewhere that doesn't check the return value.
> 
> I think that this has been discussed:
> http://ww
w.mail-archive.com/freebsd-hackersfreebsd.org/msg08638.html
> http://www.bress.net/blog/archives/34-setuid-madness.ht
ml
> 
> The problem is that if you carry the UID change, then
you effectively
> exceed the limit so if the program does not check the
return value
> you end up back to square one. If you don't change the
UID, then
> you can cause security problems.

That's what I was thinking.  It does no harm to have
setuid() return
an error if it still carries out the UID change -
properly-written
programs would error off immediately, dumb programs would do
what
they did before and carry on as the expected non-root user,
without
any new security problems (unlike the Linux example referred
to above,
where the program ran as root while it thought it had
changed to non-root)..

> Yet another is to do the checking on exec, i.e. let
> root fork() and setuid(), but enforce the limit on
exec().

This is more effective, since it will catch all of the
common cases,
like sshd and cron.  It should catch all the cases of
accidental
overflow of the process limit, as my customer did.

But a program could be taking actions after setuid() and
before exec(),
such as reading in a user-created initialization file. 
Modifying
setuid() gives us a chance to catch those too, but only a
chance
since the program could ignore the return value.

Should I open a up a PR on this?

> Should I open a up a PR on this?

FWIW: there was a security ticket about this from a year and
a half ago. I 
think it may have been closed but it was never fixed then.

  Jeremy C. Reed

On Thu, Jan 10, 2008 at 01:43:57PM -0500, Ed Ravin wrote:
> Would it make sense to have setuid() check the process
limit,
> and return an error if the user in question is over the
limit?

Yes and no. The problem is that it changes the way the Unix
security
model works. When Linux started to do that, they created a
nice number
of root exploitable issues, because processes could not drop
the setuid
and never checked for it. Isn't the check good enough if the
limit gets
inherited and the process is still counted against the real
uid?

Joerg

Joerg Sonnenberger <joergbritannica.bec.de>
writes:
> On Thu, Jan 10, 2008 at 01:43:57PM -0500, Ed Ravin
wrote:
>> Would it make sense to have setuid() check the
process limit,
>> and return an error if the user in question is over
the limit?
>
> Yes and no. The problem is that it changes the way the
Unix security
> model works. When Linux started to do that, they
created a nice number
> of root exploitable issues, because processes could not
drop the setuid
> and never checked for it. Isn't the check good enough
if the limit gets
> inherited and the process is still counted against the
real uid?

Not quite, because the user can still create far more
processes than
they have quota for by mechanisms like cron or ssh.

Making exec fail seems like a much better idea than having
setuid
fail, though you do point out that any such change probably
needs
auditing because of unexpected security issues...

Perry