Skip to content
This repository was archived by the owner on Jan 13, 2022. It is now read-only.

Commit 70fd253

Browse files
SammyKSammyK
committed
Made peer verification more secure
1 parent 8d04d00 commit 70fd253

12 files changed

+190
-4011
lines changed

‎src/Facebook/HttpClients/FacebookCurlHttpClient.php

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -146,12 +146,6 @@ public function send($url, $method = 'GET', $parameters = array())
146146
$this->openConnection($url, $method, $parameters);
147147
$this->tryToSendRequest();
148148

149-
// Need to verify the peer
150-
if ($this->curlErrorCode == 60 || $this->curlErrorCode == 77) {
151-
$this->addBundledCert();
152-
$this->tryToSendRequest();
153-
}
154-
155149
if ($this->curlErrorCode) {
156150
throw new FacebookSDKException($this->curlErrorMessage, $this->curlErrorCode);
157151
}
@@ -181,6 +175,9 @@ public function openConnection($url, $method = 'GET', $parameters = array())
181175
CURLOPT_TIMEOUT => 60,
182176
CURLOPT_RETURNTRANSFER => true, // Follow 301 redirects
183177
CURLOPT_HEADER => true, // Enable header processing
178+
CURLOPT_SSL_VERIFYHOST => 2,
179+
CURLOPT_SSL_VERIFYPEER => true,
180+
CURLOPT_CAINFO => __DIR__ . '/certs/DigiCertHighAssuranceEVRootCA.pem',
184181
);
185182

186183
if ($method !== "GET") {
@@ -202,15 +199,6 @@ public function openConnection($url, $method = 'GET', $parameters = array())
202199
self::$facebookCurl->setopt_array($options);
203200
}
204201

205-
/**
206-
* Add a bundled cert to the connection
207-
*/
208-
public function addBundledCert()
209-
{
210-
self::$facebookCurl->setopt(CURLOPT_CAINFO,
211-
dirname(__FILE__) . DIRECTORY_SEPARATOR . 'fb_ca_chain_bundle.crt');
212-
}
213-
214202
/**
215203
* Closes an existing curl connection
216204
*/

‎src/Facebook/HttpClients/FacebookGuzzleHttpClient.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,8 @@ public function send($url, $method = 'GET', $parameters = array())
108108
$options = array('body' => $parameters);
109109
}
110110

111+
$options['verify'] = __DIR__ . '/certs/DigiCertHighAssuranceEVRootCA.pem';
112+
111113
$request = self::$guzzleClient->createRequest($method, $url, $options);
112114

113115
foreach($this->requestHeaders as $k => $v) {

‎src/Facebook/HttpClients/FacebookStreamHttpClient.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,9 @@ public function send($url, $method = 'GET', $parameters = array())
107107
),
108108
'ssl' => array(
109109
'verify_peer' => true,
110-
'cafile' => dirname(__FILE__) . DIRECTORY_SEPARATOR . 'fb_ca_chain_bundle.crt',
110+
'verify_peer_name' => true,
111+
'allow_self_signed' => true, // All root certificates are self-signed
112+
'cafile' => __DIR__ . '/certs/DigiCertHighAssuranceEVRootCA.pem',
111113
),
112114
);
113115

