login_yubikey does not accept user.name

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

login_yubikey does not accept user.name

Björn Ketelaars
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 -0000 1.4
+++ login_yubikey.c 31 Mar 2012 12:57:30 -0000
@@ -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

Reply | Threaded
Open this post in threaded view
|

Re: login_yubikey does not accept user.name

Björn Ketelaars
2012/4/1 Theo de Raadt <[hidden email]>:
> 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 -0000 1.4
+++ login_yubikey.c 1 Apr 2012 08:37:07 -0000
@@ -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);
 }

Reply | Threaded
Open this post in threaded view
|

Re: login_yubikey does not accept user.name

Björn Ketelaars
2012/4/3 Theo de Raadt <[hidden email]>:
> 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 -0000 1.4
+++ login_yubikey.c 4 Apr 2012 13:23:01 -0000
@@ -36,6 +36,7 @@
 #include <ctype.h>
 #include <login_cap.h>
 #include <pwd.h>
+#include <regex.h>
 #include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -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

Reply | Threaded
Open this post in threaded view
|

Re: login_yubikey does not accept user.name

David Gwynne
pretty confident a regex wont fly :)

On 04/04/2012, at 11:34 PM, Bjvrn Ketelaars wrote:

> 2012/4/3 Theo de Raadt <[hidden email]>:
>> 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 -0000 1.4
> +++ login_yubikey.c 4 Apr 2012 13:23:01 -0000
> @@ -36,6 +36,7 @@
> #include <ctype.h>
> #include <login_cap.h>
> #include <pwd.h>
> +#include <regex.h>
> #include <stdarg.h>
> #include <stdio.h>
> #include <stdlib.h>
> @@ -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

Reply | Threaded
Open this post in threaded view
|

Re: login_yubikey does not accept user.name

Stuart Henderson
In reply to this post by Björn Ketelaars
On 2012/04/04 15:34, Bjvrn Ketelaars wrote:
> 2012/4/3 Theo de Raadt <[hidden email]>:
> > 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?

Reply | Threaded
Open this post in threaded view
|

Re: login_yubikey does not accept user.name

Björn Ketelaars
> 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 -0000 1.4
+++ login_yubikey.c 4 Apr 2012 15:00:10 -0000
@@ -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

Reply | Threaded
Open this post in threaded view
|

Re: login_yubikey does not accept user.name

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.

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: login_yubikey does not accept user.name

Björn Ketelaars
2012/4/4 Todd C. Miller <[hidden email]>:

> 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 -0000 1.4
+++ login_yubikey.c 4 Apr 2012 19:04:23 -0000
@@ -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) {

Reply | Threaded
Open this post in threaded view
|

Re: login_yubikey does not accept user.name

Otto Moerbeek
On Wed, Apr 04, 2012 at 09:43:35PM +0200, Bj?rn Ketelaars wrote:

> 2012/4/4 Todd C. Miller <[hidden email]>:
> > 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 -0000 1.4
> +++ login_yubikey.c 4 Apr 2012 19:04:23 -0000
> @@ -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) {

Reply | Threaded
Open this post in threaded view
|

Re: login_yubikey does not accept user.name

Björn Ketelaars
On Thu, Apr 5, 2012 at 6:51 AM, Otto Moerbeek <[hidden email]> 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,00000014%. 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 -0000 1.4
+++ login_yubikey.c 5 Apr 2012 10:53:57 -0000
@@ -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);
 }

Reply | Threaded
Open this post in threaded view
|

Re: login_yubikey does not accept user.name

Otto Moerbeek
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 <[hidden email]> 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,00000014%. 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 -0000 1.4
> +++ login_yubikey.c 5 Apr 2012 10:53:57 -0000
> @@ -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);
>  }