buf fix of password change

ChangeLog

@@ -1,5 +1,9 @@
 2011-05-12  NIIBE Yutaka  <gniibe@fsij.org>
 
+	* src/ac.c (verify_user_0): New.
+	(verify_pso_cds, verify_admin_0): Use verify_user_0.
+	* src/openpgp.c (cmd_change_password): Use verify_user_0.
+
 	* src/random.c (get_salt): Rename from get_random.
 	(random_bytes_get, random_bytes_free): It's 16-byte.
 

NEWS

@@ -16,13 +16,16 @@ At the initialization of the card, Gnuk becomes compatible mode by
 setting PW3.  Without setting PW3, it becomes "admin-less" mode
 by setting PW1.
 
-** Important bug fix.
+** Important two bug fixes.
-Gnuk (<= 0.11) has a bug which makes possible for attacker to guess
+Gnuk (<= 0.11) had a bug which makes possible for attacker to guess
 admin password easily.  When admin password is not set (the default
 value of factory setting), failure of VERIFY doesn't increment error
 counter in older versions.  Observing no increment of error counter,
 attacker could know that admin password is the one of factory setting.
 
+Gnuk (<= 0.11) had a bug which makes possible for attacker to change
+user password without knowing original password.
+
 ** tool/gnuk_put_binary.py now uses pyscard.
 Instead of PyUSB, it uses Python binding of PC/SC.  PyUSB version is
 still available as tool/gnuk_put_binary_libusb.py.

src/ac.c

@@ -56,34 +56,67 @@ ac_reset_other (void)
   auth_status &= ~AC_OTHER_AUTHORIZED;
 }
 
-/*
- * Verify for "Perform Security Operation : Compute Digital Signature"
- */
 int
-verify_pso_cds (const uint8_t *pw, int pw_len)
+verify_user_0 (const uint8_t *pw, int buf_len, int pw_len_known,
+	       const uint8_t *ks_pw1)
 {
+  int pw_len;
   int r;
   uint8_t keystring[KEYSTRING_MD_SIZE];
 
   if (gpg_pw_locked (PW_ERR_PW1))
     return 0;
 
-  DEBUG_INFO ("verify_pso_cds\r\n");
+  if (ks_pw1 == NULL)
-  DEBUG_BYTE (pw_len);
-
-  sha1 (pw, pw_len, keystring);
-  if ((r = gpg_do_load_prvkey (GPG_KEY_FOR_SIGNING, BY_USER, keystring)) < 0)
     {
+      pw_len = strlen (OPENPGP_CARD_INITIAL_PW1);
+      if ((pw_len_known >= 0 && pw_len_known != pw_len)
+	  || buf_len < pw_len
+	  || strncmp ((const char *)pw, OPENPGP_CARD_INITIAL_PW1, pw_len))
+	goto failure;
+      else
+	goto success;
+    }
+  else
+    pw_len = ks_pw1[0];
+
+  if ((pw_len_known >= 0 && pw_len_known != pw_len)
+      || buf_len < pw_len)
+    {
+    failure:
       gpg_pw_increment_err_counter (PW_ERR_PW1);
       return -1;
     }
-  else if (r == 0)
-    /* No key is available.  Fail even if password can match.  */
-    return -1;
 
+  sha1 (pw, pw_len, keystring);
+  if ((r = gpg_do_load_prvkey (GPG_KEY_FOR_SIGNING, BY_USER, keystring))
+      < 0)
+    goto failure;
+  else if (r == 0)
+    if (memcmp (ks_pw1+1, keystring, KEYSTRING_MD_SIZE) != 0)
+      goto failure;
+
+ success:
   gpg_pw_reset_err_counter (PW_ERR_PW1);
-  auth_status |= AC_PSO_CDS_AUTHORIZED;
+  return pw_len;
-  return 1;
+}
+
+/*
+ * Verify for "Perform Security Operation : Compute Digital Signature"
+ */
+int
+verify_pso_cds (const uint8_t *pw, int pw_len)
+{
+  const uint8_t *ks_pw1 = gpg_do_read_simple (NR_DO_KEYSTRING_PW1);
+  int r;
+
+  DEBUG_INFO ("verify_pso_cds\r\n");
+  DEBUG_BYTE (pw_len);
+
+  r = verify_user_0 (pw, pw_len, pw_len, ks_pw1);
+  if (r > 0)
+    auth_status |= AC_PSO_CDS_AUTHORIZED;
+  return r;
 }
 
 int
@@ -196,43 +229,21 @@ verify_admin_0 (const uint8_t *pw, int buf_len, int pw_len_known)
       const uint8_t *ks_pw1 = gpg_do_read_simple (NR_DO_KEYSTRING_PW1);
 
       if (ks_pw1 != NULL)
