SA-CORE-2012-001

4 files changed, 118 insertions, 18 deletions

diff --git a/modules/openid/openid.inc b/modules/openid/openid.inc
index 98af518c7..9b793d368 100644
--- a/modules/openid/openid.inc
+++ b/modules/openid/openid.inc
@@ -617,18 +617,31 @@ function _openid_get_params($str) {
  * @param $fallback_prefix
  *   An optional prefix that will be used in case no prefix is found for the
  *   target extension namespace.
+ * @param $only_signed
+ *   Return only keys that are included in the message signature in openid.sig.
+ *   Unsigned fields may have been modified or added by other parties than the
+ *   OpenID Provider.
+ *
  * @return
  *   An associative array containing all the parameters in the response message
  *   that belong to the extension. The keys are stripped from their namespace
  *   prefix.
+ *
  * @see http://openid.net/specs/openid-authentication-2_0.html#extensions
  */
-function openid_extract_namespace($response, $extension_namespace, $fallback_prefix = NULL) {
+function openid_extract_namespace($response, $extension_namespace, $fallback_prefix = NULL, $only_signed = FALSE) {
+  $signed_keys = explode(',', $response['openid.signed']);
+
   // Find the namespace prefix.
   $prefix = $fallback_prefix;
   foreach ($response as $key => $value) {
     if ($value == $extension_namespace && preg_match('/^openid\.ns\.([^.]+)$/', $key, $matches)) {
       $prefix = $matches[1];
+      if ($only_signed && !in_array('ns.' . $matches[1], $signed_keys)) {
+        // The namespace was defined but was not signed as required. In this
+        // case we do not fall back to $fallback_prefix.
+        $prefix = NULL;
+      }
       break;
     }
   }
@@ -641,7 +654,9 @@ function openid_extract_namespace($response, $extension_namespace, $fallback_pre
   foreach ($response as $key => $value) {
     if (preg_match('/^openid\.' . $prefix . '\.(.+)$/', $key, $matches)) {
       $local_key = $matches[1];
-      $output[$local_key] = $value;
+      if (!$only_signed || in_array($prefix . '.' . $local_key, $signed_keys)) {
+        $output[$local_key] = $value;
+      }
     }
   }
 
@@ -837,8 +852,8 @@ function _openid_invalid_openid_transition($identity, $response) {
     // Try to extract e-mail address from Simple Registration (SREG) or
     // Attribute Exchanges (AX) keys.
     $email = '';
-    $sreg_values = openid_extract_namespace($response, OPENID_NS_SREG, 'sreg');
-    $ax_values = openid_extract_namespace($response, OPENID_NS_AX, 'ax');
+    $sreg_values = openid_extract_namespace($response, OPENID_NS_SREG, 'sreg', TRUE);
+    $ax_values = openid_extract_namespace($response, OPENID_NS_AX, 'ax', TRUE);
     if (!empty($sreg_values['email']) && valid_email_address($sreg_values['email'])) {
       $email = $sreg_values['email'];
     }
diff --git a/modules/openid/openid.module b/modules/openid/openid.module
index f2847fc0d..e08d55718 100644
--- a/modules/openid/openid.module
+++ b/modules/openid/openid.module
@@ -185,10 +185,15 @@ function openid_form_user_register_form_alter(&$form, &$form_state) {
 
     $response = $_SESSION['openid']['response'];
 
-    // Extract Simple Registration keys from the response.
-    $sreg_values = openid_extract_namespace($response, OPENID_NS_SREG, 'sreg');
-    // Extract Attribute Exchanges keys from the response.
-    $ax_values = openid_extract_namespace($response, OPENID_NS_AX, 'ax');
+    // Extract Simple Registration keys from the response. We only include
+    // signed keys as required by OpenID Simple Registration Extension 1.0,
+    // section 4.
+    $sreg_values = openid_extract_namespace($response, OPENID_NS_SREG, 'sreg', TRUE);
+    // Extract Attribute Exchanges keys from the response. We only include
+    // signed keys. This is not required by the specification, but it is
+    // recommended by Google, see
+    // http://googlecode.blogspot.com/2011/05/security-advisory-to-websites-using.html
+    $ax_values = openid_extract_namespace($response, OPENID_NS_AX, 'ax', TRUE);
 
     if (!empty($sreg_values['nickname'])) {
       // Use the nickname returned by Simple Registration if available.
diff --git a/modules/openid/openid.test b/modules/openid/openid.test
index afb9068c6..9b6b1ad5f 100644
--- a/modules/openid/openid.test
+++ b/modules/openid/openid.test
@@ -343,17 +343,49 @@ class OpenIDFunctionalTestCase extends OpenIDWebTestCase {
     // Use a User-supplied Identity that is the URL of an XRDS document.
     $identity = url('openid-test/yadis/xrds', array('absolute' => TRUE));
 
-    // Do not sign all mandatory fields (e.g. assoc_handle).
+    // Respond with an invalid signature.
+    variable_set('openid_test_response', array('openid.sig' => 'this-is-an-invalid-signature'));
+    $this->submitLoginForm($identity);
+    $this->assertRaw('OpenID login failed.');
+
+    // Do not sign the mandatory field openid.assoc_handle.
     variable_set('openid_test_response', array('openid.signed' => 'op_endpoint,claimed_id,identity,return_to,response_nonce'));
     $this->submitLoginForm($identity);
     $this->assertRaw('OpenID login failed.');
 
-    // Sign all mandatory fields and some custom fields.
-    variable_set('openid_test_response', array('openid.foo' => 'bar', 'openid.signed' => 'op_endpoint,claimed_id,identity,return_to,response_nonce,assoc_handle,foo'));
+    // Sign all mandatory fields and a custom field.
+    $keys_to_sign = array('op_endpoint', 'claimed_id', 'identity', 'return_to', 'response_nonce', 'assoc_handle', 'foo');
+    $association = new stdClass();
+    $association->mac_key = variable_get('mac_key');
+    $response = array(
+      'openid.op_endpoint' => url('openid-test/endpoint', array('absolute' => TRUE)),
+      'openid.claimed_id' => $identity,
+      'openid.identity' => $identity,
+      'openid.return_to' => url('openid/authenticate', array('absolute' => TRUE)),
+      'openid.response_nonce' => _openid_nonce(),
+      'openid.assoc_handle' => 'openid-test',
+      'openid.foo' => 123,
+      'openid.signed' => implode(',', $keys_to_sign),
+    );
+    $response['openid.sig'] = _openid_signature($association, $response, $keys_to_sign);
+    variable_set('openid_test_response', $response);
     $this->submitLoginForm($identity);
     $this->assertNoRaw('OpenID login failed.');
-  }
+    $this->assertFieldByName('name', '', t('No username was supplied by provider.'));
+    $this->assertFieldByName('mail', '', t('No e-mail address was supplied by provider.'));
 
+    // Check that unsigned SREG fields are ignored.
+    $response = array(
+      'openid.signed' => 'op_endpoint,claimed_id,identity,return_to,response_nonce,assoc_handle,sreg.nickname',
+      'openid.sreg.nickname' => 'john',
+      'openid.sreg.email' => 'john@example.com',
+    );
+    variable_set('openid_test_response', $response);
+    $this->submitLoginForm($identity);
+    $this->assertNoRaw('OpenID login failed.');
+    $this->assertFieldByName('name', 'john', t('Username was supplied by provider.'));
+    $this->assertFieldByName('mail', '', t('E-mail address supplied by provider was ignored.'));
+  }
 }
 
 /**
@@ -728,4 +760,41 @@ class OpenIDUnitTest extends DrupalWebTestCase {
 
     $this->assertEqual(openid_normalize('http://example.com/path#fragment'), 'http://example.com/path', t('openid_normalize() correctly normalized a URL with a fragment.'));
   }
+
+  /**
+   * Test openid_extract_namespace().
+   */
+  function testOpenidExtractNamespace() {
+    $response = array(
+      'openid.sreg.nickname' => 'john',
+      'openid.ns.ext1' => OPENID_NS_SREG,
+      'openid.ext1.nickname' => 'george',
+      'openid.ext1.email' => 'george@example.com',
+      'openid.ns.ext2' => 'http://example.com/ns/ext2',
+      'openid.ext2.foo' => '123',
+      'openid.ext2.bar' => '456',
+      'openid.signed' => 'sreg.nickname,ns.ext1,ext1.email,ext2.foo',
+    );
+
+    $values = openid_extract_namespace($response, 'http://example.com/ns/dummy', NULL, FALSE);
+    $this->assertEqual($values, array(), t('Nothing found for unused namespace.'));
+
+    $values = openid_extract_namespace($response, 'http://example.com/ns/dummy', 'sreg', FALSE);
+    $this->assertEqual($values, array('nickname' => 'john'), t('Value found for fallback prefix.'));
+
+    $values = openid_extract_namespace($response, OPENID_NS_SREG, 'sreg', FALSE);
+    $this->assertEqual($values, array('nickname' => 'george', 'email' => 'george@example.com'), t('Namespace takes precedence over fallback prefix.'));
+
+    // ext1.email is signed, but ext1.nickname is not.
+    $values = openid_extract_namespace($response, OPENID_NS_SREG, 'sreg', TRUE);
+    $this->assertEqual($values, array('email' => 'george@example.com'), t('Unsigned namespaced fields ignored.'));
+
+    $values = openid_extract_namespace($response, 'http://example.com/ns/ext2', 'sreg', FALSE);
+    $this->assertEqual($values, array('foo' => '123', 'bar' => '456'), t('Unsigned fields found.'));
+
+    // ext2.foo and ext2.bar are ignored, because ns.ext2 is not signed. The
+    // fallback prefix is not used, because the namespace is specified.
+    $values = openid_extract_namespace($response, 'http://example.com/ns/ext2', 'sreg', TRUE);
+    $this->assertEqual($values, array(), t('Unsigned fields ignored.'));
+  }
 }
diff --git a/modules/openid/tests/openid_test.module b/modules/openid/tests/openid_test.module
index 629dcd335..1b0de4ec5 100644
--- a/modules/openid/tests/openid_test.module
+++ b/modules/openid/tests/openid_test.module
@@ -324,9 +324,7 @@ function _openid_test_endpoint_authenticate() {
   // Generate unique identifier for this authentication.
   $nonce = _openid_nonce();
 
-  // Generate response containing the user's identity. The openid.sreg.xxx
-  // entries contain profile data stored by the OpenID Provider (see OpenID
-  // Simple Registration Extension 1.0).
+  // Generate response containing the user's identity.
   $response = variable_get('openid_test_response', array()) + array(
     'openid.ns' => OPENID_NS_2_0,
     'openid.mode' => 'id_res',
@@ -336,14 +334,27 @@ function _openid_test_endpoint_authenticate() {
     'openid.return_to' => $_REQUEST['openid_return_to'],
     'openid.response_nonce' => $nonce,
     'openid.assoc_handle' => 'openid-test',
-    'openid.signed' => 'op_endpoint,claimed_id,identity,return_to,response_nonce,assoc_handle',
   );
 
+  if (isset($response['openid.signed'])) {
+    $keys_to_sign = explode(',', $response['openid.signed']);
+  }
+  else {
+    // Unless openid.signed is explicitly defined, all keys are signed.
+    $keys_to_sign = array();
+    foreach ($response as $key => $value) {
+      // Strip off the "openid." prefix.
+      $keys_to_sign[] = substr($key, 7);
+    }
+    $response['openid.signed'] = implode(',', $keys_to_sign);
+  }
+
   // Sign the message using the MAC key that was exchanged during association.
   $association = new stdClass();
   $association->mac_key = variable_get('mac_key');
-  $keys_to_sign = explode(',', $response['openid.signed']);
-  $response['openid.sig'] = _openid_signature($association, $response, $keys_to_sign);
+  if (!isset($response['openid.sig'])) {
+    $response['openid.sig'] = _openid_signature($association, $response, $keys_to_sign);
+  }
 
   // Put the signed message into the query string of a URL supplied by the
   // Relying Party, and redirect the user.