Re: Login not reading /etc/login.conf.db

[christos%astron.com@localhost (Christos Zoulas)]

In article <20140626125752.630F417FDA7%rebar.astron.com@localhost>,
Christos Zoulas <christos%zoulas.com@localhost> wrote:
>On Jun 26,  5:38am, darcy%NetBSD.org@localhost ("D'Arcy J.M. Cain") wrote:
>-- Subject: Re: Login not reading /etc/login.conf.db
>
>| On Wed, 25 Jun 2014 13:15:04 -0400
>| christos%zoulas.com@localhost (Christos Zoulas) wrote:
>| > On Jun 25, 12:52pm, darcy%NetBSD.org@localhost ("D'Arcy J.M. Cain") wrote:
>| > -- Subject: Re: Login not reading /etc/login.conf.db
>| > 
>| > | Not sure if that would work for my situation.  In any case, that's
>| > not | the real question.  The problem is that the login.conf.db file
>| > is | ignored unless /etc/login.conf exists.  It can even be empty.
>| > Why | can't it simply pick up the db file?
>| > 
>| > Because it checked before then, and the db pathname if formed later.
>| > 
>| > | Where is this actually checked by the way?  I couldn't find it.
>| > 
>| > http://nxr.netbsd.org/xref/src/lib/libutil/login_cap.c#80
>| 
>| OK, I read this and see a possible security flaw.  We check security on
>| the ASCII file but if the db file exists we use it without checking.
>| It seems to me that we should be checking security on the actual file
>| that we will be using.  Not sure how to fix it.  I thought of a number
>| of possibilities but they all wind up duplicating code.
>
>What I've been thinking is to add a getcap1() call that takes a flags
>argument and if the flags == 1, does the secure_file() check on the
>databases it opens. But this is a 1/2 baked thought.

This is all stupid. The function in question is not called
secure_file() but secure_path(). Not only it would suffer from
TOCTOU if it was written properly (since it checks for the file
path like access(2)), but it is not since it does not even test
the full path, only the permissions of the file.

So I can have a file owned by root in a directory writable by me,
and I can place race games with it. My suggestion is to remove the
two uses of secure_file from libutil (getbootpath and login_cap),
make the function abort() leaving it in libutil for compatibility
and document why it is bogus. To write this function properly, one
would need to either do a lot of of work, or have help from the
kernel. Unless someone objects, I will follow the above proposal
this weekend.

In this particular case, it even works incorrectly since it does not
check for the actual file being used (the .db file)...

And there is more stupidity deeper:

    http://nxr.netbsd.org/source/xref/src/lib/libc/gen/getcap.c#326

Why does expandtc determines if we are going to look into the db
file or not? I will fix that too. This has been fixed in the heimdal
version of the file in the tree...

christos