Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:98156
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.213.174 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAGa2bXaOtoH+6HfZZwEYFZbaqXaN-xfw19rhHUcwhD=Hu0AOZA@mail.gmail.com>
References: <CAPhkiZz+eHFEYFA_4yCbTeBZ8KtugbO_Vw9acERo9G+hWYGteQ@mail.gmail.com>
 <CAGa2bXaEXXeFRtie8o9uMByen9uZZbRZCxGo19dH-nZ=ZV=w1w@mail.gmail.com>
 <CAPhkiZzVEmrwp2iLukqg1Lrj_DJm=c7Ew4JnE8Vb94_a4us4DQ@mail.gmail.com>
 <CAGa2bXbFEzjb4pNHVdHyy+rm85h4pMcSM0wp3R7oLRinfB_oHA@mail.gmail.com> <CAGa2bXaOtoH+6HfZZwEYFZbaqXaN-xfw19rhHUcwhD=Hu0AOZA@mail.gmail.com>
Date: Sat, 4 Feb 2017 00:56:46 +0100
Message-ID: <CAF+90c9MSZiCWHdW=EUL_2wm9kV34fMrwj=m_qW2Srygm1oe2A@mail.gmail.com>
To: Yasuo Ohgaki <yohgaki@ohgaki.net>
Cc: Andrey Andreev <narf@devilix.net>, "internals@lists.php.net" <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=f403045dc5d2dd1b070547a9083c
Subject: Re: [PHP-DEV] [Discussion] HKDF
From: nikita.ppv@gmail.com (Nikita Popov)

--f403045dc5d2dd1b070547a9083c
Content-Type: text/plain; charset=UTF-8

On Sat, Feb 4, 2017 at 12:01 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Hi all,
>
> On Tue, Jan 17, 2017 at 4:01 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>
> > On Mon, Jan 16, 2017 at 8:16 PM, Andrey Andreev <narf@devilix.net>
> wrote:
> >
> >>
> >> On Mon, Jan 16, 2017 at 3:47 AM, Yasuo Ohgaki <yohgaki@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.
>

You are free to prepare a patch, but your patch will not get merged.

Your blatant disregard of any and all feedback you receive on your
proposals is beginning to get on my nerves. This has played out again and
again, most recently in the thread on mt_rand() seeding. Here again, you
make a suggestion, you get two responses, both telling you that your
suggestion is not acceptable, and what conclusion do you draw from this?
Why, of course, let's land it anyway!

If people stop replying to your mails, the reason is not that they have
been convinced by your arguments. The reason is that they have realized the
pointlessness of the debate.

Regards,
Nikita

--f403045dc5d2dd1b070547a9083c--