Security update for signature validation on LogoutRequest/LogoutResponse.

pitbulk · pitbulk · commit 949359f5cad5 · 2017-02-28T17:16:58.000+01:00
In order to verify Signatures on Logoutrequests and LogoutResponses we use
the verifySignature of the class XMLSecurityKey from the xmlseclibs library.
That method end up calling openssl_verify() depending on the signature algorithm used.

The openssl_verify() function returns 1 when the signature was successfully verified,
0 if it failed to verify with the given key, and -1 in case an error occurs.
PHP allows translating numerical values to boolean implicitly, with the following correspondences:
- 0 equals false.
- Non-zero equals true.

This means that an implicit conversion to boolean of the values returned by openssl_verify()
will convert an error state, signaled by the value -1, to a successful verification of the
signature (represented by the boolean true).

The LogoutRequest/LogoutResponse signature validator was performing an implicit conversion to boolean
of the values returned by the verify() method, which subsequently will return the same output
as openssl_verify() under most circumstances.
This means an error during signature verification is treated as a successful verification by the method.

Since the signature validation of SAMLResponses were not affected, the impact of this security
vulnerability is lower, but an update of the php-saml toolkit is recommended.
diff --git a/lib/Saml2/LogoutRequest.php b/lib/Saml2/LogoutRequest.php
@@ -402,7 +402,7 @@ public function isValid($retrieveParametersFromServer=false)
                     }
                 }
 
-                if (!$objKey->verifySignature($signedQuery, base64_decode($_GET['Signature']))) {
+                if ($objKey->verifySignature($signedQuery, base64_decode($_GET['Signature'])) !== 1) {
                     throw new OneLogin_Saml2_ValidationError(
                         "Signature validation failed. Logout Request rejected",
                         OneLogin_Saml2_ValidationError::INVALID_SIGNATURE
diff --git a/lib/Saml2/LogoutResponse.php b/lib/Saml2/LogoutResponse.php
@@ -209,7 +209,7 @@ public function isValid($requestId = null, $retrieveParametersFromServer=false)
                     }
                 }
 
-                if (!$objKey->verifySignature($signedQuery, base64_decode($_GET['Signature']))) {
+                if ($objKey->verifySignature($signedQuery, base64_decode($_GET['Signature'])) !== 1) {
                     throw new OneLogin_Saml2_ValidationError(
                         "Signature validation failed. Logout Response rejected",
                         OneLogin_Saml2_ValidationError::INVALID_SIGNATURE

Original file line number	Diff line number	Diff line change
`@@ -402,7 +402,7 @@ public function isValid($retrieveParametersFromServer=false)`
`402`	`402`	`}`
`403`	`403`	`}`
`404`	`404`
`405`		`- if (!$objKey->verifySignature($signedQuery, base64_decode($_GET['Signature']))) {`
	`405`	`+ if ($objKey->verifySignature($signedQuery, base64_decode($_GET['Signature'])) !== 1) {`
`406`	`406`	`throw new OneLogin_Saml2_ValidationError(`
`407`	`407`	`"Signature validation failed. Logout Request rejected",`
`408`	`408`	`OneLogin_Saml2_ValidationError::INVALID_SIGNATURE`
Original file line number	Diff line number	Diff line change
`@@ -209,7 +209,7 @@ public function isValid($requestId = null, $retrieveParametersFromServer=false)`
`209`	`209`	`}`
`210`	`210`	`}`
`211`	`211`
`212`		`- if (!$objKey->verifySignature($signedQuery, base64_decode($_GET['Signature']))) {`
	`212`	`+ if ($objKey->verifySignature($signedQuery, base64_decode($_GET['Signature'])) !== 1) {`
`213`	`213`	`throw new OneLogin_Saml2_ValidationError(`
`214`	`214`	`"Signature validation failed. Logout Response rejected",`
`215`	`215`	`OneLogin_Saml2_ValidationError::INVALID_SIGNATURE`