# [VOTE] Timing attack safe string comparison function - Thread: https://externals.io/message/72018 - Original archive: https://news-web.php.net/php.internals/72018 - Started by me@rouvenwessling.de on 2014-02-02T22:50:21+00:00 - Messages: 49 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/72018.md?page=2 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #72018 — me@rouvenwessling.de — 2014-02-02T22:50:21+00:00 URL: https://externals.io/message/72018#72018 Hi internals, as I've received no further feedback I've opened the voting on "Timing attack safe string comparison function": - https://wiki.php.net/rfc/timing_attack Voting ends on 2014/02/09 11:00PM UTC Best regards Rouven --- ## #72030 — Yasuo Ohgaki — 2014-02-03T01:48:35+00:00 URL: https://externals.io/message/72018#72030 · In reply to: #72018 Hi Rouven, > as I've received no further feedback I've opened the voting on "Timing > attack safe string comparison function": > > - https://wiki.php.net/rfc/timing_attack > > Voting ends on 2014/02/09 11:00PM UTC > > Best regards > Thank you for setting up vote! Timing attack is in the wild. This function is must have function, even for release versions. IMHO. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #72086 — Sara Golemon — 2014-02-03T16:18:49+00:00 URL: https://externals.io/message/72018#72086 · In reply to: #72018 > as I've received no further feedback I've opened the voting on "Timing attack safe string comparison function": > > - https://wiki.php.net/rfc/timing_attack > > Voting ends on 2014/02/09 11:00PM UTC > 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. -Sara --- ## #72087 — Sara Golemon — 2014-02-03T16:21:25+00:00 URL: https://externals.io/message/72018#72087 > > 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 seem to remember that topic coming up (and I agree with you), but I don't recall anyone coming up with a universally liked name. I suppose something is better than nothing. -Sara --- ## #72088 — Nikita Popov — 2014-02-03T17:10:05+00:00 URL: https://externals.io/message/72018#72088 · In reply to: #72018 On Sun, Feb 2, 2014 at 11:50 PM, Rouven Weßling wrote: > Hi internals, > > as I've received no further feedback I've opened the voting on "Timing > attack safe string comparison function": > > - https://wiki.php.net/rfc/timing_attack > > Voting ends on 2014/02/09 11:00PM UTC > > Best regards > Rouven > Did your code already get reviewed by someone with understanding of the issue? From a quick glance, two potential issues: * 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. * 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. Nikita --- ## #72110 — padraic.brady@gmail.com — 2014-02-03T19:58:21+00:00 URL: https://externals.io/message/72018#72110 · In reply to: #72088 Hi Nikita, > Did your code already get reviewed by someone with understanding of the > issue? From a quick glance, two potential issues: > * 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. > * 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. The constant time comparison function, which is fairly standard at this point, doesn't protect length - only the timing of comparing two strings. The inputs should have identical length otherwise it's being used inappropriately. It may seem that length being leaked is bad, but it's assumed to be a predictable factor in hashing passwords, etc. That may get taken for granted given how and where it's implemented in userland so an exception is not out of the question for a base function where lengths do differ. Paddy -- -- Pádraic Brady http://blog.astrumfutura.com http://www.survivethedeepend.com Zend Framework Community Review Team Zend Framework PHP-FIG Representative --- ## #72118 — Yasuo Ohgaki — 2014-02-03T20:57:13+00:00 URL: https://externals.io/message/72018#72118 · In reply to: #72088 Hi Nikita, > > […] > > Did your code already get reviewed by someone with understanding of the > issue? From a quick glance, two potential issues: > * 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. > * 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. Length leak is known issue and we may improve these. There was discussion for this. For the sake of completeness, we may address issues now or later. To be honest, I think length leak must be avoided especially for shorter strings. It would be better to iterate at least 100 times regardless of input. Perhaps, something like + /** + * If known_string has a length of 0 we set the length to 1, + * this will cause us to compare all bytes of userString with the null byte which fails + */ + mod_len = MAX(known_len, 1); len = MAX(known_len, 127); + + /* This is security sensitive code. Do not optimize this for speed. */ + result = known_len - user_len; + for (j = 0; j < user_len; j++) { for (j = 0; j < len; j++) { + result |= known_str[j % mod_len] ^ user_str[j]; result |= known_str[j % known_len] ^ user_str[j % user_len]; + } Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #72120 — Yasuo Ohgaki — 2014-02-03T21:04:35+00:00 URL: https://externals.io/message/72018#72120 · In reply to: #72118 Hi all, We can be more conservative. So 127 is not long enough for SHA-512, make it 256 for larger hashes? > Length leak is known issue and we may improve these. There was discussion > for this. > For the sake of completeness, we may address issues now or later. > > To be honest, I think length leak must be avoided especially for shorter > strings. > It would be better to iterate at least 100 times regardless of input. > > Perhaps, something like > > + /** > + * If known_string has a length of 0 we set the length to 1, > + * this will cause us to compare all bytes of userString with the null > byte which fails > + */ > + mod_len = MAX(known_len, 1); > len = MAX(known_len, 127); > len = MAX(known_len, 256); > + > + /* This is security sensitive code. Do not optimize this for speed. */ > + result = known_len - user_len; > > + for (j = 0; j < user_len; j++) { > > for (j = 0; j < len; j++) { > > > + result |= known_str[j % mod_len] ^ user_str[j]; > > result |= known_str[j % known_len] ^ user_str[j % user_len]; > + } > Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #72138 — Stas Malyshev — 2014-02-03T22:09:32+00:00 URL: https://externals.io/message/72018#72138 · In reply to: #72088 Hi! > * 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. > * You leak information on mod_len / known_len, because you will have If it's meant to compare hashes and other such things, we can presume the attacker already knows what your code does, and thus knows what the expected hash length is. What he doesn't know is what that hash is. The timing attack is based on the fact that regular comparison drops after first mismatch, so the attacker by trying different first symbols and using time as oracle between match and mismatch, can guess the hash. The length of the hash however is not useful for him - for most standard crypto protocols all lengths are already known and even if you are using some modifications basic crypto principles tell us to assume your algorithm is known to the attacker and thus most probably your known hash length is too. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #72167 — Yasuo Ohgaki — 2014-02-04T04:06:30+00:00 URL: https://externals.io/message/72018#72167 · In reply to: #72138 Hi Stas, On Tue, Feb 4, 2014 at 7:09 AM, Stas Malyshev wrote: > > […] > the > > […] > > If it's meant to compare hashes and other such things, we can presume > the attacker already knows what your code does, and thus knows what the > expected hash length is. What he doesn't know is what that hash is. The > timing attack is based on the fact that regular comparison drops after > first mismatch, so the attacker by trying different first symbols and > using time as oracle between match and mismatch, can guess the hash. The > length of the hash however is not useful for him - for most standard > crypto protocols all lengths are already known and even if you are using > some modifications basic crypto principles tell us to assume your > algorithm is known to the attacker and thus most probably your known > hash length is too. I agree partially. Length leak would not be serious issue when length is long enough. I've already posted possible improvement that would not be affected by supplied string length. + /** + * If known_string has a length of 0 we set the length to 1, + * this will cause us to compare all bytes of userString with the null byte which fails + */ + mod_len = MAX(known_len, 1); len = MAX(known_len, 256); + + /* This is security sensitive code. Do not optimize this for speed. */ + result = known_len - user_len; + for (j = 0; j < user_len; j++) { for (j = 0; j < len; j++) { + result |= known_str[j % mod_len] ^ user_str[j]; result |= known_str[j % known_len] ^ user_str[j % user_len]; + } It's enough for hash functions up to 1024 bits. For SHA-3, we may set larger minimum. Comments, corrections, improvements are appreciated. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #72168 — Yasuo Ohgaki — 2014-02-04T04:30:06+00:00 URL: https://externals.io/message/72018#72168 · In reply to: #72167 Hi all, > It's enough for hash functions up to 1024 bits. For SHA-3, we may set > larger minimum. Just an additional note. AFAIK, SHA-3 also has 512 bits maximum. Since it has more bits internally, it may be extended. There may be hash functions that have larger bits. 256 iterations would be long enough for many years. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #72175 — Tjerk Meesters — 2014-02-04T06:20:37+00:00 URL: https://externals.io/message/72018#72175 · In reply to: #72088 > On Sun, Feb 2, 2014 at 11:50 PM, Rouven Weßling > […] > > > […] > > Did your code already get reviewed by someone with understanding of the > issue? From a quick glance, two potential issues: > * 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. > I can't speak for other compilers, but gcc compiles the ternary to this: cmpl $0, -8(%rbp) cmovg -8(%rbp), %eax So it comes down to whether `cmovg` takes a variable amount of time; that said, the move operation is only required if the known length is zero, i.e. you're comparing user input against an empty string which is quite unlikely. > * 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. > There's one particular scenario that comes to mind: if the string data spans at least two memory pages; assuming memory allocation is optimized to fit allocated memory inside one page whenever possible, we're talking about string lengths bigger than the page itself, i.e. 4kB / 8kB. Given that the known string is relatively small, the leaked information may reveal that the given string is bigger than the known string. The above is my own logical deduction, I'm not an expert in this area :) 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. > 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 :) > Nikita > -- -- Tjerk --- ## #72235 — Yasuo Ohgaki — 2014-02-05T02:39:06+00:00 URL: https://externals.io/message/72018#72235 · In reply to: #72086 Hi all, > 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). > Good point! Comment just to make sure we have better implementation. hash_compare() should not accept anything other than string. `E_WARNING` is appropriate. IMO. So get parameter as zval and check types before processing is preferred. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #72236 — Yasuo Ohgaki — 2014-02-05T02:58:42+00:00 URL: https://externals.io/message/72018#72236 · In reply to: #72167 Hi all, 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); > + /** > + * If known_string has a length of 0 we set the length to 1, > + * this will cause us to compare all bytes of userString with the null > byte which fails > + */ > + mod_len = MAX(known_len, 1); > len = MAX(known_len, 256); > len = MAX(known_len, user_len, 64); > + > + /* This is security sensitive code. Do not optimize this for speed. */ > + result = known_len - user_len; > > + for (j = 0; j < user_len; j++) { > > for (j = 0; j < len; j++) { > > > + result |= known_str[j % mod_len] ^ user_str[j]; > > result |= known_str[j % known_len] ^ user_str[j % user_len]; > + } > 64 is long enough for SHA-256 and if parameter is longer than that it will be used. Even if user used it to compare 'raw password', they are protected well from timing attack. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #72306 — me@rouvenwessling.de — 2014-02-05T23:36:44+00:00 URL: https://externals.io/message/72018#72306 · In reply to: #72236 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!) > > > […] > > 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. > 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. > 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. > 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. > 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 --- ## #72310 — Yasuo Ohgaki — 2014-02-06T01:28:38+00:00 URL: https://externals.io/message/72018#72310 · In reply to: #72306 Hi Rouven, > > […] > > > 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. I think we may ignore length leak and focus comparison. len = MAX(known_len, 64) is for to make more difficult to guess short secret(known_str) byte length, hopefully. Just trying to less obvious, but it's not hiding. Perhaps, something like this would be good enough. + /** + * If known_string has a length of 0 we set the length to 1, + * this will cause us to compare all bytes of userString with the null byte which fails + */ + mod_len = MAX(known_len, 1); len = MAX(user_len, 64); // Do not care much len = MAX(known_len, len); // Do not care much // These kind of operations have done somewhere anyway // Just don't care. k = (unsinged char *)emalloc(len+1) u = (unsinged char *)emalloc(len+1); memset(k, 0, len+1); memset(u, 0, len+1); strncpy(k, known_str, known_len); strncpy(u, user_str, user_len); + + /* This is security sensitive code. Do not optimize this for speed. */ + result = known_len - user_len; + for (j = 0; j < user_len; j++) { + result |= known_str[j % mod_len] ^ user_str[j]; for (; len > 0; len--) { result |= *k++ ^ *u++; // This must be constant. Use simpler operation and keep constant operation here is enough. + } Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #72313 — Yasuo Ohgaki — 2014-02-06T03:56:11+00:00 URL: https://externals.io/message/72018#72313 · In reply to: #72310 Hi all, Padraic gave me an another idea of additional mitigation for this. Although we cannot rely on it, randomized delay can be used as mitigation. It would be good for length leak. > Perhaps, something like this would be good enough. > > + /** > + * If known_string has a length of 0 we set the length to 1, > + * this will cause us to compare all bytes of userString with the null > byte which fails > + */ > + mod_len = MAX(known_len, 1); > len = MAX(user_len, 64); // Do not care much > len = MAX(known_len, len); // Do not care much > > // These kind of operations have done somewhere anyway > // Just don't care. > k = (unsinged char *)emalloc(len+1) > u = (unsinged char *)emalloc(len+1); > memset(k, 0, len+1); > memset(u, 0, len+1); > strncpy(k, known_str, known_len); > strncpy(u, user_str, user_len); > // Determination of delay is tricky. Too short or too long delay does not work. // It depends on execution path/data. e.g. How many times strlen/strncpy/etc is called, length of string. // I'm not sure if this is sufficient/valid as mitigation for average usage. Experiments are needed. // Improvement/suggestion is appreciated. r1 = (unsigned char)get_random_byte(); r1 *= 2; // doubles range for (; r1 > 0; r1--) { r2 = (unsigned char)get_random_byte(); for (; r2 > 0; r2--) { if (r1 < r2) { buf[r2] = r2 % r1; } else { buf[r2] = r1 % r2; } } } + > + /* This is security sensitive code. Do not optimize this for speed. */ > + result = known_len - user_len; > > > + for (j = 0; j < user_len; j++) { > > > + result |= known_str[j % mod_len] ^ user_str[j]; > > for (; len > 0; len--) { > > result |= *k++ ^ *u++; // This must be constant. Use simpler operation and keep constant operation here is enough. > > > + } > -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #72314 — Yasuo Ohgaki — 2014-02-06T04:20:55+00:00 URL: https://externals.io/message/72018#72314 · In reply to: #72313 Hi all, > Padraic gave me an another idea of additional mitigation for this. > Although we cannot rely on it, randomized delay can be used > as mitigation. It would be good for length leak. > > > > > […] > > // Determination of delay is tricky. Too short or too long delay does not > work. > // It depends on execution path/data. e.g. How many times > strlen/strncpy/etc is called, length of string. > // I'm not sure if this is sufficient/valid as mitigation for average > usage. Experiments are needed. > // Improvement/suggestion is appreciated. > r1 = (unsigned char)get_random_byte(); > r1 *= 2; // doubles range > for (; r1 > 0; r1--) { > r2 = (unsigned char)get_random_byte(); > for (; r2 > 0; r2--) { > if (r1 < r2) { > buf[r2] = r2 % r1; > } else { > buf[r2] = r1 % r2; > } > } > } > > + > > […] > Since comparison of short and/or not hashed data (e.g. user supplied raw password) should not be done as the function name imply, we may better to document so that users always compare hashed values even when they store raw password/etc. So randomized delay may be overkill. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #72315 — Christopher Jones — 2014-02-06T04:23:23+00:00 URL: https://externals.io/message/72018#72315 · In reply to: #72313 > Hi all, > > Padraic gave me an another idea of additional mitigation for this. What's the status of the RFC? It's listed as under voting but there is deep discussion still ongoing. The RFC is very short on technical detail. It is also lacking an end-of-vote date. It's not clear what the RFCs path forward is. (If this info is in a mail thread, but not in the RFC then remember readers/voters should not have to trawl internals mail to understand the proposal and its direction). Personally, I suggest the vote be closed/withdrawn with the assumption the concept was accepted 15 to 1. Then work on the code until a mutually acceptable and useful implementation is found. After that, a quick vote can be made on the implementation. Chris -- christopher.jones@oracle.com http://twitter.com/ghrd Free PHP & Oracle book: http://www.oracle.com/technetwork/topics/php/underground-php-oracle-manual-098250.html --- ## #72316 — Yasuo Ohgaki — 2014-02-06T04:31:17+00:00 URL: https://externals.io/message/72018#72316 · In reply to: #72314 Sorry for multiple posts. > Since comparison of short and/or not hashed data (e.g. user supplied raw > password) should > not be done as the function name imply, we may better to document so that > users always > compare hashed values even when they store raw password/etc. > So randomized delay may be overkill. > Because user should not pass other than hashed values, we may return `FALSE` simply when length mismatches. Generated hashed length should not be a secret. This get rid of length leak issue and the function name is good for this purpose and make the operation always constant. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.netSo --- ## #72318 — Yasuo Ohgaki — 2014-02-06T05:17:18+00:00 URL: https://externals.io/message/72018#72318 · In reply to: #72316 It's me again. > Sorry for multiple posts. > > > > > […] > > Because user should not pass other than hashed values, we may > return `FALSE` simply when length mismatches. Generated hashed > length should not be a secret. This get rid of length leak issue and > the function name is good for this purpose and make the operation > always constant. > Since there is internal code that is vulnerable to timing attack, could you make it PHPAPI? For example, ext/session/mod_mm.c is comparing session ID using `strcmp()` for (prev = NULL, ret = data->hash[slot]; ret; prev = ret, ret = ret->next) { if (ret->hv == hv && !strcmp(ret->key, key)) { break; } } Regards, P.S. Other save handlers are also vulnerable to timing attack. It could be mitigated the attack by specifying minimum length of session ID. I'll add this new INI option to session module. -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #72320 — Pierre Joye — 2014-02-06T05:24:53+00:00 URL: https://externals.io/message/72018#72320 · In reply to: #72315 On Thu, Feb 6, 2014 at 5:23 AM, Christopher Jones wrote: > > > > […] > > > What's the status of the RFC? Voting phase > It's listed as under voting but there > is deep discussion still ongoing. Yes, a request for peer review has been asked, that's why I asked a couple of security related contacts to take a look at the code. It cannot hurt. > The RFC is very short on technical > detail. It is also lacking an end-of-vote date. It is one week, so let add it :) > It's not clear what > the RFCs path forward is. (If this info is in a mail thread, but not > in the RFC then remember readers/voters should not have to trawl > internals mail to understand the proposal and its direction). > > Personally, I suggest the vote be closed/withdrawn with the assumption > the concept was accepted 15 to 1. Then work on the code until a > mutually acceptable and useful implementation is found. After that, a > quick vote can be made on the implementation. We do not have to over react here, it is, for a change, that there is clear concensus about the need or wish for this feature. It is not a trivial thing to implement but we have time to make it rock solid until final 5.6.0. Cheers, -- Pierre @pierrejoye | http://www.libgd.org --- ## #72327 — Stas Malyshev — 2014-02-06T07:01:36+00:00 URL: https://externals.io/message/72018#72327 · In reply to: #72320 Hi! > We do not have to over react here, it is, for a change, that there is > clear consensus about the need or wish for this feature. It is not a > trivial thing to implement but we have time to make it rock solid > until final 5.6.0. There's a consensus about the feature as it was proposed, but when all kind of things start to be added to it, that eventually becomes a different feature from one that was voted on. If the proposal is not ready, then the vote should be delayed. If it's ready then the constant stream of changes, tweaks and additions looks strange - it's really hard to know what the vote is actually about. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #72328 — Pierre Joye — 2014-02-06T07:09:06+00:00 URL: https://externals.io/message/72018#72328 · In reply to: #72327 > Hi! > > > […] > > There's a consensus about the feature as it was proposed, For what I see there is one: https://wiki.php.net/rfc/timing_attack > but when all > kind of things start to be added to it, that eventually becomes a > different feature from one that was voted on. What I mean is that the need of a function to do hash equality tests with time attacks protection got a positive consensus. The implementation details are indeed subject to change and will certainly get updates until 5.6.0 final, and surely afterwards as well. > If the proposal is not > ready, then the vote should be delayed. If it's ready then the constant > stream of changes, tweaks and additions looks strange - it's really hard > to know what the vote is actually about. There are a couple of caveats that need to be solved. Not sure it needs a new vote but that should not be a problem to redo it. My point is that such feature seems to be desired for 5.6 and we should try to make sure it gets in, as long as it is possible. Cheers, -- Pierre @pierrejoye | http://www.libgd.org --- ## #72351 — Yasuo Ohgaki — 2014-02-06T23:05:05+00:00 URL: https://externals.io/message/72018#72351 · In reply to: #72018 Hi Rouven, I made SipHash version of str_compare() as a sample. There is timing safe php_compare(), which is stolen from BSD. https://github.com/yohgaki/php-src/compare/PHP-5.6-rfc-hash-compare [yohgaki@dev github-php-src]$ ./php-bin -r 'var_dump(str_compare("abc", "abc"));' bool(true) [yohgaki@dev github-php-src]$ ./php-bin -r 'var_dump(str_compare("asfasdf", "slkjojoeiwrj"));' bool(false) It's quick patch made less than 30 min. So it can be improved, I suppose. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- Next page: https://externals.io/message/72018.md?page=2