‎src/Facebook/HttpClients/certs/DigiCertHighAssuranceEVRootCA.pem

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
-----BEGIN CERTIFICATE-----
2+
MIIDxTCCAq2gAwIBAgIQAqxcJmoLQJuPC3nyrkYldzANBgkqhkiG9w0BAQUFADBs
3+
MQswCQYDVQQGEwJVUzEVMBMGA1UEChMMRGlnaUNlcnQgSW5jMRkwFwYDVQQLExB3
4+
d3cuZGlnaWNlcnQuY29tMSswKQYDVQQDEyJEaWdpQ2VydCBIaWdoIEFzc3VyYW5j
5+
ZSBFViBSb290IENBMB4XDTA2MTExMDAwMDAwMFoXDTMxMTExMDAwMDAwMFowbDEL
6+
MAkGA1UEBhMCVVMxFTATBgNVBAoTDERpZ2lDZXJ0IEluYzEZMBcGA1UECxMQd3d3
7+
LmRpZ2ljZXJ0LmNvbTErMCkGA1UEAxMiRGlnaUNlcnQgSGlnaCBBc3N1cmFuY2Ug
8+
RVYgUm9vdCBDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAMbM5XPm
9+
+9S75S0tMqbf5YE/yc0lSbZxKsPVlDRnogocsF9ppkCxxLeyj9CYpKlBWTrT3JTW
10+
PNt0OKRKzE0lgvdKpVMSOO7zSW1xkX5jtqumX8OkhPhPYlG++MXs2ziS4wblCJEM
11+
xChBVfvLWokVfnHoNb9Ncgk9vjo4UFt3MRuNs8ckRZqnrG0AFFoEt7oT61EKmEFB
12+
Ik5lYYeBQVCmeVyJ3hlKV9Uu5l0cUyx+mM0aBhakaHPQNAQTXKFx01p8VdteZOE3
13+
hzBWBOURtCmAEvF5OYiiAhF8J2a3iLd48soKqDirCmTCv2ZdlYTBoSUeh10aUAsg
14+
EsxBu24LUTi4S8sCAwEAAaNjMGEwDgYDVR0PAQH/BAQDAgGGMA8GA1UdEwEB/wQF
15+
MAMBAf8wHQYDVR0OBBYEFLE+w2kD+L9HAdSYJhoIAu9jZCvDMB8GA1UdIwQYMBaA
16+
FLE+w2kD+L9HAdSYJhoIAu9jZCvDMA0GCSqGSIb3DQEBBQUAA4IBAQAcGgaX3Nec
17+
nzyIZgYIVyHbIUf4KmeqvxgydkAQV8GK83rZEWWONfqe/EW1ntlMMUu4kehDLI6z
18+
eM7b41N5cdblIZQB2lWHmiRk9opmzN6cN82oNLFpmyPInngiK3BD41VHMWEZ71jF
19+
hS9OMPagMRYjyOfiZRYzy78aG6A9+MpeizGLYAiJLQwGXFK3xPkKmNEVX58Svnw2
20+
Yzi9RKR/5CYrCsSXaQ3pjOLAEFe4yHYSkVXySGnYvCoCWw9E1CAx2/S6cCZdkGCe
21+
vEsXCS+0yx5DaMkHJ8HSXPfqIbloEpw8nL+e/IBcm2PN7EeqJSdnoDfzAIJ9VNep
22+
+OkuE6N36B9K
23+
-----END CERTIFICATE-----

‎src/Facebook/HttpClients/fb_ca_chain_bundle.crt

Lines changed: 0 additions & 3920 deletions
This file was deleted.

‎tests/FacebookPageTabHelperTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ class FacebookPageTabHelperTest extends PHPUnit_Framework_TestCase
99

1010
public function testPageDataCanBeAccessed()
1111
{
12-
$_GET['signed_request'] = $this->rawSignedRequestAuthorized;
12+
$_POST['signed_request'] = $this->rawSignedRequestAuthorized;
1313
$helper = new FacebookPageTabHelper('123', 'foo_app_secret');
1414

1515
$this->assertTrue($helper->isLiked());

‎tests/FacebookRedirectLoginHelperTest.php

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,10 @@ public function testLoginURL()
2525
'sdk' => 'php-sdk-' . FacebookRequest::VERSION,
2626
'scope' => implode(',', array())
2727
);
28-
$expectedUrl = 'https://www.facebook.com/v2.0/dialog/oauth?';
29-
$this->assertTrue(strpos($loginUrl, $expectedUrl) !== false);
28+
$expectedUrl = 'https://www.facebook.com/' . FacebookRequest::GRAPH_API_VERSION . '/dialog/oauth?';
29+
$this->assertTrue(strpos($loginUrl, $expectedUrl) === 0, 'Unexpected base login URL returned from getLoginUrl().');
3030
foreach ($params as $key => $value) {
31-
$this->assertTrue(
32-
strpos($loginUrl, $key . '=' . urlencode($value)) !== false
33-
);
31+
$this->assertContains($key . '=' . urlencode($value), $loginUrl);
3432
}
3533
}
3634

