Re: CVE-2024-47191: Local root exploit in the PAM module pam_oath.so

[Solar Designer <solar () openwall com>]

On Thu, Oct 17, 2024 at 10:28:41AM +0200, Matthias Gerstner wrote:
> - setgroups() is invoked to drop supplementary group membership.

Looks good to me.

> - the usersfile is checked for additional hard-links; if the link count
>   is larger than one, then the file is rejected. This prevents possible
>   hard link attacks on the end of the unprivileged user.

There's a subtle issue here - another user's (or root's) temporary file
may be hard-linked and st_nlink may be back to 1 after the file is
unlinked by its original creator/user.  For example, tmpfile(3) unlinks
the file right away, yet the calling program is expected to proceed to
use it.  In that case, an attacker winning the race could manipulate
content of another user's temporary file, which that user's program
could then read back and use.

>   With the Linux
>   kernel sysctl protected_hardlinks set to 1 (the usual default on most
>   distributions), this attack will not work either way.

Right.

> In our case it is only a check of st_nlink. This is because we are
> dropping privileges to the owner of the file. If one would drop
> privileges to the to-be-authenticated user, then a check of st_uid would
> be in order as well.

Right.  It appears that given your decision to allow any file owner, you
cannot fully prevent hard link attacks without protected_hardlinks.

> - O_NOCTTY has been added to the open() call of the usersfile. This
>   makes this aspect explicit, although the code already checks that the
>   file is a regular file, so the situation shouldn't arise in the first
>   place.

Oh, I had thought you'd only be able to reliably post-check for regular
file, which would be too late against side-effects on open().  However,
now I realize that you first open with O_PATH, which presumably avoids
side-effects(*), then check fstat(), and only then if everything looks
good you reopen via /proc/self/fd/fd for actual usage.  That's quite a
hack, but yes, O_NOCTTY on reopen should be redundant.

(*) The man page says "Opening a file or directory with the O_PATH flag
requires no permissions on the object itself", so we'd have bigger
problems regardless of your usage if there were side-effects.

> The reason why we accept different ownership of the file is for
> increased backward compatibility. The usersfile feature in pam-oath
> allows for potentially complex scenarios regarding to the ownership of
> the file and it was not previously clearly specified which scenarios are
> supported. When only supporting the simple scenario of the usersfile
> being located directly beneath the to-be-authenticated user's home
> directory, then a lot of things become simpler, as it has been done
> in the upstream approach in a couple of aspects.

OK, this makes sense.

Alexander