WSSE Auth: Timing safe comparison

[merk]

Q	A
Doc fix?	yes
New docs?	no
Applies to	2.3+
Fixed tickets	n/a

I believe we should be providing examples that use timing safe operations when comparing password hashes, or any other kind of sensitive comparison that could leak timing information.

[stof]

👍

[stof]

btw, it could be useful to add a note explaining what this method does compared to === so that readers can understand why we should use it

[merk]

It seems like it might not be worth doing: http://security.stackexchange.com/questions/9192/timing-attacks-on-password-hashes

If we were comparing a real password rather than a hash it makes more sense (at least according to the answer at SO)

[weaverryan]

Unless we're absolutely sure, I think we should use this change. But @merk, can you add a quick comment above this line like @stof suggested to explain this it is basically === but avoids timing attacks?

[merk]

I'm more inclined to take this approach instead:

https://www.isecpartners.com/blog/2011/february/double-hmac-verification.aspx

[xabbuh]

@merk Do you think it's a good idea to show a PHP implementation of it in the docs? I think that would be rather confusing. Or do you plan to create a pull request on the Symfony repository to improve the StringUtils::equals() method?

[merk]

The StringUtils::equals function works fine for a 'mostly guaranteed' constant time comparison (it seems), but its simpler in the case of a HMAC just to HMAC each result again.

What I mean when i post that link is that sha1(base64_decode($nonce).$created.$secret, true) becomes a private function and it gets called to hash each digest a total of 2 times:

    protected function validateDigest($digest, $nonce, $created, $secret)
    {
        // ...

        $expected = sha1(sha1(base64_decode($nonce).$created.$secret, true).$secret);
        $digest = sha1(base64_decode($digest).$secret);

        return $expected === $digest;
    }

But I'm totally not an expert on this.

[weaverryan]

@merk So what is your final proposal? Are you saying that StringUtils is actually security overkill and that your above code is good enough? If so, I'd still want to use StringUtils - it looks much simpler to me.

[weaverryan]

ping @merk!

[merk]

I'm probably not the right person to be making a decision about this one - I have limited security experience.

It seems like we should be doing something to reduce leaking of timing attacks in this example and using StringUtils seems like a worthy approach to me.

[xabbuh]

I think having this change with a small note explaining why to use StringUtils::equals() would be a good start.

[xabbuh]

@merk Can you add such a note?

[merk]

Where would the note go - in the code?

This method compares the hashes using StringUtils::equals() because not using a constant time comparison for this check could lead to a timing based attack?

[xabbuh]

I would add a note directive for this right after the code block.

[xabbuh]

And it's probably a good idea to link to, for example, http://en.wikipedia.org/wiki/Timing_attack to provide a bit more context.

[merk]

So I'm not sure on the whole rst formatting but something like this?

.. note::

    The comparsion of the expected and provided digests use a constant time 
    comparison provided by the ``equals`` method of the 
    :class:`Symfony\\Component\\Security\\Core\\Util\\StringUtils``. It is
    used to stop possible `timing attacks`_.

[xabbuh]

Looks fine, I would just add the API link for the method instead of the class:

.. note::

    The comparsion of the expected and the provided digests uses a constant
    time comparison provided by the
    :method:`Symfony\\Component\\Security\\Core\\Util\\StringUtils::equals`
    method of the ``StringUtils`` class. It is used to mitigate possible
    `timing attacks`_.

What do you think?

[merk]

Just added that note, unfortunately not near a machine with git so its another commit but I assume you guys have the same magical tool that Fabien uses that can squash things without too much effort?

[xabbuh]

Indeed, @weaverryan and @wouterj use that too. However, it would be nice if you could rebase your commits when your are back. At least I am not sure if the tool could handle that.

[weaverryan]

A bit of a long road with this one, but thanks for helping to get this in @merk :). Cheers!