Re: pam_timestamp internals

["Dmitry V. Levin" <ldv () altlinux org>]

On Mon, Mar 31, 2014 at 12:57:11PM +0200, Sebastian Krahmer wrote:
> On Mon, Mar 31, 2014 at 02:32:09PM +0400, Dmitry V. Levin wrote:
> > On Mon, Mar 24, 2014 at 01:46:43PM +0100, Sebastian Krahmer wrote:
> > > When playing with some PAM modules for my own projects, I came
> > > across some implications of pam_timestamp (which is part of
> > > upstream linux-pam) that should probably be addressed.
> > >
> > > Most importantly, there seems to be a path traversal issue:
> >
> > Thanks, Sebastian!  The issue has been fixed in upstream linux-pam by commit
> > https://git.fedorahosted.org/cgit/linux-pam.git/commit/?id=Linux-PAM-1_1_8-32-g9dcead8
>
> Thanks for taking care. I was about to write a patch on my own, but seems
> not necessary anymore.
>
> However, I think that
>
> +     if (!strlen(tty) || !strcmp(tty, ".") || !strcmp(tty, "..")) {
>
> could be insufficient.

There is a code in check_tty() that handles '/':
        if (strchr(tty, '/') != NULL) {
                ...
                tty = strrchr(tty, '/') + 1;
        }

> Any occurence of "." inside tty name should be evil.

Strange - yes, but why evil?

> Above strcmp() matches exactly "." or "..",
> but you also want "../../" etc which should pass above check.

After commit 9dcead8, check_tty() handles all such cases.

> For the ruser check, the strchr(ruser, '/') safes this, but
> ".." occurence may also be treatened appropriately.

Commit 9dcead8 also makes get_ruser() reject "." and ".." as invalid
ruser values.


-- 
ldv

Attachment: _bin
Description: