Skip to content

Commit c78ba29

Browse files
committed
Secure the way the verifySignature method is used
It either returns bool, int: 1, 0 or -1 and -1 will be evaluated to be true in PHP. Making the use of this method unsafe when not strictly testing for the desired outcome.
1 parent 2924e29 commit c78ba29

2 files changed

Lines changed: 11 additions & 10 deletions

File tree

src/Http/ReceivedAuthnRequestQueryString.php

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -288,9 +288,10 @@ public function getSignedRequestPayload()
288288
*/
289289
public function verify(XMLSecurityKey $key)
290290
{
291-
if ($key->verifySignature($this->getSignedRequestPayload(), $this->getDecodedSignature())) {
292-
return true;
291+
$isVerified = $key->verifySignature($this->getSignedRequestPayload(), $this->getDecodedSignature());
292+
if ($isVerified !== 1) {
293+
return false;
293294
}
294-
return false;
295+
return true;
295296
}
296297
}

src/Signing/SignatureVerifier.php

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
use Psr\Log\LoggerInterface;
2222
use RobRichards\XMLSecLibs\XMLSecurityKey;
2323
use SAML2\Certificate\Key;
24-
use SAML2\Certificate\KeyLoader as KeyLoader;
24+
use SAML2\Certificate\KeyLoader;
2525
use SAML2\Certificate\X509;
2626
use Surfnet\SamlBundle\Entity\ServiceProvider;
2727
use Surfnet\SamlBundle\Http\ReceivedAuthnRequestQueryString;
@@ -151,13 +151,13 @@ public function isSignedWith(AuthnRequest $request, X509 $publicKey)
151151
$key = new XMLSecurityKey(XMLSecurityKey::RSA_SHA256, array('type' => 'public'));
152152
$key->loadKey($publicKey->getCertificate());
153153

154-
if ($key->verifySignature($request->getSignedRequestQuery(), $request->getSignature())) {
155-
$this->logger->debug('Signature VERIFIED');
156-
return true;
154+
$isVerified = $key->verifySignature($request->getSignedRequestQuery(), $request->getSignature());
155+
if ($isVerified !== 1) {
156+
$this->logger->debug('Signature NOT VERIFIED');
157+
return false;
157158
}
158159

159-
$this->logger->debug('Signature NOT VERIFIED');
160-
161-
return false;
160+
$this->logger->debug('Signature VERIFIED');
161+
return true;
162162
}
163163
}

0 commit comments

Comments
 (0)