Hi, 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. Salt is optional because RFC 5869 allows it to be optional. There's a reason for each of the current defaults work as they do, as well as the parameter positions: - Length is in no way actually described as optional, and that makes sense as the function's purpose is to create cryptographic keys, which by nature have fixed lengths. The only reason we could make Length optional is because hash functions' output sizes are known values, and matching the desired OKM length with the hash function size makes for better performance. - Info can be empty, but the algorithm is pretty much meaningless without it. The purpose of HKDF is to derive 2+ outputs from a single input, with the Info parameter serving as the differentiating factor. - Salt is ... while recommended, the only thing actually optional. Thus, as you can probably see - the one parameter that you want to make required is actually the only one that should be left optional. That being said, if anything is to be changed, I would be in favor of making *all* parameters required, but I mean that as either make them all required or leave it untouched; no middle ground. I just didn't originally feel that this was in PHP's "spirit" ... and again - this is a 2 year old PR. > RFC 5869 hkdf is stronger with known random salt, even stronger with secret > random salt. However, salt is the last optional option. This discourages > stronger hkdf usage. > > > Therefore, I would like to change the signature to > > > proto string hash_hkdf(string algo, string ikm, string salt [, int length = > 0, string info = '']) > > > - when salt is null, don't use salt. NOT recommended, but this > may need for compatibility with other systems. > Note: This is the same as undefined salt with current patch. > > - when salt is ''(empty string), use default static known random salt > value. > Note: hkdf's salt could be known, yet provide stronger result as RFC > states. > > 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. > - when salt is set, use the value. (The same as now) > > Note: ikm does not have to include random salt. Even when ikm includes > random value, secret salt improves security. > > > > 2) Add hash function whitelisting > > There is hash function blacklisting which disallows insecure usage. It's > good enough for now, but whitelisting would help in 3 ways. > The blacklist was added for technical reasons, more specifically because checksum hashes simply don't work in the same way as cryptographic ones; and there's no php_hash_ops flag to tell us if the hash function in use is cryptographic one or not. Yet ... I'll submit another PR to introduce that flag, and switch to a whitelist approach based on that. But while that does have the side-effect of enforcing better choices, "disallowing insecure usage" in the way you meant that is not an achievable goal. > > 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. > > > 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. Cheers, Andrey.