‎tests/FacebookRequestTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ public function testCanPostAndDelete()
4141
'DELETE',
4242
'/' . $user_id
4343
))->execute()->getGraphObject()->asArray();
44-
$this->assertTrue($response);
44+
$this->assertEquals(['success' => true], $response);
4545
}
4646

4747
public function testETagHit()

‎tests/HttpClients/FacebookCurlHttpClientTest.php

Lines changed: 126 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,29 @@ public function testCanOpenGetCurlConnection()
3434
->andReturn(null);
3535
$this->curlMock
3636
->shouldReceive('setopt_array')
37-
->with(array(
38-
CURLOPT_URL => 'http://foo.com',
39-
CURLOPT_CONNECTTIMEOUT => 10,
40-
CURLOPT_TIMEOUT => 60,
41-
CURLOPT_RETURNTRANSFER => true,
42-
CURLOPT_HEADER => true,
43-
))
37+
->with(m::on(function($arg) {
38+
$caInfo = array_diff($arg, [
39+
CURLOPT_CUSTOMREQUEST => 'GET',
40+
CURLOPT_HTTPHEADER => [],
41+
CURLOPT_URL => 'http://foo.com',
42+
CURLOPT_CONNECTTIMEOUT => 10,
43+
CURLOPT_TIMEOUT => 60,
44+
CURLOPT_RETURNTRANSFER => true,
45+
CURLOPT_HEADER => true,
46+
CURLOPT_SSL_VERIFYHOST => 2,
47+
CURLOPT_SSL_VERIFYPEER => true,
48+
]);
49+
50+
if (count($caInfo) !== 1) {
51+
return false;
52+
}
53+
54+
if (1 !== preg_match('/.+\/certs\/DigiCertHighAssuranceEVRootCA\.pem$/', $caInfo[CURLOPT_CAINFO])) {
55+
return false;
56+
}
57+
58+
return true;
59+
}))
4460
->once()
4561
->andReturn(null);
4662

@@ -55,16 +71,31 @@ public function testCanOpenGetCurlConnectionWithHeaders()
5571
->andReturn(null);
5672
$this->curlMock
5773
->shouldReceive('setopt_array')
58-
->with(array(
59-
CURLOPT_URL => 'http://foo.com',
60-
CURLOPT_CONNECTTIMEOUT => 10,
61-
CURLOPT_TIMEOUT => 60,
62-
CURLOPT_RETURNTRANSFER => true,
63-
CURLOPT_HEADER => true,
64-
CURLOPT_HTTPHEADER => array(
65-
'X-foo: bar',
66-
),
67-
))
74+
->with(m::on(function($arg) {
75+
$caInfo = array_diff($arg, [
76+
CURLOPT_CUSTOMREQUEST => 'GET',
77+
CURLOPT_HTTPHEADER => array(
78+
'X-foo: bar',
79+
),
80+
CURLOPT_URL => 'http://foo.com',
81+
CURLOPT_CONNECTTIMEOUT => 10,
82+
CURLOPT_TIMEOUT => 60,
83+
CURLOPT_RETURNTRANSFER => true,
84+
CURLOPT_HEADER => true,
85+
CURLOPT_SSL_VERIFYHOST => 2,
86+
CURLOPT_SSL_VERIFYPEER => true,
87+
]);
88+
89+
if (count($caInfo) !== 1) {
90+
return false;
91+
}
92+
93+
if (1 !== preg_match('/.+\/certs\/DigiCertHighAssuranceEVRootCA\.pem$/', $caInfo[CURLOPT_CAINFO])) {
94+
return false;
95+
}
96+
97+
return true;
98+
}))
6899
->once()
69100
->andReturn(null);
70101

@@ -80,16 +111,32 @@ public function testCanOpenPostCurlConnection()
80111
->andReturn(null);
81112
$this->curlMock
82113
->shouldReceive('setopt_array')
83-
->with(array(
84-
CURLOPT_URL => 'http://bar.com',
85-
CURLOPT_CONNECTTIMEOUT => 10,
86-
CURLOPT_TIMEOUT => 60,
87-
CURLOPT_RETURNTRANSFER => true,
88-
CURLOPT_HEADER => true,
89-
CURLOPT_POSTFIELDS => array(
90-
'baz' => 'bar',
91-
),
92-
))
114+
->with(m::on(function($arg) {
115+
$caInfo = array_diff($arg, [
116+
CURLOPT_CUSTOMREQUEST => 'POST',
117+
CURLOPT_HTTPHEADER => [],
118+
CURLOPT_URL => 'http://bar.com',
119+
CURLOPT_CONNECTTIMEOUT => 10,
120+
CURLOPT_TIMEOUT => 60,
121+
CURLOPT_RETURNTRANSFER => true,
122+
CURLOPT_HEADER => true,
123+
CURLOPT_SSL_VERIFYHOST => 2,
124+
CURLOPT_SSL_VERIFYPEER => true,
125+
CURLOPT_POSTFIELDS => array(
126+
'baz' => 'bar',
127+
),
128+
]);
129+
130+
if (count($caInfo) !== 1) {
131+
return false;
132+
}
133+
134+
if (1 !== preg_match('/.+\/certs\/DigiCertHighAssuranceEVRootCA\.pem$/', $caInfo[CURLOPT_CAINFO])) {
135+
return false;
136+
}
137+
138+
return true;
139+
}))
93140
->once()
94141
->andReturn(null);
95142

@@ -104,17 +151,32 @@ public function testCanOpenPutCurlConnection()
104151
->andReturn(null);
105152
$this->curlMock
106153
->shouldReceive('setopt_array')
107-
->with(array(
108-
CURLOPT_URL => 'http://baz.com',
109-
CURLOPT_CONNECTTIMEOUT => 10,
110-
CURLOPT_TIMEOUT => 60,
111-
CURLOPT_RETURNTRANSFER => true,
112-
CURLOPT_HEADER => true,
113-
CURLOPT_CUSTOMREQUEST => 'PUT',
114-
CURLOPT_POSTFIELDS => array(
115-
'baz' => 'bar',
116-
),
117-
))
154+
->with(m::on(function($arg) {
155+
$caInfo = array_diff($arg, [
156+
CURLOPT_CUSTOMREQUEST => 'PUT',
157+
CURLOPT_HTTPHEADER => [],
158+
CURLOPT_URL => 'http://baz.com',
159+
CURLOPT_CONNECTTIMEOUT => 10,
160+
CURLOPT_TIMEOUT => 60,
161+
CURLOPT_RETURNTRANSFER => true,
162+
CURLOPT_HEADER => true,
163+
CURLOPT_SSL_VERIFYHOST => 2,
164+
CURLOPT_SSL_VERIFYPEER => true,
165+
CURLOPT_POSTFIELDS => array(
166+
'baz' => 'bar',
167+
),
168+
]);
169+
170+
if (count($caInfo) !== 1) {
171+
return false;
172+
}
173+
174+
if (1 !== preg_match('/.+\/certs\/DigiCertHighAssuranceEVRootCA\.pem$/', $caInfo[CURLOPT_CAINFO])) {
175+
return false;
176+
}
177+
178+
return true;
179+
}))
118180
->once()
119181
->andReturn(null);
120182

