Possible bug in Utils::validateBinarySign()

[rudischenck]

I may have found a bug with this issue, not sure. The reason It works when you call processSLO with $retrieveParametersFromServer set to false is because the signed query does not get URL encoded before validation against the certificate. The offending code starts on Utils.php:1505. Because the signed query is URL encoded the url's don't match what's in the logout response and it throws an error.

        if ($retrieveParametersFromServer) {
            $signedQuery = $messageType.'='.Utils::extractOriginalQueryParam($messageType);
            if (isset($getData['RelayState'])) {
                $signedQuery .= '&RelayState='.Utils::extractOriginalQueryParam('RelayState');
            }
            $signedQuery .= '&SigAlg='.Utils::extractOriginalQueryParam('SigAlg');
        } else {
            $signedQuery = $messageType.'='.urlencode($getData[$messageType]);
            if (isset($getData['RelayState'])) {
                $signedQuery .= '&RelayState='.urlencode($getData['RelayState']);
            }
            $signedQuery .= '&SigAlg='.urlencode($signAlg);
        }

I could be wrong idk

Originally posted by @rudischenck in #80 (comment)

[pitbulk]

URL encoding could be done uppercase or lowercase and in addition, can be done following RFC 3986 or not.
Based on that, if an entity sign using a url enconding mechanism that differs the way another entity decode it, will cause Signature validation issues.
In order to avoid it, the use of retrieveParametersFromServer, directly tries to get the original query parameter retrieved by the server in order to avoid an extra decode/encoding process.

The extractOriginalQueryParam already get the parameter as its received at the server, so it is already urlencoded.

[LukasReschke]

@pitbulk, are you open for a pull request which would remove the explicit call to retrieveParametersFromServer and would perform the current check, and if that fails, falls backs to the retrieveParametersFromServer logic?

That would really help us (Nextcloud), as this would remove another custom configuration.

[pitbulk]

But removing retrieveParametersFromServer will create compatibility issues with old versions and entities connected to ADFS IdPs will have to validate the SAMLResponse 2 times.

I believe makes more sense to maintain this setting rather than the extra resources required for the x2 validation on each SSO or SLO process.