Patch: Fix user authentication + MKDB

Patch: Fix user authentication + MKDB

[Pankaj K Garg]

[-- Attachment #1: Type: text/plain, Size: 1184 bytes --]

Please take a look at the following patch that fixes the
following:

 1) MKDB: creates all parent directories in case they did
   not exist.

 2) MKDB: creates gnatsd.user_access instead of gnatsd.access

 3) PASSWORD CHECKING: The password checking in the current CVS
   directory is broken. It was not working as someone else also
   recenlty noted on this list. The problems were: (a) it was using
   the opposite logic of match(), (b) it did not default to plain
   text passwords, (c) an empty database list was confusing it, and
   (d) there was no fall-through. The last point needs a bit of
   explanation:

   Suppose I had a entry like so:
	foo:test:edit:
	*:*:view:

   The desired behavior should be that in case a user fails the password
   check for 'foo' then he should be allowed to have a 'view' access
   as everybody else. The current code will default 'foo' with a bad
   password to 'no_access'.

This should close BUG report number: 416

Pankaj

P.S. Patch file created by `cvs -up gnatsd.c misc.c mkdb.sh > patchfile.out`

---
Pankaj K Garg              gargp@acm.org
1684 Nightingale Avenue    408-373-4027
Sunnyvale, CA 94304
http://home.earthlink.net/~gargp

