Issue #3406766 by mcdruid: Upgrade to use V3 of the HIBP API, enable Add-Padding

password_haveibeenpwned.admin.inc

@@ -19,7 +19,12 @@ function password_haveibeenpwned_admin_settings($form, &$form_state) {
   $hibp_api = array(
     '!hibp_api' => l(
     t('HIBP API'),
-    'https://haveibeenpwned.com/API/v2#PwnedPasswords'),
+    'https://haveibeenpwned.com/API/v3#PwnedPasswords'),
+  );
+  $hibp_padding = array(
+    '!hibp_padding' => l(
+      t('HIBP documentation on padding'),
+      'https://www.troyhunt.com/enhancing-pwned-passwords-privacy-with-padding/'),
   );
 
   $form['policy_wrapper'] = array(
@@ -145,9 +150,15 @@ function password_haveibeenpwned_admin_settings($form, &$form_state) {
   $form['api_settings']['password_haveibeenpwned_failed_check_default'] = array(
     '#type'          => 'checkbox',
     '#title'         => t('Assume password is bad if check fails'),
-    '#description'   => 'If a call to the HIBP API fails, should the module treat the password as if it is compromised? (Default: no)',
+    '#description'   => t('If a call to the HIBP API fails, should the module treat the password as if it is compromised? (Default: no)'),
     '#default_value' => variable_get('password_haveibeenpwned_failed_check_default', PASSWORD_HAVEIBEENPWNED_FAILED_CHECK_DEFAULT),
   );
+  $form['api_settings']['password_haveibeenpwned_add_padding'] = array(
+    '#type'          => 'checkbox',
+    '#title'         => t('Ask HIBP API to Add Padding'),
+    '#description'   => t('See: !hibp_padding (Default: yes)', $hibp_padding),
+    '#default_value' => variable_get('password_haveibeenpwned_add_padding', PASSWORD_HAVEIBEENPWNED_PADDING_DEFAULT),
+  );
 
   return system_settings_form($form);
 }

password_haveibeenpwned.module

@@ -24,6 +24,9 @@ const PASSWORD_HAVEIBEENPWNED_DEFAULT_PASSWORD_THRESHOLD = 10;
 // If the password given at login is incorrect, check it in HIBP?
 const PASSWORD_HAVEIBEENPWNED_CHECK_FAILED_LOGIN = FALSE;
 
+// Ask the HIBP API to add padding?
+const PASSWORD_HAVEIBEENPWNED_PADDING_DEFAULT = TRUE;
+
 // Settings for policies.
 const PASSWORD_HAVEIBEENPWNED_IGNORE = 0;
 const PASSWORD_HAVEIBEENPWNED_WARN = 1;
@@ -67,6 +70,7 @@ function password_haveibeenpwned_check_password($password, $return_count = FALSE
   $timeout = variable_get('password_haveibeenpwned_timeout', PASSWORD_HAVEIBEENPWNED_DEFAULT_TIMEOUT);
   $failed_check_default = variable_get('password_haveibeenpwned_failed_check_default', PASSWORD_HAVEIBEENPWNED_FAILED_CHECK_DEFAULT);
   $password_threshold = variable_get('password_haveibeenpwned_password_threshold', PASSWORD_HAVEIBEENPWNED_DEFAULT_PASSWORD_THRESHOLD);
+  $hibp_padding = variable_get('password_haveibeenpwned_add_padding', PASSWORD_HAVEIBEENPWNED_PADDING_DEFAULT) ? 'true' : 'false';
 
   $result = drupal_http_request($hibp_api_url . $hash_prefix,
     array(
@@ -74,6 +78,7 @@ function password_haveibeenpwned_check_password($password, $return_count = FALSE
       'timeout' => $timeout,
       'headers' => array(
         'User-Agent' => $ua,
+        'Add-Padding' => $hibp_padding,
       ),
     )
   );
@@ -85,8 +90,7 @@ function password_haveibeenpwned_check_password($password, $return_count = FALSE
     $message .= 'Details: @error_details';
 
     $details = $result->error;
-    // @todo: is this necessary now that we only send the first 5 chars?
-    // Ensure that we do not log actual hashes.
+    // Ensure that we never log the actual password hash.
     $search = array($hash, "\n");
     $replace = array('**PASSWORD-HASH-REDACTED**', ' ');
     $details = str_replace($search, $replace, $details);
@@ -105,9 +109,13 @@ function password_haveibeenpwned_check_password($password, $return_count = FALSE
     case 200;
       $body = (string) $result->data;
       $lines = explode("\r\n", $body);
+      if ($hibp_padding === 'true') {
+        $lines = array_filter($lines, '_password_haveibeenpwned_filter_padding');
+      }
       foreach ($lines as $line) {
+        // phpcs:ignore SlevomatCodingStandard.PHP.ShortList
         list($api_hash_suffix, $count) = explode(':', $line);
-        if ($hash_suffix == $api_hash_suffix) {
+        if ($hash_suffix === $api_hash_suffix) {
           if ($return_count) {
             return $count;
           }
@@ -121,15 +129,26 @@ function password_haveibeenpwned_check_password($password, $return_count = FALSE
     case 429:
       // Rate limit has been exceeded.
       watchdog('password_haveibeenpwned', 'HTTP status 429 returned; API calls hit rate limit.', array(), WATCHDOG_INFO);
-      // Deliberately no break.
+      $status = $failed_check_default;
+      break;
+
     default:
-      // @todo: Log http errors (e.g. 401, 403, 50x); no exceptions are thrown.
+      watchdog('password_haveibeenpwned', 'HTTP status @code returned.', array('@code' => $result->code), WATCHDOG_INFO);
       $status = $failed_check_default;
       break;
   }
   return $status;
 }
 
+/**
+ * Callback to filter out padding results from the HIBP API.
+ *
+ * @see: https://www.troyhunt.com/enhancing-pwned-passwords-privacy-with-padding/
+ */
+function _password_haveibeenpwned_filter_padding($entry) {
+  return substr($entry, -2) !== ':0';
+}
+
 /**
  * Implements hook_form_alter() for user_login_form and user_login_block.
  */

tests/password_haveibeenpwned.test

@@ -10,6 +10,11 @@
  */
 class PasswordHaveIBeenPwnedTestCase extends DrupalWebTestCase {
 
+  /**
+   * An authenticated user.
+   *
+   * @var object
+   */
   protected $authUser;
 
   /**
@@ -39,7 +44,7 @@ class PasswordHaveIBeenPwnedTestCase extends DrupalWebTestCase {
     );
 
     // Switch HIBP API to use the mock endpoint.
-    variable_set('password_haveibeenpwned_api_url', $base_url . '/password_haveibeenpwned_test/mock_api/');
+    variable_set('password_haveibeenpwned_api_url', url('password_haveibeenpwned_test/mock_api/', array('absolute' => TRUE)));
 
     $this->authUser = $this->drupalCreateUser(
       array(
@@ -444,3 +449,29 @@ class PasswordHaveIBeenPwnedTestCase extends DrupalWebTestCase {
   }
 
 }
+
+/**
+ * Provides another test class and helper methods for automated tests.
+ */
+class PasswordHaveIBeenPwnedNoPaddingTestCase extends PasswordHaveIBeenPwnedTestCase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getInfo() {
+    return array(
+      'name' => 'Password Have I Been Pwned? no padding',
+      'description' => 'Tests Password Have I Been Pwned? module without padding enabled',
+      'group' => 'Password Have I Been Pwned?',
+    );
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function setUp() {
+    parent::setUp();
+    variable_set('password_haveibeenpwned_add_padding', FALSE);
+  }
+
+}

tests/password_haveibeenpwned_test.module

@@ -5,7 +5,7 @@
  * Password Have I Been Pwned? Tests.
  */
 
-const HIBP_API_NEWLINE = "\r\n";
+const PASSWORD_HAVEIBEENPWNED_TEST_HIBP_API_NEWLINE = "\r\n";
 
 /**
  * Implements hook_menu().
@@ -27,41 +27,52 @@ function password_haveibeenpwned_test_menu() {
  * Page callback for mock HIBP API endpoint.
  */
 function password_haveibeenpwned_test_mock_api($partial_hash) {
+
+  $padding = isset($_SERVER['HTTP_ADD_PADDING']) && strtolower($_SERVER['HTTP_ADD_PADDING']) === 'true';
+
   switch ($partial_hash) {
     case 'AB87D':
       $password = 'monkey';
       $suffix = substr(strtoupper(hash('sha1', ($password))), 5);
-      $suffix .= ':980209' . HIBP_API_NEWLINE;
+      $suffix .= ':980209' . PASSWORD_HAVEIBEENPWNED_TEST_HIBP_API_NEWLINE;
       break;
 
     case 'B1B37':
       $password = 'qwerty';
       $suffix = substr(strtoupper(hash('sha1', ($password))), 5);
-      $suffix .= ':3810555' . HIBP_API_NEWLINE;
+      $suffix .= ':3810555' . PASSWORD_HAVEIBEENPWNED_TEST_HIBP_API_NEWLINE;
       break;
 
     case 'CC915':
       $password = 'drupal';
       $suffix = substr(strtoupper(hash('sha1', ($password))), 5);
-      $suffix .= ':55' . HIBP_API_NEWLINE;
+      $suffix .= ':55' . PASSWORD_HAVEIBEENPWNED_TEST_HIBP_API_NEWLINE;
       break;
 
     default:
       $suffix = '';
   }
   $before = $after = _password_haveibeenpwned_test_hash_suffixes(100);
-  print trim($before . $suffix . $after);
+
+  $padding_hashes = '';
+  if ($padding) {
+    // The random number does not need to be cryptographically secure.
+    $padding_hashes = _password_haveibeenpwned_test_hash_suffixes(rand(599, 799), TRUE);
+  }
+
+  print trim($before . $suffix . $after . $padding_hashes);
   drupal_exit();
 }
 
 /**
  * Helper to generate hash suffixes for mock API responses.
  */
-function _password_haveibeenpwned_test_hash_suffixes($count) {
+function _password_haveibeenpwned_test_hash_suffixes($count, $zero = FALSE) {
   $suffixes = '';
   while ($count > 0) {
     $hash = substr(strtoupper(hash('sha1', ($count))), 5);
-    $suffixes .= $hash . ':' . $count . HIBP_API_NEWLINE;
+    $num = $zero ? 0 : $count;
+    $suffixes .= $hash . ':' . $num . PASSWORD_HAVEIBEENPWNED_TEST_HIBP_API_NEWLINE;
     $count--;
   }
   return $suffixes;