Hi all, On Tue, Jan 17, 2017 at 4:01 PM, Yasuo Ohgaki <yohg...@ohgaki.net> wrote:
> 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. > Did everyone understand why I propose salt as required parameter and specify optional salt explicitly? HKDF w/o salt is OK, but with salt, it's much stronger than w/o it. In addition, most use case with PHP is something like as follows: 1. Get password hash for the user 2. Generate new key with 1 using HKDF 3. Use key produced by 2 for encryption/etc If there is no salt in 2, simple SQL injection will allow attackers to decrypt all of encrypted users data. Note: Salt should be stored other place. e.g. In config file, other db, etc. I'll prepare patch if there is no more comments. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net