-	{			/* empty PW3, but PW1 exists */
+	{	  /* empty PW3, but PW1 exists */
-	  int r;
+	  int r = verify_user_0 (pw, buf_len, pw_len_known, ks_pw1);
-	  uint8_t keystring[KEYSTRING_MD_SIZE];
 
-	  if (gpg_pw_locked (PW_ERR_PW1))
+	  if (r > 0)
-	    return 0;
+	    admin_authorized = BY_USER;
 
-	  pw_len = ks_pw1[0];
+	  return r;
-	  if ((pw_len_known >= 0 && pw_len_known != pw_len)
-	      || buf_len < pw_len)
-	    {
-	    failure_pw1:
-	      gpg_pw_increment_err_counter (PW_ERR_PW1);
-	      return -1;
-	    }
-
-	  sha1 (pw, pw_len, keystring);
-	  if ((r = gpg_do_load_prvkey (GPG_KEY_FOR_SIGNING, BY_USER, keystring))
-	      < 0)
-	    goto failure_pw1;
-	  else if (r == 0)
-	    {
-	      if (memcmp (ks_pw1+1, keystring, KEYSTRING_MD_SIZE) != 0)
-		goto failure_pw1;
-	    }
-
-	  admin_authorized = BY_USER;
-	  gpg_pw_reset_err_counter (PW_ERR_PW1);
-	  return pw_len;
 	}
 
       if (gpg_pw_locked (PW_ERR_PW3))
 	return 0;
 
       /*
-       * For the case of empty PW3 (with empty PW1 or no signing key yet),
+       * For the case of empty PW3 (with empty PW1), pass phrase
-       * pass phrase should be OPENPGP_CARD_INITIAL_PW3
+       * should be OPENPGP_CARD_INITIAL_PW3
        */
       pw_len = strlen (OPENPGP_CARD_INITIAL_PW3);
       if ((pw_len_known >=0 && pw_len_known != pw_len)
@@ -252,9 +263,9 @@ gpg_set_pw3 (const uint8_t *newpw, int newpw_len)
   uint32_t random;
 
   ks[0] = newpw_len;
-  random = get_random ();
+  random = get_salt ();
   memcpy (&ks[1], &random, sizeof (random));
-  random = get_random ();
+  random = get_salt ();
   memcpy (&ks[5], &random, sizeof (random));
   ks[9] = 0x60;			/* 65536 iterations */
 

src/openpgp-do.c

@@ -716,7 +716,7 @@ gpg_do_write_prvkey (enum kind_of_key kk, const uint8_t *key_data, int key_len,
 
   memcpy (kdi.data, key_data, KEY_CONTENT_LEN);
   kdi.check = calc_check32 (key_data, KEY_CONTENT_LEN);
-  kdi.random = get_random ();
+  kdi.random = get_salt ();
   memcpy (kdi.magic, GNUK_MAGIC, KEY_MAGIC_LEN);
 
   dek = random_bytes_get (); /* 16-byte random bytes */

src/openpgp.c

@@ -302,24 +302,24 @@ cmd_change_password (void)
 
   if (who == BY_USER)			/* PW1 */
     {
-      const uint8_t *pk = gpg_do_read_simple (NR_DO_KEYSTRING_PW1);
+      const uint8_t *ks_pw1 = gpg_do_read_simple (NR_DO_KEYSTRING_PW1);
 
-      if (pk == NULL)
+      pw_len = verify_user_0 (pw, len, -1, ks_pw1);
+
+      if (pw_len < 0)
 	{
-	  if (len < (int)strlen (OPENPGP_CARD_INITIAL_PW1))
+	  DEBUG_INFO ("permission denied.\r\n");
-	    {
+	  GPG_SECURITY_FAILURE ();
-	      DEBUG_INFO ("permission denied.\r\n");
+	  return;
-	      GPG_SECURITY_FAILURE ();
+	}
-	      return;
+      else if (pw_len == 0)
-	    }
+	{
-
+	  DEBUG_INFO ("blocked.\r\n");
-	  pw_len = strlen (OPENPGP_CARD_INITIAL_PW1);
+	  GPG_SECURITY_AUTH_BLOCKED ();
-	  newpw = pw + pw_len;
+	  return;
-	  newpw_len = len - pw_len;
 	}
       else
 	{
-	  pw_len = pk[0];
 	  newpw = pw + pw_len;
 	  newpw_len = len - pw_len;
 	}
@@ -368,7 +368,6 @@ cmd_change_password (void)
       gpg_do_write_simple (NR_DO_KEYSTRING_PW1, new_ks0, KEYSTRING_SIZE_PW1);
       ac_reset_pso_cds ();
       ac_reset_other ();
-      gpg_pw_reset_err_counter (PW_ERR_PW1);
       DEBUG_INFO ("Changed DO_KEYSTRING_PW1.\r\n");
       GPG_SUCCESS ();
     }
@@ -377,7 +376,6 @@ cmd_change_password (void)
       gpg_do_write_simple (NR_DO_KEYSTRING_PW1, new_ks0, 1);
       ac_reset_pso_cds ();
       ac_reset_other ();
-      gpg_pw_reset_err_counter (PW_ERR_PW1);
       DEBUG_INFO ("Changed length of DO_KEYSTRING_PW1.\r\n");
       GPG_SUCCESS ();
     }
@@ -385,7 +383,6 @@ cmd_change_password (void)
     {
       DEBUG_INFO ("done.\r\n");
       ac_reset_admin ();
-      gpg_pw_reset_err_counter (PW_ERR_PW3);
       GPG_SUCCESS ();
     }
 }