Hi everyone,
sorry for not getting back earlier, real life taking it's toll. I'll try to answer things
in just a couple of E-Mail to not cause too much traffic (there seems to be a lot lately!)
On 03.02.2014, at 13:23, Andrea Faulds <[email protected]> wrote:
> On 02/02/14 22:50, Rouven Weßling wrote:
>> as I've received no further feedback I've opened the voting on "Timing
>> attack safe string comparison function":
>
> I've voted yes. However, at the risk of opening more bikeshedding again, I should say that
> I don't think hash_compare is an appropriate name. It's a timing attack-safe string
> comparison function, so I think something like str_equals_time_constant might be better as it is not
> so much a hash comparison function as a string comparison function.
I'd rather not discuss this anymore. The discussion was open since mid-december. By my reading,
there was no universally agreed name. Since it was suggested this goes into ext/hash starting with
hash_ also seemed important to me.
On 03.02.2014, at 17:18, Sara Golemon <[email protected]> wrote:
> Voted yes, but IMO the comparison function should behave a little more
> like ===. That is: something like hash_compare(null,"") should return
> false. Possibly be even more strict and require both input parameters
> to be string (e.g. hash_compare(123,123) would return false as well).
>
> But there's some "non-PHP"ish about that idea so I'm not horribly fussed by
> it.
That sounds like a good idea to me. I'd wager we can change this post RFC too? Or is the
process more strict than I think it is?
I like the suggestion of an E_WARNING for that case too.
On 03.02.2014, at 18:10, Nikita Popov <[email protected]> wrote:
> Did your code already get reviewed by someone with understanding of the issue? From a quick
> glance, two potential issues:
Anthony Ferra reviewed (and improved) my original patch, however it has been changed since. Notably
the MAX change wasn't included originally.
> * You are using MAX, i.e. an if-then-else branch. I'm pretty sure that the if and else
> branches will have different instruction counts in that case. Simple alternative would be something
> fixed like mod_len = known_len+1 or known_len&1.
This problem should be limited by that fact, that except for the case where known_len == 0, always
the same branch is hit.
> * You leak information on mod_len / known_len, because you will have different cache access
> patterns for comparing always the same 10 memory positions and 10000 different ones, at least
> I'd assume so.
> I don't know how you can prevent the latter issue, and if it is possible at all.
> Personally I'd just drop the length magic and explicitly document it to be safe for
> equal-length strings only. In any case you should have this reviewed by someone with more than just
> a cursory understanding of the matter.
I agree leaking length may happen. I'm wondering if my first patch wasn't safer in this
regard. If no better implementation can be found I'd suggest to just document, that length may
leak. However to me that's acceptable as long as this doesn't allow approximating the
known string by changing the input byte by byte.
On 04.02.2014, at 07:20, Tjerk Meesters <[email protected]> wrote:
> Given the name hash_compare() it makes a good case to add a bail-out
> branch based on string length, in which case you don't have to remember
> which argument should come first ... still, it may be nice to have a
> generic comparison function :)
While I like the idea, of foregoing this entire discussion I think it is a good idea to get
something better than === out to people who compare mixed length strings.
On 05.02.2014, at 03:58, Yasuo Ohgaki <[email protected]> wrote:
> It could be optimized a little since 256 is too much for now.
> How about make MAX returns max of 3 values?
>
> len = MAX(known_len, user_len, 64);
What would this buy us? We still get branch on MAX and the memory access would still go the same
memory if the string is shorter than 64 bytes.
Best regards
Rouven