Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixing a vulnerability with signed_requests #56

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

kilotaras commented Jan 11, 2013

See the following use case

  1. Alice is logged into Facebook
  2. Alice visits example.com/with_js_sdk.php (our PHP SDK example endpoint). The SDK will write her signed_request, user_id, and access_token to a session cookie.
  3. Alice is then tricked into visiting example.com/with_js_sdk.php?signed_request={Bob's signed request}. The SDK will then set the user_id to Bob, while keeping Alice's access_token in persistent storage
  4. If example.com has another endpoint that authenticates based on getUser(), Bob now has access to data returned by Alice's access_token.

This fixes it.

@oyvindkinsey oyvindkinsey commented on the diff Jan 14, 2013

tests/tests.php
@@ -335,6 +336,34 @@ public function testGetUserFromSignedRequest() {
'Failed to get user ID from a valid signed request.');
}
+ public function testSignedRequestRewrite(){
+ $facebook = new FBRewrite(array(
+ 'appId' => self::APP_ID,
+ 'secret' => self::SECRET,
+ ));
+
+ $_REQUEST['signed_request'] = self::kValidSignedRequest(self::TEST_USER, 'Hello sweetie');
+
+ $this->assertEquals('499834690', $facebook->getUser(),
@oyvindkinsey

oyvindkinsey Jan 14, 2013

Contributor

self::TEST_USER

@oyvindkinsey oyvindkinsey commented on the diff Jan 14, 2013

tests/tests.php
+ 'secret' => self::SECRET,
+ ));
+
+ $_REQUEST['signed_request'] = self::kValidSignedRequest(self::TEST_USER, 'Hello sweetie');
+
+ $this->assertEquals('499834690', $facebook->getUser(),
+ 'Failed to get user ID from a valid signed request.');
+
+ $this->assertEquals('Hello sweetie', $facebook->getAccessToken(),
+ 'Failed to get access token from signed request');
+
+ $facebook->uncache();
+
+ $_REQUEST['signed_request'] = self::kValidSignedRequest(self::TEST_USER_2, 'spoilers');
+
+ $this->assertEquals('499835484', $facebook->getUser(),
@oyvindkinsey

oyvindkinsey Jan 14, 2013

Contributor

self::TEST_USER_2

@oyvindkinsey oyvindkinsey commented on the diff Jan 14, 2013

tests/tests.php
+
+ $this->assertEquals('499834690', $facebook->getUser(),
+ 'Failed to get user ID from a valid signed request.');
+
+ $this->assertEquals('Hello sweetie', $facebook->getAccessToken(),
+ 'Failed to get access token from signed request');
+
+ $facebook->uncache();
+
+ $_REQUEST['signed_request'] = self::kValidSignedRequest(self::TEST_USER_2, 'spoilers');
+
+ $this->assertEquals('499835484', $facebook->getUser(),
+ 'Failed to get user ID from a valid signed request.');
+
+ $this->assertNotEquals('Hello sweetie', $facebook->getAccessToken(),
+ 'Failed to get access token from signed request');
@oyvindkinsey

oyvindkinsey Jan 14, 2013

Contributor

This is redundant due to the next test.

Contributor

oyvindkinsey commented Jan 14, 2013

Can you squash these two commits?

And take a look a the inline comments - are you sure the test case actually use persistant storage? You can easily try that by making the test fail first, and then apply your fix.

@oyvindkinsey oyvindkinsey commented on the diff Jan 14, 2013

src/base_facebook.php
@@ -529,6 +529,11 @@ protected function getUserFromAvailableData() {
if ($signed_request) {
if (array_key_exists('user_id', $signed_request)) {
$user = $signed_request['user_id'];
+
+ if($user != $this->user || $user != $this->getPersistentData('user_id')){
@oyvindkinsey

oyvindkinsey Jan 14, 2013

Contributor

If $this->user is set, then you will never end up in this codepath, so only checking the second conditional should be enough, or you end up clearing even when there is nothing to clear.

Contributor

kilotaras commented Jan 14, 2013

A test case is actually a little more complicated and tbh.

It doesn't look like pull request can be changed (only adding commits is possible). Just reject this one and I'll pull request from other branch.

@kilotaras kilotaras closed this Jan 14, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment