Re: [RFC] Timing attack safe string comparison function

From: Date: Fri, 27 Dec 2013 13:57:51 +0000
Subject: Re: [RFC] Timing attack safe string comparison function
References: 1 2 3 4  Groups: php.internals 
Request: Send a blank email to internals+get-70880@lists.php.net to get a copy of this message
>> Perhaps this could be implemented by adding an optional parameter to the >> existing str...cmp() series of functions instead of adding more functions. > > We may do this. > However, strcmp() returns 0 for equal strings while strcmp_secure() will > return TRUE for equal strings. We could make strcmp_secure() returns > FALSE/0 for equal strings, but it does not make much sense. > There's no reason the return value has to be (or even should be) different from normal strcmp() usage. The important aspect is that the timing be (reasonably) constant. So if I called strcmp($userSupplied, $knownValue, STRCMP_CONSTANT_TIME) or whatever, I would reasonably (as a user) expect the return values to still be 0, -1, 1 in line with "normal" strcmp() usage. So on that count, I reject your counter-argument. However, I do worry about using this syntax for a couple reasons of unintended consequences: * Semantics: The first two parameters are no longer interchangeable (since timing depends on one of them). Putting it in the wrong spot negates the usefulness, and that'll be easy with the temptation to codemod an extra argument in blindly. * Fat Fingers: A third int/bool field to strcmp() could very easily get misinterpreted by accidentally using strncmp(). Now that true/0x01 looks like a length of 1 and your strcmp() only has to match the first character! So while I like the elegance of adding an options field to strcmp/strncmp(), I see it potentially making matters worse. Lastly, please stay away from names like "strcmp_secure()". 5-10 years from now such a function will inevitably turn out to be insecure in some way and we'll need to add strcmp_really_secure_I_mean_it_this_time(). That way lies madness. -Sara

« previous php.internals (#70880) next »