Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:98152 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 2451 invoked from network); 3 Feb 2017 23:02:44 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 3 Feb 2017 23:02:44 -0000 Authentication-Results: pb1.pair.com header.from=yohgaki@ohgaki.net; sender-id=pass Authentication-Results: pb1.pair.com smtp.mail=yohgaki@ohgaki.net; spf=pass; sender-id=pass Received-SPF: pass (pb1.pair.com: domain ohgaki.net designates 180.42.98.130 as permitted sender) X-PHP-List-Original-Sender: yohgaki@ohgaki.net X-Host-Fingerprint: 180.42.98.130 ns1.es-i.jp Received: from [180.42.98.130] ([180.42.98.130:47784] helo=es-i.jp) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id 4D/59-38491-F0C05985 for ; Fri, 03 Feb 2017 18:02:41 -0500 Received: (qmail 38847 invoked by uid 89); 3 Feb 2017 23:02:35 -0000 Received: from unknown (HELO mail-qt0-f171.google.com) (yohgaki@ohgaki.net@209.85.216.171) by 0 with ESMTPA; 3 Feb 2017 23:02:35 -0000 Received: by mail-qt0-f171.google.com with SMTP id k15so59155096qtg.3 for ; Fri, 03 Feb 2017 15:02:35 -0800 (PST) X-Gm-Message-State: AMke39k7+E4fuH6cdouEekGPjs0vTmDIv+7Cfx9hDK7pRcPeTk8S57190Cn58Yyg+DLdjya6WhZ1f0B5YXGsdQ== X-Received: by 10.55.166.66 with SMTP id p63mr15623353qke.142.1486162949278; Fri, 03 Feb 2017 15:02:29 -0800 (PST) MIME-Version: 1.0 Received: by 10.140.19.232 with HTTP; Fri, 3 Feb 2017 15:01:48 -0800 (PST) In-Reply-To: References: Date: Sat, 4 Feb 2017 08:01:48 +0900 X-Gmail-Original-Message-ID: Message-ID: To: Andrey Andreev Cc: "internals@lists.php.net" Content-Type: multipart/alternative; boundary=94eb2c070ec8aba37b0547a84692 Subject: Re: [PHP-DEV] [Discussion] HKDF From: yohgaki@ohgaki.net (Yasuo Ohgaki) --94eb2c070ec8aba37b0547a84692 Content-Type: text/plain; charset=UTF-8 Hi all, On Tue, Jan 17, 2017 at 4:01 PM, Yasuo Ohgaki wrote: > On Mon, Jan 16, 2017 at 8:16 PM, Andrey Andreev wrote: > >> >> On Mon, Jan 16, 2017 at 3:47 AM, Yasuo Ohgaki 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 yohgaki@ohgaki.net --94eb2c070ec8aba37b0547a84692--