Login not reading /etc/login.conf.db

[D'Arcy J.M. Cain]

Perhaps the behavior is correct but I can't see why. I have my
login.conf checked into my local SVN repo. I tried to symlink it
into /etc but that doesn't seem to work, probably for good reason. So
I tried compiling it from my repo with cap_mkdb outputting to /etc.

# cap_mkdb -f /etc/login.conf /...my_repo/login.conf

This creates /etc/login.conf.db but when I log in I get basic
defaults. I then did "touch /etc/login.conf" and logged in again.
This time I got the correct settings. It seems that /etc/login.conf
needs to exist before it will read /etc/login.conf.db even if it is
empty. Does this seem like correct behavior or should I open a PR?

[Christos Zoulas]

Post by D'Arcy J.M. Cain
Perhaps the behavior is correct but I can't see why. I have my
login.conf checked into my local SVN repo. I tried to symlink it
into /etc but that doesn't seem to work, probably for good reason. So
I tried compiling it from my repo with cap_mkdb outputting to /etc.
# cap_mkdb -f /etc/login.conf /...my_repo/login.conf
This creates /etc/login.conf.db but when I log in I get basic
defaults. I then did "touch /etc/login.conf" and logged in again.
This time I got the correct settings. It seems that /etc/login.conf
needs to exist before it will read /etc/login.conf.db even if it is
empty. Does this seem like correct behavior or should I open a PR?

Well, we could reconsider what secure_path(3) means. Right now it
only checks that the file referenced:
is a plain file
it is owned by root
it is not writable by group or others

We could consider instead:
- each component of the referenced path is a directory
owned by root and not writable by group and others
- only the last component of the path can be a symbolic link
and if it is, the realpath() of that is secure.

christos

[D'Arcy J.M. Cain]

On Wed, 25 Jun 2014 15:20:18 +0000 (UTC)

Post by Christos Zoulas

Post by D'Arcy J.M. Cain
# cap_mkdb -f /etc/login.conf /...my_repo/login.conf
This creates /etc/login.conf.db but when I log in I get basic
defaults. I then did "touch /etc/login.conf" and logged in again.
This time I got the correct settings. It seems that /etc/login.conf
needs to exist before it will read /etc/login.conf.db even if it is
empty. Does this seem like correct behavior or should I open a PR?

Well, we could reconsider what secure_path(3) means. Right now it
is a plain file
it is owned by root
it is not writable by group or others

That's why it rejects my symlink.

Post by Christos Zoulas
- each component of the referenced path is a directory
owned by root and not writable by group and others
- only the last component of the path can be a symbolic link
and if it is, the realpath() of that is secure.

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?

Where is this actually checked by the way? I couldn't find it.

[Christos Zoulas]

On Jun 25, 12:52pm, ***@NetBSD.org ("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

christos

[D'Arcy J.M. Cain]

On Wed, 25 Jun 2014 13:15:04 -0400

Post by Christos Zoulas
-- 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.

[Christos Zoulas]

On Jun 26, 5:38am, ***@NetBSD.org ("D'Arcy J.M. Cain") wrote:
-- Subject: Re: Login not reading /etc/login.conf.db

| On Wed, 25 Jun 2014 13:15:04 -0400
| ***@zoulas.com (Christos Zoulas) wrote:
| > On Jun 25, 12:52pm, ***@NetBSD.org ("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.

christos

[D'Arcy J.M. Cain]

On Thu, 26 Jun 2014 08:57:52 -0400

Post by Christos Zoulas
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.

Yes, I was thinking along those lines as well. I guess it doesn't
really need to duplicate code. Something like this assuming that
cgetent1 exists. Note that my version always returns the defaults if
there is an error.

Index: login_cap.c
===================================================================
RCS file: /cvsroot/src/lib/libutil/login_cap.c,v
retrieving revision 1.31
diff -u -r1.31 login_cap.c
--- login_cap.c 29 Jun 2013 04:52:55 -0000 1.31
+++ login_cap.c 26 Jun 2014 13:25:43 -0000
@@ -77,12 +77,8 @@

/* class may be NULL */

- if (secure_path(_PATH_LOGIN_CONF) == 0) {
- classfiles[0] = _PATH_LOGIN_CONF;
- classfiles[1] = NULL;
- } else {
- classfiles[0] = NULL;
- }
+ classfiles[0] = _PATH_LOGIN_CONF;
+ classfiles[1] = NULL;

if ((lc = malloc(sizeof(login_cap_t))) == NULL) {
syslog(LOG_ERR, "%s:%d malloc: %m", __FILE__, __LINE__);
@@ -102,41 +98,31 @@
}

/*
- * Not having a login.conf file is not an error condition.
+ * Not having a valid login.conf file is not an error condition.
* The individual routines deal reasonably with missing
* capabilities and use default values.
*/
- if (classfiles[0] == NULL)
- return(lc);

- if ((res = cgetent(&lc->lc_cap, classfiles, lc->lc_class)) != 0) {
+ if ((res = cgetent1(&lc->lc_cap, classfiles, lc->lc_class)) != 0) {
lc->lc_cap = 0;
switch (res) {
case 1:
- syslog(LOG_ERR, "%s: couldn't resolve 'tc'",
- lc->lc_class);
+ syslog(LOG_ERR, "%s: couldn't resolve 'tc'", lc->lc_class);
break;
case -1:
- if (strcmp(lc->lc_class, LOGIN_DEFCLASS) == 0)
- return (lc);
syslog(LOG_ERR, "%s: unknown class", lc->lc_class);
break;
case -2:
- syslog(LOG_ERR, "%s: getting class information: %m",
- lc->lc_class);
+ syslog(LOG_ERR, "%s: getting class information: %m",
lc->lc_class); break;
case -3:
- syslog(LOG_ERR, "%s: 'tc' reference loop",
- lc->lc_class);
+ syslog(LOG_ERR, "%s: 'tc' reference loop", lc->lc_class);
break;
default:
- syslog(LOG_ERR, "%s: unexpected cgetent error",
- lc->lc_class);
+ syslog(LOG_ERR, "%s: unexpected cgetent error",
lc->lc_class); break;
}
- free(lc->lc_class);
- free(lc);
- return (0);
+ syslog(LOG_ERR, "using defaults")
}
return (lc);
}

[D'Arcy J.M. Cain]

On Thu, 26 Jun 2014 09:29:21 -0400

Post by D'Arcy J.M. Cain
Yes, I was thinking along those lines as well. I guess it doesn't
really need to duplicate code. Something like this assuming that
cgetent1 exists. Note that my version always returns the defaults if
there is an error.

And after hitting send I saw my error. The return should be the same
as before on error.

[Christos Zoulas]

Post by Christos Zoulas
-- Subject: Re: Login not reading /etc/login.conf.db
| On Wed, 25 Jun 2014 13:15:04 -0400
| > -- 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