robrichards / xmlseclibs

A PHP library for XML Security
BSD 3-Clause "New" or "Revised" License
387 stars 181 forks source link

Non-timing safe comparison in XMLSecurityKey::verifySignature #175

Open enygma opened 6 years ago

enygma commented 6 years ago

In the XMLSecurityKey::verifySignature method for the self:: HMAC_SHA1 comparison the strcmp function is use to compare the $signature versus the $expectedSignature. This function is not a timing safe comparison and should be replaced with something like hash_equals.

public function verifySignature($data, $signature)
    {
        switch ($this->cryptParams['library']) {
            case 'openssl':
                return $this->verifyOpenSSL($data, $signature);
            case (self::HMAC_SHA1):
                $expectedSignature = hash_hmac("sha1", $data, $this->key, true);
                return hash_equals($signature, $expectedSignature);
        }
    }

The hash_equals function was added in PHP 5.6 but this library only requires >=5.4. Fortunately there's a polyfill that can be used that will use the built-in function if available: https://gist.github.com/christianfutterlieb/3cf85bc3fe16c70c0442

robrichards commented 6 years ago

Thanks, I'm taking a look at the changes I am going to be releasing shortly and considering wether just to bump version or not; otherwise will look at the polyfill

jaimeperez commented 5 years ago

Even though having a polyfill is generally a good idea, as it makes your code simpler, you can always use your own function to perform the comparison, and use hash_equals() if available, or compare manually byte-by-byte. Here's an example.