Re: login_yubikey does not accept user.name
On Thu, Apr 05, 2012 at 01:32:46PM +0200, Bj?rn Ketelaars wrote:
> On Thu, Apr 5, 2012 at 6:51 AM, Otto Moerbeek wrote:
> > That sounds like timing bases attacks to guess a username still will work.
> >
> >-Otto
>
> First thing I thought when reading your reply: absolute nonsense. With
> that mindset I really wanted to disprove your suggestion. For this I
> have set-up two different scenarios:
>
> 1.) Local 'attack' without use of getpwnam (plain-vanilla login_yubikey)
> 2.) Local 'attack' with use of getpwnam (altered version, read: with
> applied diff)
>
> Each scenario was run two times: one time targeting existing users and
> one time targeting not existing users. I measured the time it took to
> get a reply (accept / reject) after sending a password. Each test-run
> was done a couple ten thousand times. I compared data from the two
> test-runs within each scenario using a simple t-test. For scenario 1,
> I found that the probability that one can guess that a user exist or
> not is less than 0,0014%. For scenario 2 the probability is less
> than 0.0021%. Albeit the probability is small, it increased four
> orders in magnitude.
>
> In my own opinion I have failed to disprove your suggestion. I now can
> imagine that somebody who has knowledge on the subject can derive
> useful information as a result from the alteration to login_yubikey.
> So, in a nutshell: I guess you are right!
>
> >From the different diffs I've send in I learned that code needs to be
> readable, regexp is a no-no and that the use of getpwnam is probably
> not a good idea. I changed my first diff to reflect the above
> learning's.
>
> As I am out of alternative ideas: Opinions? / Alternatives? / Comments?
I'm really wondering what's the purpose of clean_string(). If a bogus
name is suppied, the fopen will fail and that's it. Unless I'm missing
something obvious.
-Otto
>
> Index: login_yubikey.c
> ===
> RCS file: /cvs/src/libexec/login_yubikey/login_yubikey.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 login_yubikey.c
> --- login_yubikey.c 1 Feb 2012 16:07:28 - 1.4
> +++ login_yubikey.c 5 Apr 2012 10:53:57 -
> @@ -165,10 +165,18 @@ main(int argc, char *argv[])
> static int
> clean_string(const char *s)
> {
> + int nlim, n = 1;
> +
> + nlim = strlen(s);
> while (*s) {
> - if (!isalnum(*s) && *s != '-' && *s != '_')
> + if (n == 1 && !isalnum(*s) && *s != '_')
> + return (0);
> + else if (n == nlim && !isalnum(*s) && *s != '_' && *s != '-')
> + return (0);
> + else if (!isalnum(*s) && *s != '_' && *s != '-' && *s != '.')
> return (0);
> ++s;
> + ++n;
> }
> return (1);
> }
Re: login_yubikey does not accept user.name
On Thu, Apr 5, 2012 at 6:51 AM, Otto Moerbeek wrote:
> That sounds like timing bases attacks to guess a username still will work.
>
>-Otto
First thing I thought when reading your reply: absolute nonsense. With
that mindset I really wanted to disprove your suggestion. For this I
have set-up two different scenarios:
1.) Local 'attack' without use of getpwnam (plain-vanilla login_yubikey)
2.) Local 'attack' with use of getpwnam (altered version, read: with
applied diff)
Each scenario was run two times: one time targeting existing users and
one time targeting not existing users. I measured the time it took to
get a reply (accept / reject) after sending a password. Each test-run
was done a couple ten thousand times. I compared data from the two
test-runs within each scenario using a simple t-test. For scenario 1,
I found that the probability that one can guess that a user exist or
not is less than 0,0014%. For scenario 2 the probability is less
than 0.0021%. Albeit the probability is small, it increased four
orders in magnitude.
In my own opinion I have failed to disprove your suggestion. I now can
imagine that somebody who has knowledge on the subject can derive
useful information as a result from the alteration to login_yubikey.
So, in a nutshell: I guess you are right!
>From the different diffs I've send in I learned that code needs to be
readable, regexp is a no-no and that the use of getpwnam is probably
not a good idea. I changed my first diff to reflect the above
learning's.
As I am out of alternative ideas: Opinions? / Alternatives? / Comments?
Index: login_yubikey.c
===
RCS file: /cvs/src/libexec/login_yubikey/login_yubikey.c,v
retrieving revision 1.4
diff -u -p -r1.4 login_yubikey.c
--- login_yubikey.c 1 Feb 2012 16:07:28 - 1.4
+++ login_yubikey.c 5 Apr 2012 10:53:57 -
@@ -165,10 +165,18 @@ main(int argc, char *argv[])
static int
clean_string(const char *s)
{
+ int nlim, n = 1;
+
+ nlim = strlen(s);
while (*s) {
- if (!isalnum(*s) && *s != '-' && *s != '_')
+ if (n == 1 && !isalnum(*s) && *s != '_')
+ return (0);
+ else if (n == nlim && !isalnum(*s) && *s != '_' && *s != '-')
+ return (0);
+ else if (!isalnum(*s) && *s != '_' && *s != '-' && *s != '.')
return (0);
++s;
+ ++n;
}
return (1);
}
Re: login_yubikey does not accept user.name
On Wed, Apr 04, 2012 at 09:43:35PM +0200, Bj?rn Ketelaars wrote:
> 2012/4/4 Todd C. Miller :
> > Why do we care if the user exists? Ideally, you want the code to
> > behave more or less the same whether the user is real or not.
> > Otherwise, a remote attacker can guess valid usernames by timing a
> > login attempt.
> >
> > For safety's sake, it makes sense to reject a username with a '/'
> > in it since the yubikey database is just a directory where each
> > user has a file. But I don't see the need to bail early just because
> > the user is not in the passwd database since yubikey_login() will
> > only succeed if the user has an entry.
>
> You are right! It does not make sense to bail out early if the user
> does not exist in the passwd database. However, I still like the idea
> of using the same database for looking up purposes. So another
> attempt: This time every type of username is accepted (comparable - in
> behaviour - to login_passwd). The username and the password are then
> passed to yubikey_login() which first checks if the username is valid,
> i.e. exists in the passwd database. If not, then the user will receive
> AUTH_FAILED).
That sounds like timing bases attacks to guess a username still will work.
-Otto
>
> OK?
>
>
> Index: login_yubikey.c
> ===
> RCS file: /cvs/src/libexec/login_yubikey/login_yubikey.c,v
> retrieving revision 1.4
> diff -u -r1.4 login_yubikey.c
> --- login_yubikey.c 1 Feb 2012 16:07:28 - 1.4
> +++ login_yubikey.c 4 Apr 2012 19:04:23 -
> @@ -54,7 +54,6 @@
>
> static const char *path = "/var/db/yubikey";
>
> -static int clean_string(const char *);
> static int yubikey_login(const char *, const char *);
>
> int
> @@ -102,10 +101,6 @@
> /* passed by sshd(8) for non-existing users */
> if (!strcmp(username, "NOUSER"))
> exit(EXIT_FAILURE);
> - if (!clean_string(username)) {
> - syslog(LOG_ERR, "clean_string username");
> - exit(EXIT_FAILURE);
> - }
>
> if (f == NULL && (f = fdopen(3, "r+")) == NULL) {
> syslog(LOG_ERR, "user %s: fdopen: %m", username);
> @@ -163,17 +158,6 @@
> }
>
> static int
> -clean_string(const char *s)
> -{
> - while (*s) {
> - if (!isalnum(*s) && *s != '-' && *s != '_')
> - return (0);
> - ++s;
> - }
> - return (1);
> -}
> -
> -static int
> yubikey_login(const char *username, const char *password)
> {
> char fn[MAXPATHLEN];
> @@ -183,6 +167,10 @@
> yubikey_token_st tok;
> u_int32_t last_ctr = 0, ctr;
>
> + if (getpwnam(username) == NULL) {
> + syslog(LOG_INFO, "user %s: invalid username", username);
> + return (AUTH_FAILED);
> + }
> if (strlen(password) > 32)
> password = password + strlen(password) - 32;
> if (strlen(password) != 32) {
Re: login_yubikey does not accept user.name
2012/4/4 Todd C. Miller :
> Why do we care if the user exists? Ideally, you want the code to
> behave more or less the same whether the user is real or not.
> Otherwise, a remote attacker can guess valid usernames by timing a
> login attempt.
>
> For safety's sake, it makes sense to reject a username with a '/'
> in it since the yubikey database is just a directory where each
> user has a file. But I don't see the need to bail early just because
> the user is not in the passwd database since yubikey_login() will
> only succeed if the user has an entry.
You are right! It does not make sense to bail out early if the user
does not exist in the passwd database. However, I still like the idea
of using the same database for looking up purposes. So another
attempt: This time every type of username is accepted (comparable - in
behaviour - to login_passwd). The username and the password are then
passed to yubikey_login() which first checks if the username is valid,
i.e. exists in the passwd database. If not, then the user will receive
AUTH_FAILED).
OK?
Index: login_yubikey.c
===
RCS file: /cvs/src/libexec/login_yubikey/login_yubikey.c,v
retrieving revision 1.4
diff -u -r1.4 login_yubikey.c
--- login_yubikey.c 1 Feb 2012 16:07:28 - 1.4
+++ login_yubikey.c 4 Apr 2012 19:04:23 -
@@ -54,7 +54,6 @@
static const char *path = "/var/db/yubikey";
-static int clean_string(const char *);
static int yubikey_login(const char *, const char *);
int
@@ -102,10 +101,6 @@
/* passed by sshd(8) for non-existing users */
if (!strcmp(username, "NOUSER"))
exit(EXIT_FAILURE);
- if (!clean_string(username)) {
- syslog(LOG_ERR, "clean_string username");
- exit(EXIT_FAILURE);
- }
if (f == NULL && (f = fdopen(3, "r+")) == NULL) {
syslog(LOG_ERR, "user %s: fdopen: %m", username);
@@ -163,17 +158,6 @@
}
static int
-clean_string(const char *s)
-{
- while (*s) {
- if (!isalnum(*s) && *s != '-' && *s != '_')
- return (0);
- ++s;
- }
- return (1);
-}
-
-static int
yubikey_login(const char *username, const char *password)
{
char fn[MAXPATHLEN];
@@ -183,6 +167,10 @@
yubikey_token_st tok;
u_int32_t last_ctr = 0, ctr;
+ if (getpwnam(username) == NULL) {
+ syslog(LOG_INFO, "user %s: invalid username", username);
+ return (AUTH_FAILED);
+ }
if (strlen(password) > 32)
password = password + strlen(password) - 32;
if (strlen(password) != 32) {
Re: login_yubikey does not accept user.name
Why do we care if the user exists? Ideally, you want the code to behave more or less the same whether the user is real or not. Otherwise, a remote attacker can guess valid usernames by timing a login attempt. For safety's sake, it makes sense to reject a username with a '/' in it since the yubikey database is just a directory where each user has a file. But I don't see the need to bail early just because the user is not in the passwd database since yubikey_login() will only succeed if the user has an entry. - todd
Re: login_yubikey does not accept user.name
> Why doesn't login_yubikey just use getpwnam() to check if the
> user exists like the other login_* mechs?
Why make it simple if there are exciting pattern matching options like
regexp or multiple if-statements ;-)
Index: login_yubikey.c
===
RCS file: /cvs/src/libexec/login_yubikey/login_yubikey.c,v
retrieving revision 1.4
diff -u -r1.4 login_yubikey.c
--- login_yubikey.c 1 Feb 2012 16:07:28 - 1.4
+++ login_yubikey.c 4 Apr 2012 15:00:10 -
@@ -54,7 +54,6 @@
static const char *path = "/var/db/yubikey";
-static int clean_string(const char *);
static int yubikey_login(const char *, const char *);
int
@@ -102,8 +101,8 @@
/* passed by sshd(8) for non-existing users */
if (!strcmp(username, "NOUSER"))
exit(EXIT_FAILURE);
- if (!clean_string(username)) {
- syslog(LOG_ERR, "clean_string username");
+ if (getpwnam(username) == NULL) {
+ syslog(LOG_ERR, "invalid user %s", username);
exit(EXIT_FAILURE);
}
@@ -160,17 +159,6 @@
}
closelog();
return (EXIT_SUCCESS);
-}
-
-static int
-clean_string(const char *s)
-{
- while (*s) {
- if (!isalnum(*s) && *s != '-' && *s != '_')
- return (0);
- ++s;
- }
- return (1);
}
static int
Re: login_yubikey does not accept user.name
On 2012/04/04 15:34, Bjvrn Ketelaars wrote: > 2012/4/3 Theo de Raadt : > > Hmm, I'd like to see that refactored somehow. > > > > Also, '-' should not be legal at the start of a login name. There > > are things that care. I think at the end it is OK, though. > > > > Crazy eh. Isn't there something else in libc that checks this? > > New diff: Why doesn't login_yubikey just use getpwnam() to check if the user exists like the other login_* mechs?
Re: login_yubikey does not accept user.name
pretty confident a regex wont fly :)
On 04/04/2012, at 11:34 PM, Bjvrn Ketelaars wrote:
> 2012/4/3 Theo de Raadt :
>> Hmm, I'd like to see that refactored somehow.
>>
>> Also, '-' should not be legal at the start of a login name. There
>> are things that care. I think at the end it is OK, though.
>>
>> Crazy eh. Isn't there something else in libc that checks this?
>
> New diff:
>
> Index: login_yubikey.c
> ===
> RCS file: /cvs/src/libexec/login_yubikey/login_yubikey.c,v
> retrieving revision 1.4
> diff -u -r1.4 login_yubikey.c
> --- login_yubikey.c 1 Feb 2012 16:07:28 - 1.4
> +++ login_yubikey.c 4 Apr 2012 13:23:01 -
> @@ -36,6 +36,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -165,12 +166,15 @@
> static int
> clean_string(const char *s)
> {
> - while (*s) {
> - if (!isalnum(*s) && *s != '-' && *s != '_')
> - return (0);
> - ++s;
> - }
> - return (1);
> + char p[] = "^[0-9a-z_]+(($|[0-9a-z_-]$)|([0-9a-z\\._-]+[0-9a-z_-]$))";
> + int ret = 0;
> + regex_t r;
> +
> + regcomp(&r,p,REG_EXTENDED);
> + if (regexec(&r,s,0,0,0) == 0)
> + ret = 1;
> + regfree(&r);
> + return (ret);
> }
>
> static int
Re: login_yubikey does not accept user.name
2012/4/3 Theo de Raadt :
> Hmm, I'd like to see that refactored somehow.
>
> Also, '-' should not be legal at the start of a login name. There
> are things that care. I think at the end it is OK, though.
>
> Crazy eh. Isn't there something else in libc that checks this?
New diff:
Index: login_yubikey.c
===
RCS file: /cvs/src/libexec/login_yubikey/login_yubikey.c,v
retrieving revision 1.4
diff -u -r1.4 login_yubikey.c
--- login_yubikey.c 1 Feb 2012 16:07:28 - 1.4
+++ login_yubikey.c 4 Apr 2012 13:23:01 -
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -165,12 +166,15 @@
static int
clean_string(const char *s)
{
- while (*s) {
- if (!isalnum(*s) && *s != '-' && *s != '_')
- return (0);
- ++s;
- }
- return (1);
+ char p[] = "^[0-9a-z_]+(($|[0-9a-z_-]$)|([0-9a-z\\._-]+[0-9a-z_-]$))";
+ int ret = 0;
+ regex_t r;
+
+ regcomp(&r,p,REG_EXTENDED);
+ if (regexec(&r,s,0,0,0) == 0)
+ ret = 1;
+ regfree(&r);
+ return (ret);
}
static int
Re: login_yubikey does not accept user.name
2012/4/1 Theo de Raadt :
> You should really re-do that so that the login name cannot start or
> end with a '.'
Index: login_yubikey.c
===
RCS file: /cvs/src/libexec/login_yubikey/login_yubikey.c,v
retrieving revision 1.4
diff -u -r1.4 login_yubikey.c
--- login_yubikey.c 1 Feb 2012 16:07:28 - 1.4
+++ login_yubikey.c 1 Apr 2012 08:37:07 -
@@ -165,10 +165,15 @@
static int
clean_string(const char *s)
{
+ int nlim, n = 0;
+
+ nlim = strlen(s) - 1;
while (*s) {
- if (!isalnum(*s) && *s != '-' && *s != '_')
+ if (!isalnum(*s) && *s != '-' && *s != '_' &&
+ !(*s == '.' && !(n == 0 || n == nlim)))
return (0);
++s;
+ n++;
}
return (1);
}
login_yubikey does not accept user.name
login_yubikey does not accept user names with a dot (e.g. user.name).
If one is offered login fails. As other authentication types (e.g.
login_passwd) do accept the use of a dot, it seems that this is a
'feature' related to login_yubikey.
A small patch:
ok?
Index: login_yubikey.c
===
RCS file: /cvs/src/libexec/login_yubikey/login_yubikey.c,v
retrieving revision 1.4
diff -u -r1.4 login_yubikey.c
--- login_yubikey.c 1 Feb 2012 16:07:28 - 1.4
+++ login_yubikey.c 31 Mar 2012 12:57:30 -
@@ -166,7 +166,7 @@
clean_string(const char *s)
{
while (*s) {
- if (!isalnum(*s) && *s != '-' && *s != '_')
+ if (!isalnum(*s) && *s != '-' && *s != '_' && *s != '.')
return (0);
++s;
}
--
Bjvrn Ketelaars
