Permalink
Browse files

Merge pull request #57 from kilotaras/vulnerability_bug

Fixed a vulnerability with signed requests
  • Loading branch information...
2 parents aecd4bf + 7324e40 commit bf99924386be074da30a8e9d6bbcb49d3333da12 @oyvindkinsey oyvindkinsey committed Jan 15, 2013
Showing with 52 additions and 4 deletions.
  1. +5 −0 src/base_facebook.php
  2. +47 −4 tests/tests.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->getPersistentData('user_id')){
+ $this->clearAllPersistentData();
@Vittel
Vittel Sep 2, 2013

this makes it somehow impossible to retrieve a extended token and also retrieve a user id in one step, since the extended token deletes all persisted data as well as retrieveing the user id delets em.

+ }
+
$this->setPersistentData('user_id', $signed_request['user_id']);
return $user;
}
View
@@ -22,18 +22,20 @@ class PHPSDKTestCase extends PHPUnit_Framework_TestCase {
const MIGRATED_APP_ID = '174236045938435';
const MIGRATED_SECRET = '0073dce2d95c4a5c2922d1827ea0cca6';
- const TEST_USER = 499834690;
+ const TEST_USER = 499834690;
+ const TEST_USER_2 = 499835484;
private static $kExpiredAccessToken = 'AAABrFmeaJjgBAIshbq5ZBqZBICsmveZCZBi6O4w9HSTkFI73VMtmkL9jLuWsZBZC9QMHvJFtSulZAqonZBRIByzGooCZC8DWr0t1M4BL9FARdQwPWPnIqCiFQ';
- private static function kValidSignedRequest() {
+ private static function kValidSignedRequest($id = self::TEST_USER, $oauth_token = null) {
$facebook = new FBPublic(array(
'appId' => self::APP_ID,
'secret' => self::SECRET,
));
return $facebook->publicMakeSignedRequest(
array(
- 'user_id' => self::TEST_USER,
+ 'user_id' => $id,
+ 'oauth_token' => $oauth_token
)
);
}
@@ -321,7 +323,6 @@ public function testGetCodeWithMissingCSRFState() {
// intentionally don't set CSRF token at all
$this->assertFalse($facebook->publicGetCode(),
'Expect getCode to fail, CSRF state not sent back.');
-
}
public function testGetUserFromSignedRequest() {
@@ -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(self::TEST_USER, $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(self::TEST_USER_2, $facebook->getUser(),
+ 'Failed to get user ID from a valid signed request.');
+
+ $_REQUEST['signed_request'] = null;
+ $facebook ->uncacheSignedRequest();
+
+ $this->assertNotEquals('Hello sweetie', $facebook->getAccessToken(),
+ 'Failed to clear access token');
+ }
+
public function testGetSignedRequestFromCookie() {
$facebook = new FBPublicCookie(array(
'appId' => self::APP_ID,
@@ -1969,6 +1998,20 @@ public function publicGetMetadataCookieName() {
}
}
+class FBRewrite extends Facebook{
+
+ public function uncacheSignedRequest(){
+ $this->signedRequest = null;
+ }
+
+ public function uncache()
+ {
+ $this->user = null;
+ $this->signedRequest = null;
+ $this->accessToken = null;
+ }
+}
+
class FBPublicGetAccessTokenFromCode extends TransientFacebook {
public function publicGetAccessTokenFromCode($code, $redirect_uri = null) {

0 comments on commit bf99924

Please sign in to comment.