[-- Attachment #2: patchfile.out --]
[-- Type: text/plain, Size: 3043 bytes --]

Index: gnatsd.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/gnatsd.c,v
retrieving revision 1.47
diff -u -p -r1.47 gnatsd.c
--- gnatsd.c	4 Aug 2002 10:58:29 -0000	1.47
+++ gnatsd.c	24 Sep 2002 00:04:50 -0000
@@ -256,9 +256,9 @@ password_match (const char *password, co
   if (! strncmp (hash, "$0$", 3))
     {
       /* explicit plain-text password */
-      return ! match (password, hash, TRUE);
+      return match (password, hash+3, TRUE);
     }
-  else
+  else if (! strncmp (hash, "$1", 3))
     {
       /* DES crypt or MD5 hash of the password */
 #ifdef HAVE_LIBCRYPT
@@ -269,6 +269,9 @@ password_match (const char *password, co
       return FALSE;
 #endif
     }
+  else {
+    return match (password, hash, TRUE);
+  }
 }
 
 /*  */
@@ -451,7 +454,6 @@ findUserAccessLevel (const char *file, c
 		{
 		  /* Username matched but password didn't.  */
 		  *access = ACCESS_NONE;
-		  found = 1;
 		}
 	      else
 		{
@@ -460,7 +462,10 @@ findUserAccessLevel (const char *file, c
 		      /* Compare all given names against the name of the
 			 requested database. */
 		      const char *l2 = ent->admFields[3];
-
+		      
+		      if (l2 == NULL)
+			found = 1;
+		      
 		      while (l2 != NULL && ! found)
 			{
 			  char *token = get_next_field (&l2, ',');
Index: misc.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/misc.c,v
retrieving revision 1.36
diff -u -p -r1.36 misc.c
--- misc.c	6 Jan 2002 16:13:20 -0000	1.36
+++ misc.c	24 Sep 2002 00:04:50 -0000
@@ -287,7 +287,10 @@ get_next_field (const char **line_ptr, i
       *line_ptr = NULL;
     }
 
-  return res;
+  if (end_line == line)
+    return NULL ;
+  else
+    return res;
 }
 
 /* Adds quote-marks (") around the string, and escapes any quotes that
Index: mkdb.sh
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/mkdb.sh,v
retrieving revision 1.9
diff -u -p -r1.9 mkdb.sh
--- mkdb.sh	4 Aug 2002 10:57:17 -0000	1.9
+++ mkdb.sh	24 Sep 2002 00:04:50 -0000
@@ -28,7 +28,7 @@ DATADIR=xSYSCONFDIRx/gnats/defaults
 LIBEXECDIR=xLIBEXECDIRx
 
 domkdir() {
-    mkdir "$1" || { echo "Can't create directory $1, exiting"; exit 1 ; }
+    mkdir -p "$1" || { echo "Can't create directory $1, exiting"; exit 1 ; }
     chown "${GNATS_USER}" "$1"
 }
 
@@ -82,8 +82,8 @@ echo "Copying default files from ${DATAD
 docp categories "${dbdir}/gnats-adm/categories"
 docp submitters "${dbdir}/gnats-adm/submitters"
 docp responsible "${dbdir}/gnats-adm/responsible"
-docp gnatsd.access "${dbdir}/gnats-adm/gnatsd.access"
-chmod 600 "${dbdir}/gnats-adm/gnatsd.access"
+docp gnatsd.access "${dbdir}/gnats-adm/gnatsd.user_access"
+chmod 600 "${dbdir}/gnats-adm/gnatsd.user_access"
 docp addresses "${dbdir}/gnats-adm/addresses"
 docp states "${dbdir}/gnats-adm/states"
 docp classes "${dbdir}/gnats-adm/classes"

Re: Patch: Fix user authentication + MKDB

[Lars Henriksen]

On Mon, Sep 23, 2002 at 05:49:21PM -0700, Pankaj K Garg wrote:
> Please take a look at the following patch that fixes the
> following:
> 
>  1) MKDB: creates all parent directories in case they did
>    not exist.

I'm not sure I agree. Let mkdb do what it's supposed to do: create
the database, and let the gnats administrator do what he is supposed
to do: establish the prerequisites for invoking mkdb.

>  2) MKDB: creates gnatsd.user_access instead of gnatsd.access

Agreed.

>  3) PASSWORD CHECKING: The password checking in the current CVS
>    directory is broken. It was not working as someone else also
>    recenlty noted on this list. The problems were: (a) it was using
>    the opposite logic of match(), (b) it did not default to plain
>    text passwords, (c) an empty database list was confusing it, and
>    (d) there was no fall-through.

I agree with (a) and (c), but not with (b); (d) should be considered.

As for (b), the password checking behaves as described in version 4 of the
gnats manual (Keeping Track), see section C.4. Yngve Svendsen put a lot
of work into this and I believe it behaves as intended. There is no
"default". You get the kind of password checking you ask for:

   plain text for passwords with a $0$ prefix,
   MD5 format for passwords with a $1$ prefix, and
   DES format for passwords without a prefix.

Your example below will be interpreted as having traditional Unix
DES-crypted passwords and will effectively be no-login entries.

>                                   The last point needs a bit of
>    explanation:
> 
>    Suppose I had a entry like so:
> 	foo:test:edit:
> 	*:*:view:

Now for (d); I'll assume that gnats is kept as is with no "default"
password checking. For the discussion I'll change your example to read

  	foo:$0$test:edit:
  	*:$0$*:view:

>    The desired behavior should be that in case a user fails the password
>    check for 'foo' then he should be allowed to have a 'view' access
>    as everybody else. The current code will default 'foo' with a bad
>    password to 'no_access'.

This is new behavior, but worth consideration. As it is, the user access
file is processed like the Unix password file: the first line with a
matching user is used. But with an all-user entry (or several-user entries),
it makes a lot of sense to continue processing. If adopted the behavior
should of course be documented in the manual.

What do others think?

Lars Henriksen


_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://mail.gnu.org/mailman/listinfo/help-gnats

Re: Patch: Fix user authentication + MKDB

[Dirk Schenkewitz]

Lars Henriksen wrote:
> 
> On Mon, Sep 23, 2002 at 05:49:21PM -0700, Pankaj K Garg wrote:
> ...
> >  3) PASSWORD CHECKING: The password checking in the current CVS
> >    directory is broken. It was not working as someone else also
> >    recenlty noted on this list. The problems were: (a) it was using
> >    the opposite logic of match(), (b) it did not default to plain
> >    text passwords, (c) an empty database list was confusing it, and
> >    (d) there was no fall-through.
> 
> I agree with (a) and (c), but not with (b); (d) should be considered.
> 
> As for (b), the password checking behaves as described in version 4 of the
> gnats manual (Keeping Track), see section C.4. Yngve Svendsen put a lot
> of work into this and I believe it behaves as intended. There is no
> "default". You get the kind of password checking you ask for:
> 
>    plain text for passwords with a $0$ prefix,
>    MD5 format for passwords with a $1$ prefix, and
>    DES format for passwords without a prefix.

IMHO this is better than default=plaintext-passwords.

> ...
> Now for (d); I'll assume that gnats is kept as is with no "default"
> password checking. For the discussion I'll change your example to read
> 
>         foo:$0$test:edit:
>         *:$0$*:view:
> 
> >    The desired behavior should be that in case a user fails the password
> >    check for 'foo' then he should be allowed to have a 'view' access
> >    as everybody else. The current code will default 'foo' with a bad
> >    password to 'no_access'.
> 
> This is new behavior, but worth consideration. As it is, the user access
> file is processed like the Unix password file: the first line with a
> matching user is used. But with an all-user entry (or several-user entries),
> it makes a lot of sense to continue processing. If adopted the behavior
> should of course be documented in the manual.

I believe, a fall-through/continued-processing would be a good thing.

If you don't want anybody to read your database, you can have a password
even for reading. But what if a user mis-types his password? If there is
another possibility that fits him (without a proper password), he might
be silently put in "anybody else"-status with read-only-access. That can
be unwanted. Then again, if for "everybody" or "a group of users" read-
only-access is desired, why not give them empty passwords ? As in:

         foo:$0$test:edit:
         *::view:

Then the behavior IMHO should be:
 - user foo gives correct password  --> 'edit' access
 - user foo gives wrong password    --> no access
 - user foo gives no/empty password --> 'view' access
 - user bar gives any password      --> no access
 - user bar gives no/empty password --> 'view' access

Would that be possible ?

> What do others think?

:-) well, I'll repeat that :-)

greetings
	dirk
-- 
Dirk Schenkewitz 

InterFace AG                 fon: +49 (0)89 / 610 49 - 126
Leipziger Str. 16            fax: +49 (0)89 / 610 49 - 83
D-82008 Unterhaching         
http://www.interface-ag.de   mailto:dirk.schenkewitz@interface-ag.de


_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://mail.gnu.org/mailman/listinfo/help-gnats

RE: Patch: Fix user authentication + MKDB

[Pankaj K Garg]

> > ...
> >    plain text for passwords with a $0$ prefix,
> >    MD5 format for passwords with a $1$ prefix, and
> >    DES format for passwords without a prefix.
> 
> IMHO this is better than default=plaintext-passwords.

Yes. I'll fix this in the patch tomorrow.

> > ...
> ... why not give them empty passwords ? As in:
> 
>          foo:$0$test:edit:
>          *::view:
> 
> Then the behavior IMHO should be:
>  - user foo gives correct password  --> 'edit' access
>  - user foo gives wrong password    --> no access
>  - user foo gives no/empty password --> 'view' access
>  - user bar gives any password      --> no access
>  - user bar gives no/empty password --> 'view' access
> 
> Would that be possible ?

 I see your point: If the user mis-types his password, and we
silently give them read access, they don't know what happened.
I'm in favor of your solution--should be possible to implement.

 Regards

Pankaj

 



_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://mail.gnu.org/mailman/listinfo/help-gnats

RE: Patch: Fix user authentication + MKDB

[Pankaj K Garg]

> >  1) MKDB: creates all parent directories in case they did
> >    not exist.
> I'm not sure I agree. Let mkdb do what it's supposed to do: create
> the database, and let the gnats administrator do what he is supposed
> to do: establish the prerequisites for invoking mkdb.

I used this to fix the problem of the 'com' directory not being there
for the creation of the default database. Another fix would be to create
the <prefix>/com directory as part of the 'make install'...I can do that
if people prefer that. This seemed like a more general fix, as why do
things in two steps when you can do it in one?

> >  3) PASSWORD CHECKING: The password checking in the current CVS
> >    directory is broken. It was not working as someone else also
> >    recenlty noted on this list. The problems were: (a) it was using
> >    the opposite logic of match(), (b) it did not default to plain
> >    text passwords, (c) an empty database list was confusing it, and
> >    (d) there was no fall-through.
> 
> I agree with (a) and (c), but not with (b); (d) should be considered.
> 
> As for (b), the password checking behaves as described in 
> version 4 of the
> gnats manual (Keeping Track), see section C.4. Yngve Svendsen 
> put a lot
> of work into this and I believe it behaves as intended. There is no
> "default". You get the kind of password checking you ask for:
> 
>    plain text for passwords with a $0$ prefix,
>    MD5 format for passwords with a $1$ prefix, and
>    DES format for passwords without a prefix.
> 
> Your example below will be interpreted as having traditional Unix
> DES-crypted passwords and will effectively be no-login entries.

OK, sorry I did not take a look at the manual:-) I was tripped off
by the default line '#*:*:view' in the 'gnatsd.user_access' file and
thought that the default behavior was for plain-text passwords. So,this
requires fixing in my patch. I'll redo it such that it defaults to DES
instead of plain-text.

Regards

Pankaj



_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://mail.gnu.org/mailman/listinfo/help-gnats

Re: Patch: Fix user authentication + MKDB

[Lars Henriksen]

On Wed, Sep 25, 2002 at 08:56:07AM -0700, Pankaj K Garg wrote:
> > >  1) MKDB: creates all parent directories in case they did
> > >    not exist.
> 
> I used this to fix the problem of the 'com' directory not being there
> for the creation of the default database. Another fix would be to create
> the <prefix>/com directory as part of the 'make install'...I can do that
> if people prefer that. This seemed like a more general fix, as why do
> things in two steps when you can do it in one?

Do you perform gnats administration as root or as the gnats user? I prefer
doing it as the gnats user, but I believe others do it as root. And it's
just that I don't like the thought of invoking "mkdir -p" as root. Anyway,
this isn't very important and your patch has already been committed.

> OK, sorry I did not take a look at the manual:-) I was tripped off
> by the default line '#*:*:view' in the 'gnatsd.user_access' file and
> thought that the default behavior was for plain-text passwords.

Yes, that IS misleading and should be changed to '#*:$0$*:view'. It would
be nice if you included a patch for that as well ;-)

Lars Henriksen


_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://mail.gnu.org/mailman/listinfo/help-gnats

RE: Patch: Fix user authentication + MKDB

[Pankaj K Garg]

[-- Attachment #1: Type: text/plain, Size: 1486 bytes --]

> ...
>          foo:$0$test:edit:
>          *::view:
> 
> Then the behavior IMHO should be:
>  - user foo gives correct password  --> 'edit' access
>  - user foo gives wrong password    --> no access
>  - user foo gives no/empty password --> 'view' access
>  - user bar gives any password      --> no access
>  - user bar gives no/empty password --> 'view' access
> 
> Would that be possible ?
> ...

I'm attaching a patch "gnatspatch.out" with this message that
does this. Seems to work on my small tries here, but would
appreciate if someone else can take a look at it also.

The empty password bit required a change (backwards compatible)
to the protocol. Previously, the client was required to
give:

 USER <name> <password>
 CHDB <db> <name> <password>

Now, clients can give:

 USER <name> [<password>]
 CHDB <db> <name> [<password>]

Hence, the password is optional and will be assumed to be
empty.

For clients to take advantage of this, however, they may
need some modification. For example, gnatsweb assumes that
the user always requires some value for the password.
So, I'm also attaching a patch for gnatsweb ("gnatswebpatch.out")
that takes care of empty password situation.

If the gnats gurus will accept the gnats patch, then I can
submit the gnatsweb patch to the appropriate mailing list.

BTW, the documentation in gnatsd.access states that $1$ implies
use of MD5. In the code I did not find the use of MD5 hashes...
am I missing something here?

Regards

Pankaj



[-- Attachment #2: gnatspatch.out --]
[-- Type: text/plain, Size: 6628 bytes --]

Index: cmds.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/cmds.c,v
retrieving revision 1.69
diff -u -p -r1.69 cmds.c
--- cmds.c	12 Aug 2002 12:33:30 -0000	1.69
+++ cmds.c	26 Sep 2002 22:19:02 -0000
@@ -318,11 +318,11 @@ GNATS_user (int ac, char **av)
       printf ("%d %s\r\n", CODE_INFORMATION,
 	      access_level_str (user_access));
     }
-  else if (ac == 2)
+  else if ((ac == 1) || (ac == 2))
     {
       if (databaseValid (currentDatabase))
 	{
-	  if (gnatsdChdb (databaseName (currentDatabase), av[0], av[1], 0,
+	  if (gnatsdChdb (databaseName (currentDatabase), av[0], ac == 2 ? av[1] : "", 0,
 			  &err) != 0)
 	    {
 	      print_server_errors (err);
@@ -339,14 +339,21 @@ GNATS_user (int ac, char **av)
 	      free (currentPassword);
 	    }
 	  currentUsername = xstrdup (av[0]);
-	  currentPassword = xstrdup (av[1]);
+	  if (ac == 2)
+	    {
+	      currentPassword = xstrdup (av[1]);
+	    }
+	  else
+	    {
+	      currentPassword = (char *)"";
+	    }
 	  printf ("%d Current database is not valid; use CHDB to set the database\r\n",
 		  CODE_OK);
 	}
     }
   else
     {
-      printf ("%d Need two arguments, username and password\r\n", 
+      printf ("%d Need one or two arguments, username and optionally a password\r\n", 
 	      CODE_CMD_ERROR);
     }
 }
@@ -593,14 +600,18 @@ gnatsdChdb (const char *nameOfDb, const 
       currentUsername = xstrdup (username);
     }
 
+  if (currentPassword != NULL)
+    {
+      free (currentPassword);
+    }
   if (passwd != NULL)
     {
-      if (currentPassword != NULL)
-	{
-	  free (currentPassword);
-	}
       currentPassword = xstrdup (passwd);
     }
+  else
+    {
+      currentPassword = NULL;
+    }
 
   if (currentUsername == NULL)
     {
@@ -670,9 +681,9 @@ GNATS_chdb (int ac, char **av)
   const char *user = NULL;
   const char *passwd = NULL;
 
-  if (ac != 1 && ac != 3)
+  if (ac != 1 && ac != 2 && ac != 3)
     {
-      printf ("%d One or three arguments required.\r\n", CODE_CMD_ERROR); 
+      printf ("%d One, two, or three arguments required.\r\n", CODE_CMD_ERROR); 
       return;
     }
 
@@ -680,6 +691,10 @@ GNATS_chdb (int ac, char **av)
     {
       user = av[1];
       passwd = av[2];
+    }
+  else if (ac == 2)
+    {
+      user = av[1];
     }
 
   if (gnatsdChdb (av[0], user, passwd, 0, &err) != 0)
Index: gnatsd.access
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/gnatsd.access,v
retrieving revision 1.5
diff -u -p -r1.5 gnatsd.access
--- gnatsd.access	16 Oct 2001 15:06:56 -0000	1.5
+++ gnatsd.access	26 Sep 2002 22:19:02 -0000
@@ -17,6 +17,8 @@
 #               assumed to be encrypted with standard crypt(), while passwords
 #               prefixed with $1$ are assumed to be MD5 encrypted.
 #               MD5 and crypt() encryption may not be available on all systems.
+#		An empty field value means that the user should not supply any
+#		password.
 #       * access-level: (default = edit)
 #               deny     - gnatsd closes the connection
 #               none     - no further access until userid and password given
@@ -33,4 +35,4 @@
 #         It's ignored in gnatsd-adm/gnatsd.access since this file is already
 #         database specific.
 #
-#*:*:view:
+#*::view:
Index: gnatsd.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/gnatsd.c,v
retrieving revision 1.47
diff -u -p -r1.47 gnatsd.c
--- gnatsd.c	4 Aug 2002 10:58:29 -0000	1.47
+++ gnatsd.c	26 Sep 2002 22:19:03 -0000
@@ -253,21 +253,45 @@ match (const char *line, const char *pat
 static int
 password_match (const char *password, const char *hash)
 {
-  if (! strncmp (hash, "$0$", 3))
-    {
-      /* explicit plain-text password */
-      return ! match (password, hash, TRUE);
-    }
-  else
+  char *hashvalue, *encrypted;
+
+  if (strlen(password) && hash)
     {
-      /* DES crypt or MD5 hash of the password */
+      if (! strncmp (hash, "$0$", 3))
+	{
+	  /* explicit plain-text password */
+	  return match (password, hash+3, TRUE);
+	}
+      else
+	{
 #ifdef HAVE_LIBCRYPT
-      char *encrypted = crypt (password, hash);
-      return encrypted && ! strcmp (encrypted, hash);
+	  if (! strncmp (hash, "$1$", 3))
+	    {
+	      hashvalue = (char *)hash+3;
+	    }
+	  else
+	    {
+	      hashvalue = (char *)hash;
+	    }
+	  /* DES crypt or MD5 hash of the password */
+	  encrypted = crypt (password, hashvalue);
+	  return encrypted && ! strcmp (encrypted, hashvalue);
 #else
-      /* TODO: log some warning */
-      return FALSE;
+	  /* TODO: log some warning */
+	  return FALSE;
 #endif
+	}
+    }
+  else
+    {
+      if (strlen(password))
+	{
+	  return FALSE;
+	}
+      else
+	{
+	  return ! hash ;
+	}
     }
 }
 
@@ -437,10 +461,10 @@ findUserAccessLevel (const char *file, c
   while (! found)
     {
       char *buffer = read_line (acc, NULL);
+
       if (buffer != NULL)
 	{
 	  AdmEntry *ent = build_adm_entry (buffer, InvalidFieldIndex);
-
 	  free (buffer);
 	  /* Is there a mistake on the setup of this line? (must have 3 or
 	     4 fields) */
@@ -450,8 +474,11 @@ findUserAccessLevel (const char *file, c
 	      if (! password_match (passwd, ent->admFields[1]))
 		{
 		  /* Username matched but password didn't.  */
-		  *access = ACCESS_NONE;
-		  found = 1;
+		  if (ent->admFields[1] && strlen(passwd))
+		    {
+		      *access = ACCESS_NONE;
+		      found = 1;
+		    }
 		}
 	      else
 		{
@@ -460,7 +487,10 @@ findUserAccessLevel (const char *file, c
 		      /* Compare all given names against the name of the
 			 requested database. */
 		      const char *l2 = ent->admFields[3];
-
+		      
+		      if (l2 == NULL)
+			found = 1;
+		      
 		      while (l2 != NULL && ! found)
 			{
 			  char *token = get_next_field (&l2, ',');
Index: misc.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/misc.c,v
retrieving revision 1.36
diff -u -p -r1.36 misc.c
--- misc.c	6 Jan 2002 16:13:20 -0000	1.36
+++ misc.c	26 Sep 2002 22:19:03 -0000
@@ -287,7 +287,10 @@ get_next_field (const char **line_ptr, i
       *line_ptr = NULL;
     }
 
-  return res;
+  if (end_line == line)
+    return NULL ;
+  else
+    return res;
 }
 
 /* Adds quote-marks (") around the string, and escapes any quotes that

[-- Attachment #3: gnatswebpatch.out --]
[-- Type: text/plain, Size: 1226 bytes --]

Index: gnatsweb.pl
===================================================================
RCS file: /cvsroot/gnatsweb/gnatsweb/gnatsweb.pl,v
retrieving revision 1.101
diff -u -p -r1.101 gnatsweb.pl
--- gnatsweb.pl	24 Sep 2002 20:39:17 -0000	1.101
+++ gnatsweb.pl	26 Sep 2002 22:20:26 -0000
@@ -4079,7 +4079,13 @@ sub init_prefs
   }
   %db_prefs = ();
   set_pref('user', \%db_prefs, \%cvals);
-  set_pref('password', \%db_prefs, \%cvals);
+  if (defined $q->param('password')) {
+    $db_prefs{'password'} = $q->param('password');
+  }
+  else
+  {
+    set_pref('password', \%db_prefs, \%cvals);
+  }
 
   # Debug.
   warn "global_prefs = ", Dumper(\%global_prefs) if $debug;
@@ -4189,10 +4195,9 @@ sub main
   ### Cookie-related code must happen before we print the HTML header.
   init_prefs();
 
-  if(!$global_prefs{'database'}
-        || !$db_prefs{'user'} || !$db_prefs{'password'})
+  if(!$global_prefs{'database'} || !$db_prefs{'user'})
   {
-    # We don't have username/password/database; give login page then
+    # We don't have username/database; give login page then
     # redirect to the url they really want (self_url).
     print_header();
     login_page($q->self_url());

RE: Patch: Fix user authentication + MKDB

[Yngve Svendsen]

At 16:16 26.09.2002 -0700, Pankaj K Garg wrote:
>I'm attaching a patch "gnatspatch.out" with this message that
>does this. Seems to work on my small tries here, but would
>appreciate if someone else can take a look at it also.
>
>The empty password bit required a change (backwards compatible)
>to the protocol. Previously, the client was required to
>give:
>
>  USER <name> <password>
>  CHDB <db> <name> <password>
>
>Now, clients can give:
>
>  USER <name> [<password>]
>  CHDB <db> <name> [<password>]
>
>Hence, the password is optional and will be assumed to be
>empty.
>
>For clients to take advantage of this, however, they may
>need some modification. For example, gnatsweb assumes that
>the user always requires some value for the password.
>So, I'm also attaching a patch for gnatsweb ("gnatswebpatch.out")
>that takes care of empty password situation.
>
>If the gnats gurus will accept the gnats patch, then I can
>submit the gnatsweb patch to the appropriate mailing list.

You don't need to. If we decide to apply the patch to GNATS, I will apply 
the necessary changes to Gnatsweb as well.

I think the behaviour this patch would introduce is sensible, but I will 
wait for people's comments before possibly applying it. Be aware that I am 
not a seasoned C coder, so if someone would take a look at the 
tecnicalities of the patch, that would be excellent.

Yngve Svendsen
Gnatsweb maintainer



_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://mail.gnu.org/mailman/listinfo/help-gnats

Re: Patch: Fix user authentication + MKDB

[Lars Henriksen]

On Thu, Sep 26, 2002 at 04:16:53PM -0700, Pankaj K Garg wrote:
> > ...
> >          foo:$0$test:edit:
> >          *::view:
> > 
> > Then the behavior IMHO should be:
> >  - user foo gives correct password  --> 'edit' access
> >  - user foo gives wrong password    --> no access
> >  - user foo gives no/empty password --> 'view' access
> >  - user bar gives any password      --> no access
> >  - user bar gives no/empty password --> 'view' access

Looks OK to me.

> I'm attaching a patch "gnatspatch.out" with this message that
> does this. Seems to work on my small tries here, but would
> appreciate if someone else can take a look at it also.
...
> BTW, the documentation in gnatsd.access states that $1$ implies
> use of MD5. In the code I did not find the use of MD5 hashes...
> am I missing something here?

As I understand it, MD5 password encryption is built into some versions
of libcrypt, again see the "Keep Track" manual (section C.4),
it's not bad :-)

My system doesn't support MD5 (HAVE_LIBCRYPT undefined) so your patch
wouldn't compile. That was easily fixed by moving things around:

*** gnatsd.c.patch      2002-09-27 14:16:49.000000000 +0200
--- gnatsd.c    2002-09-27 07:50:09.000000000 +0200
***************
*** 253,260 ****
  static int
  password_match (const char *password, const char *hash)
  {
-   char *hashvalue, *encrypted;
- 
    if (strlen(password) && hash)
      {
        if (! strncmp (hash, "$0$", 3))
--- 253,258 ----
***************
*** 265,270 ****
--- 263,270 ----
        else
        {
  #ifdef HAVE_LIBCRYPT
+         char *hashvalue, *encrypted;
+ 
          if (! strncmp (hash, "$1$", 3))
            {
              hashvalue = (char *)hash+3;

Then from the command line:

130$ ./gnatsd -n
200 cluster2.netman.dk GNATS server 4.0-beta1 ready.
user lh
Segmentation fault (core dumped)
131$ dbx gnatsd core
dbx version 5.1
Type 'help' for help.
Core file created by program "gnatsd"

signal Segmentation fault at >*[strlen, 0x3ff800d1d30]  ldq_u   t0, 0(a0)
(dbx) where
>  0 strlen(0x120033f58, 0xf, 0xf, 0x140032660, 0x1200340d4) [0x3ff800d1d30]
   1 xstrdup() ["../../gnats-4/libiberty/xstrdup.c":6, 0x1200340b4]
   2 copy_adm_entry() ["../../gnats-4/gnats/adm.c":6, 0x12001a1f8]
   3 get_responsible_address() ["../../gnats-4/gnats/mail.c":6, 0x120026504]
   4 get_one_responsible_addr() ["../../gnats-4/gnats/mail.c":6, 0x120026738]
   5 get_responsible_addr() ["../../gnats-4/gnats/mail.c":6, 0x120026b60]
   6 gnatsdChdb() ["../../gnats-4/gnats/cmds.c":6, 0x12000b5a0]
   7 GNATS_user() ["../../gnats-4/gnats/cmds.c":6, 0x12000aae8]
   8 serverMainLoop() ["../../gnats-4/gnats/gnatsd.c":6, 0x120009ca0]
   9 main() ["../../gnats-4/gnats/gnatsd.c":6, 0x12000a0e8]
(dbx)

As an aside: if gnatsd for some reason cannot access the responsible file
(or the gnatsd.user_access file for that matter) it silently ignores the fact
which seems rather strange (but has nothing to do with your patch).

By the way, don't forget the GNATS_help() function at the end of cmds.c
and the manual of course (I'm willing to help with that).

Lars Henriksen


_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://mail.gnu.org/mailman/listinfo/help-gnats

RE: Patch: Fix user authentication + MKDB

[Pankaj K Garg]

[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]

Thanks for testing the patch.

> ...
> signal Segmentation fault at >*[strlen, 0x3ff800d1d30]  ldq_u 
>   t0, 0(a0)
> (dbx) where
> >  0 strlen(0x120033f58, 0xf, 0xf, 0x140032660, 0x1200340d4) 
> [0x3ff800d1d30]
>    1 xstrdup() ["../../gnats-4/libiberty/xstrdup.c":6, 0x1200340b4]
>    2 copy_adm_entry() ["../../gnats-4/gnats/adm.c":6, 0x12001a1f8]
>    3 get_responsible_address() 
> ["../../gnats-4/gnats/mail.c":6, 0x120026504]
>    4 get_one_responsible_addr() 
> ["../../gnats-4/gnats/mail.c":6, 0x120026738]
>    5 get_responsible_addr() ["../../gnats-4/gnats/mail.c":6, 
> 0x120026b60]
>    6 gnatsdChdb() ["../../gnats-4/gnats/cmds.c":6, 0x12000b5a0]
>    7 GNATS_user() ["../../gnats-4/gnats/cmds.c":6, 0x12000aae8]
>    8 serverMainLoop() ["../../gnats-4/gnats/gnatsd.c":6, 0x120009ca0]
>    9 main() ["../../gnats-4/gnats/gnatsd.c":6, 0x12000a0e8]
> (dbx)
>

I was not able to replicate this problem even with LIB_CRYPT set
to 0. But, I suspect this is happening because I changed the
fields routines to return a NULL in case of an empty field. Sounds
like a bad idea, as obviously some routines depend on it being
non-NULL.

So, I've changed this in a new patch. Now, for an empty field
we return an empty string (""). Hopefully, this should fix
things. Please try the attached patch and let me know.

> As nn aside: if gnatsd for some reason cannot access the 
> responsible file
> (or the gnatsd.user_access file for that matter) it silently 
> ignores the fact
> which seems rather strange (but has nothing to do with your patch).

Yes, we should log it somewhere. I can take care of this after this
round of changes stabilize.

> By the way, don't forget the GNATS_help() function at the end 
> of cmds.c
> and the manual of course (I'm willing to help with that).

I made some changes to the GNATS_help() function. We can fix the manual
once we stabilize the code.

Regards

Pankaj
 

[-- Attachment #2: patchfile.out --]
[-- Type: text/plain, Size: 6463 bytes --]

Index: cmds.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/cmds.c,v
retrieving revision 1.69
diff -u -p -r1.69 cmds.c
--- cmds.c	12 Aug 2002 12:33:30 -0000	1.69
+++ cmds.c	27 Sep 2002 17:02:55 -0000
@@ -318,11 +318,11 @@ GNATS_user (int ac, char **av)
       printf ("%d %s\r\n", CODE_INFORMATION,
 	      access_level_str (user_access));
     }
-  else if (ac == 2)
+  else if ((ac == 1) || (ac == 2))
     {
       if (databaseValid (currentDatabase))
 	{
-	  if (gnatsdChdb (databaseName (currentDatabase), av[0], av[1], 0,
+	  if (gnatsdChdb (databaseName (currentDatabase), av[0], ac == 2 ? av[1] : "", 0,
 			  &err) != 0)
 	    {
 	      print_server_errors (err);
@@ -339,14 +339,21 @@ GNATS_user (int ac, char **av)
 	      free (currentPassword);
 	    }
 	  currentUsername = xstrdup (av[0]);
-	  currentPassword = xstrdup (av[1]);
+	  if (ac == 2)
+	    {
+	      currentPassword = xstrdup (av[1]);
+	    }
+	  else
+	    {
+	      currentPassword = (char *)"";
+	    }
 	  printf ("%d Current database is not valid; use CHDB to set the database\r\n",
 		  CODE_OK);
 	}
     }
   else
     {
-      printf ("%d Need two arguments, username and password\r\n", 
+      printf ("%d Need one or two arguments, username and optionally a password\r\n", 
 	      CODE_CMD_ERROR);
     }
 }
@@ -593,14 +600,18 @@ gnatsdChdb (const char *nameOfDb, const 
       currentUsername = xstrdup (username);
     }
 
+  if (currentPassword != NULL)
+    {
+      free (currentPassword);
+    }
   if (passwd != NULL)
     {
-      if (currentPassword != NULL)
-	{
-	  free (currentPassword);
-	}
       currentPassword = xstrdup (passwd);
     }
+  else
+    {
+      currentPassword = NULL;
+    }
 
   if (currentUsername == NULL)
     {
@@ -670,9 +681,9 @@ GNATS_chdb (int ac, char **av)
   const char *user = NULL;
   const char *passwd = NULL;
 
-  if (ac != 1 && ac != 3)
+  if (ac != 1 && ac != 2 && ac != 3)
     {
-      printf ("%d One or three arguments required.\r\n", CODE_CMD_ERROR); 
+      printf ("%d One, two, or three arguments required.\r\n", CODE_CMD_ERROR); 
       return;
     }
 
@@ -681,6 +692,10 @@ GNATS_chdb (int ac, char **av)
       user = av[1];
       passwd = av[2];
     }
+  else if (ac == 2)
+    {
+      user = av[1];
+    }
 
   if (gnatsdChdb (av[0], user, passwd, 0, &err) != 0)
     {
@@ -1786,11 +1801,11 @@ GNATS_help (int ac ATTRIBUTE_UNUSED, cha
  	  CODE_INFORMATION);
   printf ("%d-   SUBM                    submit a new PR\r\n",
 	  CODE_INFORMATION);
-  printf ("%d-   CHDB <database> [<user> <passwd>]\r\n",
+  printf ("%d-   CHDB <database> [<user> [<passwd>]]\r\n",
 	  CODE_INFORMATION);
   printf ("%d-                           change GNATS ROOT to <database>\r\n",
 	  CODE_INFORMATION);
-  printf ("%d-   USER <name> <passwd>    Sets the current user\r\n",
+  printf ("%d-   USER <name> [<passwd>]  Sets the current user\r\n",
 	  CODE_INFORMATION);
   printf ("%d-   USER                    Report current access level\r\n",
 	  CODE_INFORMATION);
Index: gnatsd.access
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/gnatsd.access,v
retrieving revision 1.5
diff -u -p -r1.5 gnatsd.access
--- gnatsd.access	16 Oct 2001 15:06:56 -0000	1.5
+++ gnatsd.access	27 Sep 2002 17:02:55 -0000
@@ -17,6 +17,8 @@
 #               assumed to be encrypted with standard crypt(), while passwords
 #               prefixed with $1$ are assumed to be MD5 encrypted.
 #               MD5 and crypt() encryption may not be available on all systems.
+#		An empty field value means that the user should not supply any
+#		password.
 #       * access-level: (default = edit)
 #               deny     - gnatsd closes the connection
 #               none     - no further access until userid and password given
@@ -33,4 +35,4 @@
 #         It's ignored in gnatsd-adm/gnatsd.access since this file is already
 #         database specific.
 #
-#*:*:view:
+#*::view:
Index: gnatsd.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/gnatsd.c,v
retrieving revision 1.47
diff -u -p -r1.47 gnatsd.c
--- gnatsd.c	4 Aug 2002 10:58:29 -0000	1.47
+++ gnatsd.c	27 Sep 2002 17:02:55 -0000
@@ -253,21 +253,45 @@ match (const char *line, const char *pat
 static int
 password_match (const char *password, const char *hash)
 {
-  if (! strncmp (hash, "$0$", 3))
+  if (strlen(password) && strlen(hash))
     {
-      /* explicit plain-text password */
-      return ! match (password, hash, TRUE);
-    }
-  else
-    {
-      /* DES crypt or MD5 hash of the password */
+      if (! strncmp (hash, "$0$", 3))
+	{
+	  /* explicit plain-text password */
+	  return match (password, hash+3, TRUE);
+	}
+      else
+	{
 #ifdef HAVE_LIBCRYPT
-      char *encrypted = crypt (password, hash);
-      return encrypted && ! strcmp (encrypted, hash);
+	  char *hashvalue, *encrypted;
+
+	  if (! strncmp (hash, "$1$", 3))
+	    {
+	      hashvalue = (char *)hash+3;
+	    }
+	  else
+	    {
+	      hashvalue = (char *)hash;
+	    }
+	  /* DES crypt or MD5 hash of the password */
+	  encrypted = crypt (password, hashvalue);
+	  return encrypted && ! strcmp (encrypted, hashvalue);
 #else
-      /* TODO: log some warning */
-      return FALSE;
+	  /* TODO: log some warning */
+	  return FALSE;
 #endif
+	}
+    }
+  else
+    {
+      if (strlen(password))
+	{
+	  return FALSE;
+	}
+      else
+	{
+	  return ! strlen(hash) ;
+	}
     }
 }
 
@@ -450,8 +474,11 @@ findUserAccessLevel (const char *file, c
 	      if (! password_match (passwd, ent->admFields[1]))
 		{
 		  /* Username matched but password didn't.  */
-		  *access = ACCESS_NONE;
-		  found = 1;
+		  if (strlen(ent->admFields[1]) && strlen(passwd))
+		    {
+		      *access = ACCESS_NONE;
+		      found = 1;
+		    }
 		}
 	      else
 		{
@@ -460,7 +487,10 @@ findUserAccessLevel (const char *file, c
 		      /* Compare all given names against the name of the
 			 requested database. */
 		      const char *l2 = ent->admFields[3];
-
+		      
+		      if (! strlen(l2))
+			found = 1;
+		      
 		      while (l2 != NULL && ! found)
 			{
 			  char *token = get_next_field (&l2, ',');

Re: Patch: Fix user authentication + MKDB

[Lars Henriksen]

On Fri, Sep 27, 2002 at 10:37:04AM -0700, Pankaj K Garg wrote:

> So, I've changed this in a new patch. Now, for an empty field
> we return an empty string (""). Hopefully, this should fix
> things. Please try the attached patch and let me know.

Yes, that helped. As you already know I'm only able to test plaintext
and no password. They seem to work as agreed. I've tested with two
databases, with users present in or absent from the responsible file,
with and without password and with the site and database specific
gnatsd.user_access files.

I've spotted one difference in behaviour: gnatsd no longer seems to
remember the user when you switch between databases (it still remembers
the database when you switch user). In the following sessions there
are two databases, both with anybody/any password edit access
specified in the database specific user access file; the site access
level is listdb.

Two sessions follow. First the existing gnatsd:

15$ ./gnatsd -n
200 cluster2.netman.dk GNATS server 4.0-beta1 ready.
chdb development x y
210-Now accessing GNATS database 'development'
210 User access level set to 'edit'
chdb support
210-Now accessing GNATS database 'support'
210 User access level set to 'edit'
quit
201 Later.

Then the patched version:

17$ ./gnatsd -n
200 cluster2.netman.dk GNATS server 4.0-beta1 ready.
chdb development x y
210-Now accessing GNATS database 'development'
210 User access level set to 'edit'
chdb support
210-Now accessing GNATS database 'support'
210 User access level set to 'listdb'
user x y
210-Now accessing GNATS database 'support'
210 User access level set to 'edit'
quit
201 Later.

Regards
Lars


_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://mail.gnu.org/mailman/listinfo/help-gnats

RE: Patch: Fix user authentication + MKDB

[Pankaj K Garg]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

>...
> I've spotted one difference in behaviour: gnatsd no longer seems to
> remember the user when you switch between databases (it still 
> remembers
> the database when you switch user). In the following sessions there
> are two databases, both with anybody/any password edit access
> specified in the database specific user access file; the site access
> level is listdb.

OOpss...yes, this should not have happened. There was a problem with
the way I was handling NULL usernames and passwords.

Should be fixed in the attached patch.

Regards

Pankaj
 

[-- Attachment #2: patchfile.out --]
[-- Type: text/plain, Size: 6491 bytes --]

Index: cmds.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/cmds.c,v
retrieving revision 1.69
diff -u -p -r1.69 cmds.c
--- cmds.c	12 Aug 2002 12:33:30 -0000	1.69
+++ cmds.c	27 Sep 2002 21:22:25 -0000
@@ -318,11 +318,11 @@ GNATS_user (int ac, char **av)
       printf ("%d %s\r\n", CODE_INFORMATION,
 	      access_level_str (user_access));
     }
-  else if (ac == 2)
+  else if ((ac == 1) || (ac == 2))
     {
       if (databaseValid (currentDatabase))
 	{
-	  if (gnatsdChdb (databaseName (currentDatabase), av[0], av[1], 0,
+	  if (gnatsdChdb (databaseName (currentDatabase), av[0], ac == 2 ? av[1] : "", 0,
 			  &err) != 0)
 	    {
 	      print_server_errors (err);
@@ -339,14 +339,21 @@ GNATS_user (int ac, char **av)
 	      free (currentPassword);
 	    }
 	  currentUsername = xstrdup (av[0]);
-	  currentPassword = xstrdup (av[1]);
+	  if (ac == 2)
+	    {
+	      currentPassword = xstrdup (av[1]);
+	    }
+	  else
+	    {
+	      currentPassword = (char *)"";
+	    }
 	  printf ("%d Current database is not valid; use CHDB to set the database\r\n",
 		  CODE_OK);
 	}
     }
   else
     {
-      printf ("%d Need two arguments, username and password\r\n", 
+      printf ("%d Need one or two arguments, username and optionally a password\r\n", 
 	      CODE_CMD_ERROR);
     }
 }
@@ -591,15 +598,19 @@ gnatsdChdb (const char *nameOfDb, const 
 	  free (currentUsername);
 	}
       currentUsername = xstrdup (username);
-    }
-
-  if (passwd != NULL)
-    {
+      
       if (currentPassword != NULL)
 	{
 	  free (currentPassword);
 	}
-      currentPassword = xstrdup (passwd);
+      if (passwd != NULL)
+	{
+	  currentPassword = xstrdup (passwd);
+	}
+      else
+	{
+	  currentPassword = NULL;
+	}
     }
 
   if (currentUsername == NULL)
@@ -670,9 +681,9 @@ GNATS_chdb (int ac, char **av)
   const char *user = NULL;
   const char *passwd = NULL;
 
-  if (ac != 1 && ac != 3)
+  if (ac != 1 && ac != 2 && ac != 3)
     {
-      printf ("%d One or three arguments required.\r\n", CODE_CMD_ERROR); 
+      printf ("%d One, two, or three arguments required.\r\n", CODE_CMD_ERROR); 
       return;
     }
 
@@ -681,6 +692,10 @@ GNATS_chdb (int ac, char **av)
       user = av[1];
       passwd = av[2];
     }
+  else if (ac == 2)
+    {
+      user = av[1];
+    }
 
   if (gnatsdChdb (av[0], user, passwd, 0, &err) != 0)
     {
@@ -1786,11 +1801,11 @@ GNATS_help (int ac ATTRIBUTE_UNUSED, cha
  	  CODE_INFORMATION);
   printf ("%d-   SUBM                    submit a new PR\r\n",
 	  CODE_INFORMATION);
-  printf ("%d-   CHDB <database> [<user> <passwd>]\r\n",
+  printf ("%d-   CHDB <database> [<user> [<passwd>]]\r\n",
 	  CODE_INFORMATION);
   printf ("%d-                           change GNATS ROOT to <database>\r\n",
 	  CODE_INFORMATION);
-  printf ("%d-   USER <name> <passwd>    Sets the current user\r\n",
+  printf ("%d-   USER <name> [<passwd>]  Sets the current user\r\n",
 	  CODE_INFORMATION);
   printf ("%d-   USER                    Report current access level\r\n",
 	  CODE_INFORMATION);
Index: gnatsd.access
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/gnatsd.access,v
retrieving revision 1.5
diff -u -p -r1.5 gnatsd.access
--- gnatsd.access	16 Oct 2001 15:06:56 -0000	1.5
+++ gnatsd.access	27 Sep 2002 21:22:25 -0000
@@ -17,6 +17,8 @@
 #               assumed to be encrypted with standard crypt(), while passwords
 #               prefixed with $1$ are assumed to be MD5 encrypted.
 #               MD5 and crypt() encryption may not be available on all systems.
+#		An empty field value means that the user should not supply any
+#		password.
 #       * access-level: (default = edit)
 #               deny     - gnatsd closes the connection
 #               none     - no further access until userid and password given
@@ -33,4 +35,4 @@
 #         It's ignored in gnatsd-adm/gnatsd.access since this file is already
 #         database specific.
 #
-#*:*:view:
+#*::view:
Index: gnatsd.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/gnatsd.c,v
retrieving revision 1.47
diff -u -p -r1.47 gnatsd.c
--- gnatsd.c	4 Aug 2002 10:58:29 -0000	1.47
+++ gnatsd.c	27 Sep 2002 21:22:25 -0000
@@ -253,21 +253,45 @@ match (const char *line, const char *pat
 static int
 password_match (const char *password, const char *hash)
 {
-  if (! strncmp (hash, "$0$", 3))
+  if (strlen(password) && strlen(hash))
     {
-      /* explicit plain-text password */
-      return ! match (password, hash, TRUE);
-    }
-  else
-    {
-      /* DES crypt or MD5 hash of the password */
+      if (! strncmp (hash, "$0$", 3))
+	{
+	  /* explicit plain-text password */
+	  return match (password, hash+3, TRUE);
+	}
+      else
+	{
 #ifdef HAVE_LIBCRYPT
-      char *encrypted = crypt (password, hash);
-      return encrypted && ! strcmp (encrypted, hash);
+	  char *hashvalue, *encrypted;
+
+	  if (! strncmp (hash, "$1$", 3))
+	    {
+	      hashvalue = (char *)hash+3;
+	    }
+	  else
+	    {
+	      hashvalue = (char *)hash;
+	    }
+	  /* DES crypt or MD5 hash of the password */
+	  encrypted = crypt (password, hashvalue);
+	  return encrypted && ! strcmp (encrypted, hashvalue);
 #else
-      /* TODO: log some warning */
-      return FALSE;
+	  /* TODO: log some warning */
+	  return FALSE;
 #endif
+	}
+    }
+  else
+    {
+      if (strlen(password))
+	{
+	  return FALSE;
+	}
+      else
+	{
+	  return ! strlen(hash) ;
+	}
     }
 }
 
@@ -450,8 +474,11 @@ findUserAccessLevel (const char *file, c
 	      if (! password_match (passwd, ent->admFields[1]))
 		{
 		  /* Username matched but password didn't.  */
-		  *access = ACCESS_NONE;
-		  found = 1;
+		  if (strlen(ent->admFields[1]) && strlen(passwd))
+		    {
+		      *access = ACCESS_NONE;
+		      found = 1;
+		    }
 		}
 	      else
 		{
@@ -460,7 +487,10 @@ findUserAccessLevel (const char *file, c
 		      /* Compare all given names against the name of the
 			 requested database. */
 		      const char *l2 = ent->admFields[3];
-
+		      
+		      if (! strlen(l2))
+			found = 1;
+		      
 		      while (l2 != NULL && ! found)
 			{
 			  char *token = get_next_field (&l2, ',');

Re: Patch: Fix user authentication + MKDB

[Lars Henriksen]

On Fri, Sep 27, 2002 at 02:59:28PM -0700, Pankaj K Garg wrote:
> >...
> > I've spotted one difference in behaviour: gnatsd no longer seems to
> > remember the user when you switch between databases (it still 
> > remembers
> 
> OOpss...yes, this should not have happened. There was a problem with
> the way I was handling NULL usernames and passwords.
> 
> Should be fixed in the attached patch.

It is! I believe your patch is OK now as far as plaintext/no password
is concerned. Apart from making the password checking work, this is a
convenient improvement.

But as for DES/MD5 I believe the original code is correct:

  else
    {
      /* DES crypt or MD5 hash of the password */
#ifdef HAVE_LIBCRYPT
      char *encrypted = crypt (password, hash);
      return encrypted && ! strcmp (encrypted, hash);
#else
      /* TODO: log some warning */
      return FALSE;
#endif
    }

It is for crypt() to decide the form of password encryption based on
the contents of hash: if hash begins with $1$ it will use MD5, if it
doesn't begin with $<digit>$ it will use DES. The return value of
crypt() is similarly adjusted with a starting $1$ for MD5. This assumes
an MD5-supporting crypt(3) (e.g. FreeBSD or GNU). With a traditional
Unix crypt() function you will of course get DES encryption.

Regards
Lars Henriksen


_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://mail.gnu.org/mailman/listinfo/help-gnats

RE: Patch: Fix user authentication + MKDB

[Pankaj K Garg]

[-- Attachment #1: Type: text/plain, Size: 743 bytes --]

>...

> But as for DES/MD5 I believe the original code is correct:
> 
>   else
>     {
>       /* DES crypt or MD5 hash of the password */
> #ifdef HAVE_LIBCRYPT
>       char *encrypted = crypt (password, hash);
>       return encrypted && ! strcmp (encrypted, hash);
> #else
>       /* TODO: log some warning */
>       return FALSE;
> #endif
>     }
> ...

Thanks, yes. I stand corrected. I was thrown off by the man page
for crypt on my Linux machine. But on a bit of searching found out
that FreeBSD crypt will do this. BTW, the documentation with FreeBSD
also mentioned the use of $2$ for BlowFish algorithm. Should we mention
that in the documentation?

I'm attaching a corrected patch with the original DES/MD5 code.

Regards

Pankaj

 

[-- Attachment #2: patchfile.out --]
[-- Type: text/plain, Size: 6594 bytes --]

Index: cmds.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/cmds.c,v
retrieving revision 1.69
diff -u -p -r1.69 cmds.c
--- cmds.c	12 Aug 2002 12:33:30 -0000	1.69
+++ cmds.c	30 Sep 2002 03:22:27 -0000
@@ -318,11 +318,11 @@ GNATS_user (int ac, char **av)
       printf ("%d %s\r\n", CODE_INFORMATION,
 	      access_level_str (user_access));
     }
-  else if (ac == 2)
+  else if ((ac == 1) || (ac == 2))
     {
       if (databaseValid (currentDatabase))
 	{
-	  if (gnatsdChdb (databaseName (currentDatabase), av[0], av[1], 0,
+	  if (gnatsdChdb (databaseName (currentDatabase), av[0], ac == 2 ? av[1] : "", 0,
 			  &err) != 0)
 	    {
 	      print_server_errors (err);
@@ -339,14 +339,21 @@ GNATS_user (int ac, char **av)
 	      free (currentPassword);
 	    }
 	  currentUsername = xstrdup (av[0]);
-	  currentPassword = xstrdup (av[1]);
+	  if (ac == 2)
+	    {
+	      currentPassword = xstrdup (av[1]);
+	    }
+	  else
+	    {
+	      currentPassword = (char *)"";
+	    }
 	  printf ("%d Current database is not valid; use CHDB to set the database\r\n",
 		  CODE_OK);
 	}
     }
   else
     {
-      printf ("%d Need two arguments, username and password\r\n", 
+      printf ("%d Need one or two arguments, username and optionally a password\r\n", 
 	      CODE_CMD_ERROR);
     }
 }
@@ -591,15 +598,19 @@ gnatsdChdb (const char *nameOfDb, const 
 	  free (currentUsername);
 	}
       currentUsername = xstrdup (username);
-    }
-
-  if (passwd != NULL)
-    {
+      
       if (currentPassword != NULL)
 	{
 	  free (currentPassword);
 	}
-      currentPassword = xstrdup (passwd);
+      if (passwd != NULL)
+	{
+	  currentPassword = xstrdup (passwd);
+	}
+      else
+	{
+	  currentPassword = NULL;
+	}
     }
 
   if (currentUsername == NULL)
@@ -670,9 +681,9 @@ GNATS_chdb (int ac, char **av)
   const char *user = NULL;
   const char *passwd = NULL;
 
-  if (ac != 1 && ac != 3)
+  if (ac != 1 && ac != 2 && ac != 3)
     {
-      printf ("%d One or three arguments required.\r\n", CODE_CMD_ERROR); 
+      printf ("%d One, two, or three arguments required.\r\n", CODE_CMD_ERROR); 
       return;
     }
 
@@ -681,6 +692,10 @@ GNATS_chdb (int ac, char **av)
       user = av[1];
       passwd = av[2];
     }
+  else if (ac == 2)
+    {
+      user = av[1];
+    }
 
   if (gnatsdChdb (av[0], user, passwd, 0, &err) != 0)
     {
@@ -1786,11 +1801,11 @@ GNATS_help (int ac ATTRIBUTE_UNUSED, cha
  	  CODE_INFORMATION);
   printf ("%d-   SUBM                    submit a new PR\r\n",
 	  CODE_INFORMATION);
-  printf ("%d-   CHDB <database> [<user> <passwd>]\r\n",
+  printf ("%d-   CHDB <database> [<user> [<passwd>]]\r\n",
 	  CODE_INFORMATION);
   printf ("%d-                           change GNATS ROOT to <database>\r\n",
 	  CODE_INFORMATION);
-  printf ("%d-   USER <name> <passwd>    Sets the current user\r\n",
+  printf ("%d-   USER <name> [<passwd>]  Sets the current user\r\n",
 	  CODE_INFORMATION);
   printf ("%d-   USER                    Report current access level\r\n",
 	  CODE_INFORMATION);
Index: gnatsd.access
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/gnatsd.access,v
retrieving revision 1.5
diff -u -p -r1.5 gnatsd.access
--- gnatsd.access	16 Oct 2001 15:06:56 -0000	1.5
+++ gnatsd.access	30 Sep 2002 03:22:27 -0000
@@ -14,9 +14,11 @@
 #       * userid:   a user id to gain access to gnatsd
 #       * password: a password for the user.  Passwords prefixed by $0$ are
 #               assumed to be plain-text.  Passwords without a prefix are
-#               assumed to be encrypted with standard crypt(), while passwords
+#               assumed to be encrypted with standard (DES) crypt(), while passwords
 #               prefixed with $1$ are assumed to be MD5 encrypted.
 #               MD5 and crypt() encryption may not be available on all systems.
+#		An empty field value means that the user should not supply any
+#		password.
 #       * access-level: (default = edit)
 #               deny     - gnatsd closes the connection
 #               none     - no further access until userid and password given
@@ -33,4 +35,4 @@
 #         It's ignored in gnatsd-adm/gnatsd.access since this file is already
 #         database specific.
 #
-#*:*:view:
+#*::view:
Index: gnatsd.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/gnatsd.c,v
retrieving revision 1.47
diff -u -p -r1.47 gnatsd.c
--- gnatsd.c	4 Aug 2002 10:58:29 -0000	1.47
+++ gnatsd.c	30 Sep 2002 03:22:27 -0000
@@ -253,21 +253,35 @@ match (const char *line, const char *pat
 static int
 password_match (const char *password, const char *hash)
 {
-  if (! strncmp (hash, "$0$", 3))
+  if (strlen(password) && strlen(hash))
     {
-      /* explicit plain-text password */
-      return ! match (password, hash, TRUE);
-    }
-  else
-    {
-      /* DES crypt or MD5 hash of the password */
+      if (! strncmp (hash, "$0$", 3))
+	{
+	  /* explicit plain-text password */
+	  return match (password, hash+3, TRUE);
+	}
+      else
+	{
 #ifdef HAVE_LIBCRYPT
-      char *encrypted = crypt (password, hash);
-      return encrypted && ! strcmp (encrypted, hash);
+	  /* DES crypt or MD5 hash of the password */
+	  char *encrypted = crypt (password, hash);
+	  return encrypted && ! strcmp (encrypted, hash);
 #else
-      /* TODO: log some warning */
-      return FALSE;
+	  /* TODO: log some warning */
+	  return FALSE;
 #endif
+	}
+    }
+  else
+    {
+      if (strlen(password))
+	{
+	  return FALSE;
+	}
+      else
+	{
+	  return ! strlen(hash) ;
+	}
     }
 }
 
@@ -450,8 +464,11 @@ findUserAccessLevel (const char *file, c
 	      if (! password_match (passwd, ent->admFields[1]))
 		{
 		  /* Username matched but password didn't.  */
-		  *access = ACCESS_NONE;
-		  found = 1;
+		  if (strlen(ent->admFields[1]) && strlen(passwd))
+		    {
+		      *access = ACCESS_NONE;
+		      found = 1;
+		    }
 		}
 	      else
 		{
@@ -460,7 +477,10 @@ findUserAccessLevel (const char *file, c
 		      /* Compare all given names against the name of the
 			 requested database. */
 		      const char *l2 = ent->admFields[3];
-
+		      
+		      if (! strlen(l2))
+			found = 1;
+		      
 		      while (l2 != NULL && ! found)
 			{
 			  char *token = get_next_field (&l2, ',');

Re: Patch: Fix user authentication + MKDB

[Lars Henriksen]

I've mailed this once before, but it never reached the list.

On Sun, Sep 29, 2002 at 09:01:58PM -0700, Pankaj K Garg wrote:
> 
>                                  BTW, the documentation with FreeBSD
> also mentioned the use of $2$ for BlowFish algorithm. Should we mention
> that in the documentation?

I think so. Or maybe a general remark that gnats supports whatever algorithm
the local crypt() supports, e.g. MD5 and BlowFish on FreeBSD.

> I'm attaching a corrected patch with the original DES/MD5 code.

Thanks. I believe this version is OK and may be comitted.

Regards
Lars Henriksen


_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://mail.gnu.org/mailman/listinfo/help-gnats

Re: Patch: Fix user authentication + MKDB

[Lars Henriksen]

On Sun, Sep 29, 2002 at 09:01:58PM -0700, Pankaj K Garg wrote:
> 
>                                  BTW, the documentation with FreeBSD
> also mentioned the use of $2$ for BlowFish algorithm. Should we mention
> that in the documentation?

I think so. Or maybe a general remark that gnats supports whatever algorithm
the local crypt() supports, e.g. MD5 and BlowFish on FreeBSD.

> I'm attaching a corrected patch with the original DES/MD5 code.

Thanks. I believe this version is OK and may be comitted.

Regards
Lars Henriksen


_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://mail.gnu.org/mailman/listinfo/help-gnats

RE: Patch: Fix user authentication + MKDB

[Pankaj K Garg]

[-- Attachment #1: Type: text/plain, Size: 406 bytes --]

> 
> Thanks. I believe this version is OK and may be comitted.
> 

Thanks. Here's the final version of the patch for both
Gnats and Gnatsweb.

I've taken care of the appropriate documentation for
Gnats also.

Can someone please commit these patches.

Regards

Pankaj

--- 
Pankaj K Garg              gargp@acm.org
1684 Nightingale Avenue    408-373-4027
Sunnyvale, CA 94304
http://home.earthlink.net/~gargp

[-- Attachment #2: gnatsweb.patch --]
[-- Type: application/octet-stream, Size: 1225 bytes --]

Index: gnatsweb.pl
===================================================================
RCS file: /cvsroot/gnatsweb/gnatsweb/gnatsweb.pl,v
retrieving revision 1.101
diff -u -p -r1.101 gnatsweb.pl
--- gnatsweb.pl	24 Sep 2002 20:39:17 -0000	1.101
+++ gnatsweb.pl	4 Oct 2002 17:06:30 -0000
@@ -4079,7 +4079,13 @@ sub init_prefs
   }
   %db_prefs = ();
   set_pref('user', \%db_prefs, \%cvals);
-  set_pref('password', \%db_prefs, \%cvals);
+  if (defined $q->param('password')) {
+    $db_prefs{'password'} = $q->param('password');
+  }
+  else
+  {
+    set_pref('password', \%db_prefs, \%cvals);
+  }
 
   # Debug.
   warn "global_prefs = ", Dumper(\%global_prefs) if $debug;
@@ -4189,10 +4195,9 @@ sub main
   ### Cookie-related code must happen before we print the HTML header.
   init_prefs();
 
-  if(!$global_prefs{'database'}
-        || !$db_prefs{'user'} || !$db_prefs{'password'})
+  if(!$global_prefs{'database'} || !$db_prefs{'user'})
   {
-    # We don't have username/password/database; give login page then
+    # We don't have username/database; give login page then
     # redirect to the url they really want (self_url).
     print_header();
     login_page($q->self_url());

[-- Attachment #3: gnats.patch --]
[-- Type: application/octet-stream, Size: 8481 bytes --]

Index: doc/gnats.texi
===================================================================
RCS file: /cvsroot/gnats/gnats/doc/gnats.texi,v
retrieving revision 1.26
diff -u -p -r1.26 gnats.texi
--- doc/gnats.texi	14 Jul 2002 17:27:47 -0000	1.26
+++ doc/gnats.texi	4 Oct 2002 17:04:55 -0000
@@ -1704,7 +1704,8 @@ OpenBSD.}.
 If the password is in plain text format, it must be prefixed by
 @samp{$0$} and if it is in MD5 format, it needs to be prefixed by the
 string @samp{$1$}.  Passwords encrypted by @code{crypt()} should have no
-prefix.
+prefix. If no password is given then users can login with an empty
+password string.
 
 A @code{gnats-passwd} tool to manage @file{gnatsd.user_access} files is
 planned.  In the meantime, @code{crypt()} passwords can be generated by
@@ -1739,20 +1740,20 @@ plain text passwords:
 @example
 rickm:$0$ruckm:edit
 pablo:$0$pueblo:view
-*:*:none
+*::none
 @end example
 
 @noindent And this is the same file with MD5-encrypted passwords:
 @example
 rickm:$1$92388613$D7ZIYikzTUqd./dODTFrI.:edit
 pablo:$1$92388652$QRfAhIBG5elT.FQjQKhj80:view
-*:*:none
+*::none
 @end example
 
 @noindent In these examples, anybody other than rickm and pablo get denied access,
 assuming that the host access level is also @code{none}.  You could set
-the catch-all rule at the end to be @code{*:*:view} to allow view access
-to anyone.
+the catch-all rule at the end to be @code{*::view} to allow view access
+to anyone who does not supply a password.
 
 The overall user access file
 @w{@file{@var{SYSCONFDIR}/gnats/gnatsd.user_access}}, usually
Index: gnats/cmds.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/cmds.c,v
retrieving revision 1.69
diff -u -p -r1.69 cmds.c
--- gnats/cmds.c	12 Aug 2002 12:33:30 -0000	1.69
+++ gnats/cmds.c	4 Oct 2002 17:04:56 -0000
@@ -318,11 +318,11 @@ GNATS_user (int ac, char **av)
       printf ("%d %s\r\n", CODE_INFORMATION,
 	      access_level_str (user_access));
     }
-  else if (ac == 2)
+  else if ((ac == 1) || (ac == 2))
     {
       if (databaseValid (currentDatabase))
 	{
-	  if (gnatsdChdb (databaseName (currentDatabase), av[0], av[1], 0,
+	  if (gnatsdChdb (databaseName (currentDatabase), av[0], ac == 2 ? av[1] : "", 0,
 			  &err) != 0)
 	    {
 	      print_server_errors (err);
@@ -339,14 +339,21 @@ GNATS_user (int ac, char **av)
 	      free (currentPassword);
 	    }
 	  currentUsername = xstrdup (av[0]);
-	  currentPassword = xstrdup (av[1]);
+	  if (ac == 2)
+	    {
+	      currentPassword = xstrdup (av[1]);
+	    }
+	  else
+	    {
+	      currentPassword = (char *)"";
+	    }
 	  printf ("%d Current database is not valid; use CHDB to set the database\r\n",
 		  CODE_OK);
 	}
     }
   else
     {
-      printf ("%d Need two arguments, username and password\r\n", 
+      printf ("%d Need one or two arguments, username and optionally a password\r\n", 
 	      CODE_CMD_ERROR);
     }
 }
@@ -591,15 +598,19 @@ gnatsdChdb (const char *nameOfDb, const 
 	  free (currentUsername);
 	}
       currentUsername = xstrdup (username);
-    }
-
-  if (passwd != NULL)
-    {
+      
       if (currentPassword != NULL)
 	{
 	  free (currentPassword);
 	}
-      currentPassword = xstrdup (passwd);
+      if (passwd != NULL)
+	{
+	  currentPassword = xstrdup (passwd);
+	}
+      else
+	{
+	  currentPassword = NULL;
+	}
     }
 
   if (currentUsername == NULL)
@@ -670,9 +681,9 @@ GNATS_chdb (int ac, char **av)
   const char *user = NULL;
   const char *passwd = NULL;
 
-  if (ac != 1 && ac != 3)
+  if (ac != 1 && ac != 2 && ac != 3)
     {
-      printf ("%d One or three arguments required.\r\n", CODE_CMD_ERROR); 
+      printf ("%d One, two, or three arguments required.\r\n", CODE_CMD_ERROR); 
       return;
     }
 
@@ -681,6 +692,10 @@ GNATS_chdb (int ac, char **av)
       user = av[1];
       passwd = av[2];
     }
+  else if (ac == 2)
+    {
+      user = av[1];
+    }
 
   if (gnatsdChdb (av[0], user, passwd, 0, &err) != 0)
     {
@@ -1786,11 +1801,11 @@ GNATS_help (int ac ATTRIBUTE_UNUSED, cha
  	  CODE_INFORMATION);
   printf ("%d-   SUBM                    submit a new PR\r\n",
 	  CODE_INFORMATION);
-  printf ("%d-   CHDB <database> [<user> <passwd>]\r\n",
+  printf ("%d-   CHDB <database> [<user> [<passwd>]]\r\n",
 	  CODE_INFORMATION);
   printf ("%d-                           change GNATS ROOT to <database>\r\n",
 	  CODE_INFORMATION);
-  printf ("%d-   USER <name> <passwd>    Sets the current user\r\n",
+  printf ("%d-   USER <name> [<passwd>]  Sets the current user\r\n",
 	  CODE_INFORMATION);
   printf ("%d-   USER                    Report current access level\r\n",
 	  CODE_INFORMATION);
Index: gnats/gnatsd.access
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/gnatsd.access,v
retrieving revision 1.5
diff -u -p -r1.5 gnatsd.access
--- gnats/gnatsd.access	16 Oct 2001 15:06:56 -0000	1.5
+++ gnats/gnatsd.access	4 Oct 2002 17:04:57 -0000
@@ -13,10 +13,12 @@
 #
 #       * userid:   a user id to gain access to gnatsd
 #       * password: a password for the user.  Passwords prefixed by $0$ are
-#               assumed to be plain-text.  Passwords without a prefix are
-#               assumed to be encrypted with standard crypt(), while passwords
-#               prefixed with $1$ are assumed to be MD5 encrypted.
-#               MD5 and crypt() encryption may not be available on all systems.
+#               assumed to be plain-text.  Passwords without a $0$ prefix are
+#               assumed to be encrypted with standard crypt(). For example, on
+#		FreeBSD systems, no prefix implies DES encryption, a prefix of $1$
+#		implies MD5 encryption, while $2$ prefix implies Blowfish encryption.
+#		An empty field value means that the user should not supply any
+#		password.
 #       * access-level: (default = edit)
 #               deny     - gnatsd closes the connection
 #               none     - no further access until userid and password given
@@ -33,4 +35,4 @@
 #         It's ignored in gnatsd-adm/gnatsd.access since this file is already
 #         database specific.
 #
-#*:*:view:
+#*::view:
Index: gnats/gnatsd.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/gnatsd.c,v
retrieving revision 1.47
diff -u -p -r1.47 gnatsd.c
--- gnats/gnatsd.c	4 Aug 2002 10:58:29 -0000	1.47
+++ gnats/gnatsd.c	4 Oct 2002 17:04:57 -0000
@@ -253,21 +253,35 @@ match (const char *line, const char *pat
 static int
 password_match (const char *password, const char *hash)
 {
-  if (! strncmp (hash, "$0$", 3))
+  if (strlen(password) && strlen(hash))
     {
-      /* explicit plain-text password */
-      return ! match (password, hash, TRUE);
-    }
-  else
-    {
-      /* DES crypt or MD5 hash of the password */
+      if (! strncmp (hash, "$0$", 3))
+	{
+	  /* explicit plain-text password */
+	  return match (password, hash+3, TRUE);
+	}
+      else
+	{
 #ifdef HAVE_LIBCRYPT
-      char *encrypted = crypt (password, hash);
-      return encrypted && ! strcmp (encrypted, hash);
+	  /* DES crypt or MD5 hash of the password */
+	  char *encrypted = crypt (password, hash);
+	  return encrypted && ! strcmp (encrypted, hash);
 #else
-      /* TODO: log some warning */
-      return FALSE;
+	  /* TODO: log some warning */
+	  return FALSE;
 #endif
+	}
+    }
+  else
+    {
+      if (strlen(password))
+	{
+	  return FALSE;
+	}
+      else
+	{
+	  return ! strlen(hash) ;
+	}
     }
 }
 
@@ -450,8 +464,11 @@ findUserAccessLevel (const char *file, c
 	      if (! password_match (passwd, ent->admFields[1]))
 		{
 		  /* Username matched but password didn't.  */
-		  *access = ACCESS_NONE;
-		  found = 1;
+		  if (strlen(ent->admFields[1]) && strlen(passwd))
+		    {
+		      *access = ACCESS_NONE;
+		      found = 1;
+		    }
 		}
 	      else
 		{
@@ -460,7 +477,10 @@ findUserAccessLevel (const char *file, c
 		      /* Compare all given names against the name of the
 			 requested database. */
 		      const char *l2 = ent->admFields[3];
-
+		      
+		      if (! strlen(l2))
+			found = 1;
+		      
 		      while (l2 != NULL && ! found)
 			{
 			  char *token = get_next_field (&l2, ',');

RE: Patch: Fix user authentication + MKDB

[Yngve Svendsen]

At 10:45 04.10.2002 -0700, Pankaj K Garg wrote:
> >
> > Thanks. I believe this version is OK and may be comitted.
> >
>
>Thanks. Here's the final version of the patch for both
>Gnats and Gnatsweb.
>
>I've taken care of the appropriate documentation for
>Gnats also.
>
>Can someone please commit these patches.

I hope to be able to take a good look at it next week, if no one else with 
commit rights wants to do it before that.

- Yngve



_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://mail.gnu.org/mailman/listinfo/help-gnats