@@ -129,34 +191,38 @@ public function testCanOpenDeleteCurlConnection()
129191
->andReturn(null);
130192
$this->curlMock
131193
->shouldReceive('setopt_array')
132-
->with(array(
133-
CURLOPT_URL => 'http://faz.com',
134-
CURLOPT_CONNECTTIMEOUT => 10,
135-
CURLOPT_TIMEOUT => 60,
136-
CURLOPT_RETURNTRANSFER => true,
137-
CURLOPT_HEADER => true,
138-
CURLOPT_CUSTOMREQUEST => 'DELETE',
139-
CURLOPT_POSTFIELDS => array(
140-
'baz' => 'bar',
141-
),
142-
))
194+
->with(m::on(function($arg) {
195+
$caInfo = array_diff($arg, [
196+
CURLOPT_CUSTOMREQUEST => 'DELETE',
197+
CURLOPT_HTTPHEADER => [],
198+
CURLOPT_URL => 'http://faz.com',
199+
CURLOPT_CONNECTTIMEOUT => 10,
200+
CURLOPT_TIMEOUT => 60,
201+
CURLOPT_RETURNTRANSFER => true,
202+
CURLOPT_HEADER => true,
203+
CURLOPT_SSL_VERIFYHOST => 2,
204+
CURLOPT_SSL_VERIFYPEER => true,
205+
CURLOPT_POSTFIELDS => array(
206+
'baz' => 'bar',
207+
),
208+
]);
209+
210+
if (count($caInfo) !== 1) {
211+
return false;
212+
}
213+
214+
if (1 !== preg_match('/.+\/certs\/DigiCertHighAssuranceEVRootCA\.pem$/', $caInfo[CURLOPT_CAINFO])) {
215+
return false;
216+
}
217+
218+
return true;
219+
}))
143220
->once()
144221
->andReturn(null);
145222

146223
$this->curlClient->openConnection('http://faz.com', 'DELETE', array('baz' => 'bar'));
147224
}
148225

149-
public function testCanAddBundledCert()
150-
{
151-
$this->curlMock
152-
->shouldReceive('setopt')
153-
->with(CURLOPT_CAINFO, '/.fb_ca_chain_bundle\.crt$/')
154-
->once()
155-
->andReturn(null);
156-
157-
$this->curlClient->addBundledCert();
158-
}
159-
160226
public function testCanCloseConnection()
161227
{
162228
$this->curlMock

‎tests/HttpClients/FacebookGuzzleHttpClientTest.php

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,14 @@ public function testCanSendNormalRequest()
4949
$this->guzzleMock
5050
->shouldReceive('createRequest')
5151
->once()
52-
->with('GET', 'http://foo.com/', array())
52+
->with('GET', 'http://foo.com/', m::on(function($arg) {
53+
if (1 !== preg_match('/.+\/certs\/DigiCertHighAssuranceEVRootCA\.pem$/', $arg['verify'])) {
54+
return false;
55+
}
56+
return true;
57+
}))
5358
->andReturn($requestMock);
59+
5460
$this->guzzleMock
5561
->shouldReceive('send')
5662
->once()
@@ -83,8 +89,14 @@ public function testThrowsExceptionOnClientError()
8389
$this->guzzleMock
8490
->shouldReceive('createRequest')
8591
->once()
86-
->with('GET', 'http://foo.com/', array())
92+
->with('GET', 'http://foo.com/', m::on(function($arg) {
93+
if (1 !== preg_match('/.+\/certs\/DigiCertHighAssuranceEVRootCA\.pem$/', $arg['verify'])) {
94+
return false;
95+
}
96+
return true;
97+
}))
8798
->andReturn($requestMock);
99+
88100
$this->guzzleMock
89101
->shouldReceive('send')
90102
->once()

0 commit comments

Comments
 (0)