Hi Andrey, On Mon, Jan 16, 2017 at 8:16 PM, Andrey Andreev <n...@devilix.net> wrote:
> > On Mon, Jan 16, 2017 at 3:47 AM, Yasuo Ohgaki <yohg...@ohgaki.net> wrote: > >> Nice function, I like it. >> Modified patch is committed to master >> http://git.php.net/?p=php-src.git;a=commitdiff;h=4bf7ef08061 >> 720586cb0a2f410720e26719d97f3 >> >> I have 2 improvement ideas. >> >> >> >> 1) Make salt parameter required. >> >> Current hash_hkdf() has this signature. >> >> proto string hash_hkdf(string algo, string ikm [, int length = 0, string >> info = '', string salt = '']) >> >> I posted inline comment to PR so that make hkdf stronger, but it seems it >> was overlooked. >> >> RFC 5869 Section 3.1 states >> >> HKDF is defined to operate with and without random salt. This is >> done to accommodate applications where a salt value is not available. >> We stress, however, that the use of salt adds significantly to the >> strength of HKDF, ensuring independence between different uses of the >> hash function, supporting "source-independent" extraction, and >> strengthening the analytical results that back the HKDF design. >> >> >> *SNIP >> >> It is worth noting that, while not the typical case, some >> applications may even have a secret salt value available for use; in >> such a case, HKDF provides an even stronger security guarantee. >> >> >> > There's no comment from you on the PR, inline or not, but I can assure you > this was not overlooked. > I did inline comment. Nikita remembered at least. It seems I didn't get notification email, though. If it is static and known - it is not random. On the other hand, if you > wanted to generate it on the fly - it would be unusable as the output > wouldn't be reproducible > Section 3.1 of RC 5869 that you quoted above is advice for application > developers; it is NOT a description of how the function should be > implemented as a primitive. > It is only for general dictionary not to work. I don't mind at all not to have static hard coded salt. Let's not have it. 1. Warn obsolete hash function usage. e.g. md2() >> >> > I'd be in favor of such a warning, but only if 1) we're talking about ALL > of ext/hash and not just this function; and 2) it's just an E_WARNING and > doesn't actually block the functionality, for BC reasons. > > >> 2. When new hash function is added and blacklist update is >> forgotten, whitelist mitigate it. >> (We are better to have .phpt for it to notify problem to new hash >> function author) >> >> > This is a 2-edged sword that can also restrict usage of newly introduced > but *more secure* hashes - I don't think it's a good idea. > > >> 3. Raise E_RECOVEREABLE _ERROR for blacklisted hashes. >> Note: Blacklisted hash usage results in returning FALSE. This may >> result in very weak encryption/etc with @ operator, for example. >> >> > Depends on what you mean ... For non-cryptographic hashes - meh, I don't > care; for those that are considered obsolete - no thanks. > Letting not to use invalid hashes (E_RECOVERABLE_ERROR) make sense because it is new function. As time goes by, we may have additional blacklisted hashes. hash_hkdf() returns FALSE for bad hashes and the return value must never be used as key. Since the return value must never be used, users must update code regardless of raised error type. Therefore, E_RECOVERABLE _ERROR makes sense. It prevents sloppy error event fix like '@hash_hkdf()' also. More secure hash must be listed in the whitelist. Properly written .phpt will help new hash function authors to notice not updated whitelist/blacklist. I'm not going to add whitelist since it could be handled in other places. i.e. Nikita's post. Let's forget about whitelisting. Issue is error type for blacklist error. Do you still think E_RECOVERABLE_ERROR is better? > > > >> >> >> Comments are appreciated. >> If there is no comment, I'll work on these improvements hopefully soon. >> >> > This is not the first time you go ahead with your ideas like that ... > There have been BC breaks in ext/session because of this approach, and I > remember at least one instance of an RM having to revert your frivolous > commits. > Please don't do that. > I don't aware of ext/session issue, but there was uniqid() issue. uniqid()'s entropy discussion was spoiled by totally wrong FUD and patch was reverted. I restarted the discussion recently and uniqid() will have proper entropy for it soon because nobody objects use of better PRNG this time. Anyway, I wrote why we should make "salt" a required parameter in reply to NIkita's post. If you still think it should not, please explain the reason why. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net
