Hi all,
There's a pending GitHub pull request of mine to include a HKDF
implementation into ext/hash.
Mostly anybody who saw it agreed that it probably doesn't require an RFC
vote, but I hadn't originally announced it here on the list either, so this
is what I'm doing now ...
For technical details, I'd say it is best to read IETF RFC 5869, which
defines it, but here's the TL;DR version:
- HKDF stands for "HMAC-based Key Derivation Function"
- Useful in constructing encryption schemes, most notably to derive
separate keys for encryption, authentication using only a single input key.
Unless you're doing that, you probably don't care about it. - Unlike e.g. PBKDF2, it is supposed to be fast (as it's not a
password-based KDF), making it great for encryption/decryption on the fly
in web applications
There's one thing that may be contentious - whether to call it hkdf() or
hash_hkdf(); there are valid reasons for both and that's what I'd like to
discuss mostly, as everything else boils down to just a tumbs up/down for
the entire thing. But of course, I appreciate all feedback. :)
GitHub PR: https://github.com/php/php-src/pull/1105
IETF RFC: https://tools.ietf.org/html/rfc5869
P.S.: The PR was submitted a long time ago - almost 2 years - thanks to Joe
for bumping it up.
Cheers,
Andrey.
Hi all,
Hi all,
There's a pending GitHub pull request of mine to include a HKDF
implementation into ext/hash.
Mostly anybody who saw it agreed that it probably doesn't require an RFC
vote, but I hadn't originally announced it here on the list either, so this
is what I'm doing now ...For technical details, I'd say it is best to read IETF RFC 5869, which
defines it, but here's the TL;DR version:
- HKDF stands for "HMAC-based Key Derivation Function"
- Useful in constructing encryption schemes, most notably to derive
separate keys for encryption, authentication using only a single input key.
Unless you're doing that, you probably don't care about it.- Unlike e.g. PBKDF2, it is supposed to be fast (as it's not a
password-based KDF), making it great for encryption/decryption on the fly
in web applicationsThere's one thing that may be contentious - whether to call it hkdf() or
hash_hkdf(); there are valid reasons for both and that's what I'd like to
discuss mostly, as everything else boils down to just a tumbs up/down for
the entire thing. But of course, I appreciate all feedback. :)GitHub PR: https://github.com/php/php-src/pull/1105
IETF RFC: https://tools.ietf.org/html/rfc5869P.S.: The PR was submitted a long time ago - almost 2 years - thanks to Joe
for bumping it up.
Nice function, I like it.
Modified patch is committed to master
http://git.php.net/?p=php-src.git;a=commitdiff;h=
4bf7ef08061720586cb0a2f410720e26719d97f3
I have 2 improvement ideas.
- 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.
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. -
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.
- 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.
-
Warn obsolete hash function usage. e.g. md2()
-
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) -
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.
Comments are appreciated.
If there is no comment, I'll work on these improvements hopefully soon.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
Comments are appreciated.
If there is no comment, I'll work on these improvements hopefully soon.
Forgot to mention one more thing.
I also would like to make HKDF a PHPAPI.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
Hi all,
There's a pending GitHub pull request of mine to include a HKDF
implementation into ext/hash.
Mostly anybody who saw it agreed that it probably doesn't require an RFC
vote, but I hadn't originally announced it here on the list either, so
this
is what I'm doing now ...For technical details, I'd say it is best to read IETF RFC 5869, which
defines it, but here's the TL;DR version:
- HKDF stands for "HMAC-based Key Derivation Function"
- Useful in constructing encryption schemes, most notably to derive
separate keys for encryption, authentication using only a single input
key.
Unless you're doing that, you probably don't care about it.- Unlike e.g. PBKDF2, it is supposed to be fast (as it's not a
password-based KDF), making it great for encryption/decryption on the fly
in web applicationsThere's one thing that may be contentious - whether to call it hkdf() or
hash_hkdf(); there are valid reasons for both and that's what I'd like to
discuss mostly, as everything else boils down to just a tumbs up/down for
the entire thing. But of course, I appreciate all feedback. :)GitHub PR: https://github.com/php/php-src/pull/1105
IETF RFC: https://tools.ietf.org/html/rfc5869P.S.: The PR was submitted a long time ago - almost 2 years - thanks to
Joe
for bumping it up.Nice function, I like it.
Modified patch is committed to master
http://git.php.net/?p=php-src.git;a=commitdiff;h=
4bf7ef08061720586cb0a2f410720e26719d97f3I have 2 improvement ideas.
- 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.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.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.
Making the salt required makes no sense to me.
HKDF has a number of different applications:
a) Derive multiple strong keys from strong keying material. Typical case
for this is deriving independent encryption and authentication keys from a
master key. This requires only specification of $length. A salt is neither
necessary nor useful in this case, because you start with strong
cryptographic keying material.
b) Generating per-session (or similar) keys from a (strong cryptographic)
master key. For this purpose you can specify the $info parameter. again, a
salt is neither necessary nor useful in this case. (You could probably also
use $salt instead of $info in this case, but the design of the function
implies that $info should be used for this purpose.)
c) Extracting strong cryptographic keying material from weak cryptographic
keying material. Standard example here is extracting strong keys from DH
g^xy values (which are non-uniform) and similar. This is the usage that
benefits from a $salt.
d) Combinations thereof.
Remember that HKDF is an extract-and-expand algorithm, and the extract step
(which uses the salt) is only necessary if the input keying material is
weak. We always include the extract step for compatibility with the overall
HKDF construction (per the RFCs recommendation), but it's essentially just
an unnecessary operation if you work on strong keying material.
The only thing that we may want to discuss is whether we should swap the
$info and the $salt parameters. This depends on which usage (b or c) we
consider more likely.
- 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.
Warn obsolete hash function usage. e.g. md2()
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)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.
The blacklist is an intermediate solution for PHP 7.1. For master the
information on whether a hash function is cryptographic will be integrated
into the hash ops and we will check against this value. The check will also
be expanded to HMAC and PBKDF2.
Note that the reason we're doing these checks is not to discourage use of
obsolete hash functions, but because the HMAC construction has certain
requirements on the hash function, which are generally not satisfied by
non-cryptographic hashes. In particular, HMAC requires a block-based hash
where the block size is greater than or equal to the digest size.
Nikita
Hi Nikita,
Making the salt required makes no sense to me.
It does not make sense to me too if API requires a "salt value" always.
"salt" should be able to be omitted.
My proposal is to "make salt parameter required" and "let programmers
specify not to use salt explicitly".
HKDF has a number of different applications:
a) Derive multiple strong keys from strong keying material. Typical case
for this is deriving independent encryption and authentication keys from a
master key. This requires only specification of $length. A salt is neither
necessary nor useful in this case, because you start with strong
cryptographic keying material.
b) Generating per-session (or similar) keys from a (strong cryptographic)
master key. For this purpose you can specify the $info parameter. again, a
salt is neither necessary nor useful in this case. (You could probably also
use $salt instead of $info in this case, but the design of the function
implies that $info should be used for this purpose.)
c) Extracting strong cryptographic keying material from weak cryptographic
keying material. Standard example here is extracting strong keys from DH
g^xy values (which are non-uniform) and similar. This is the usage that
benefits from a $salt.
d) Combinations thereof.Remember that HKDF is an extract-and-expand algorithm, and the extract
step (which uses the salt) is only necessary if the input keying material
is weak. We always include the extract step for compatibility with the
overall HKDF construction (per the RFCs recommendation), but it's
essentially just an unnecessary operation if you work on strong keying
material.
I'm aware of use cases, but your explanation is mandatory for discussion so
that everyone understand it. Thank you.
The only thing that we may want to discuss is whether we should swap the
$info and the $salt parameters. This depends on which usage (b or c) we
consider more likely.
Swapping $info and $salt parameter order will encourage users to use $salt
when this is possible.
I would recommend $salt use whenever possible. It is possible for many use
cases. i.e. Modern applications set random values for security reasons and
developers should set it for HKDF also.
Developers are better to have secret salt whenever it is possible.
Consider following following attack scenario:
-
password_hash()output is used for $ikm -
hash_hkdf()output without salt is used for encryption key - app has SQL injection vulnerability and password hashes are stolen
Attackers are easily decrypt encrypted data by generating key from password
hash
because salt is not used.
Apps can have additional security by using salt defined as configuration
parameter.
IMO, this kind of usage would be the most common usage with PHP.
Therefore, making salt a required parameter encourages users to adopt
better security.
- 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.
Warn obsolete hash function usage. e.g. md2()
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)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.The blacklist is an intermediate solution for PHP 7.1. For master the
information on whether a hash function is cryptographic will be integrated
into the hash ops and we will check against this value. The check will also
be expanded to HMAC and PBKDF2.
Sounds good.
Note that the reason we're doing these checks is not to discourage use of
obsolete hash functions, but because the HMAC construction has certain
requirements on the hash function, which are generally not satisfied by
non-cryptographic hashes. In particular, HMAC requires a block-based hash
where the block size is greater than or equal to the digest size.
"md2" for HKDF does not make sense, but we may document that stronger hash
provide more security than weaker one.
Let's recommend SHA-3 in document. I prefer to warn weak/obsolete hash
usage to users, but documentation would work also.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
Nice function, I like it.
Modified patch is committed to master
http://git.php.net/?p=php-src.git;a=commitdiff;h=4bf7ef08061
720586cb0a2f410720e26719d97f3I have 2 improvement ideas.
- 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.
- 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.
- 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.
- 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.
- 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.
Hi Andrey,
Nice function, I like it.
Modified patch is committed to master
http://git.php.net/?p=php-src.git;a=commitdiff;h=4bf7ef08061
720586cb0a2f410720e26719d97f3I have 2 improvement ideas.
- 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.
- 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 anE_WARNINGand
doesn't actually block the functionality, for BC reasons.
- 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.
- 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
yohgaki@ohgaki.net
Hi all,
Nice function, I like it.
Modified patch is committed to master
http://git.php.net/?p=php-src.git;a=commitdiff;h=4bf7ef08061
720586cb0a2f410720e26719d97f3I have 2 improvement ideas.
- 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.
- 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 anE_WARNINGand
doesn't actually block the functionality, for BC reasons.
- 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.
- 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()
returnsFALSEfor 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_ERRORis 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 anduniqid()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:
- Get password hash for the user
- Generate new key with 1 using HKDF
- 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
Hi all,
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
720586cb0a2f410720e26719d97f3I have 2 improvement ideas.
- 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.
- 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 anE_WARNING
and
doesn't actually block the functionality, for BC reasons.
- 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.
- 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()
returnsFALSEfor 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_ERRORis 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 anduniqid()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:
- Get password hash for the user
- Generate new key with 1 using HKDF
- 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
Hi Nikita,
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 onmt_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.
This is because there is no logical explanation why against to have salt.
Internet RFC clearly states the benefits. Moreover, it recommends salt
whenever
it is possible by emphasizing improved security by salt. Or am I
misunderstood the RFC?
There isn't any valid reason to have "salt" parameter as the last optional
parameter so far.
Why it should be the last optional parameter?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
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 onmt_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.This is because there is no logical explanation why against to have salt.
Internet RFC clearly states the benefits. Moreover, it recommends salt
whenever
it is possible by emphasizing improved security by salt. Or am I
misunderstood the RFC?There isn't any valid reason to have "salt" parameter as the last optional
parameter so far.
Why it should be the last optional parameter?
BTW, I think the other 2 parameters should be optional.
string hash_hkdf(string algo, string ikm [, int length = 0, string info =
'', string salt = ''])
However, salt must be 1st optional parameter at least.
Considering most usage with PHP, it should be required and make it optional
explicitly.
In addition, I would like to have "info" as the first optional parameter.
string hash_hkdf(string algo, string ikm, string salt [, string info = '',
int length = 0])
- Set salt to
NULLif salt cannot be used, reject null string as invalid.
Better security by default is the way to go. IMHO.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
string hash_hkdf(string algo, string ikm [, int length = 0, string info =
'', string salt = ''])
I copied and pasted this proto from the source.
It's wrong. Correct one is
string hash_hkdf(string algo, string ikm [, int length = 0 [, string info =
'' [, string salt = '']]])
so my proposal is
string hash_hkdf(string algo, string ikm, string salt [, string info = ''
[, int length = 0]])
- Set salt to
NULLif salt cannot be used, reject null string as invalid.
I will change my mind if there is convincing logical
reason(s) not to do this. I suppose there isn't.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
I will change my mind if there is convincing logical
reason(s) not to do this. I suppose there isn't.
Not only are you ignoring everybody else's arguments, but also behaving
like PHP is your own personal project.
Please stop.
Cheers,
Andrey.
I will change my mind if there is convincing logical
reason(s) not to do this. I suppose there isn't.Not only are you ignoring everybody else's arguments, but also behaving
like PHP is your own personal project.
Please stop.
I would say, please stop posting nonsense replay.
Your argument does not make sense.
If you think your argument is sane.
Please reply the other thread that I'm questioning why
you insist and recommend vulnerable usage this much.
Thank you.
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
Did everyone understand why I propose salt as required parameter and
specify optional salt explicitly?
I did, and I disagreed.
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:
- Get password hash for the user
- Generate new key with 1 using HKDF
- Use key produced by 2 for encryption/etc
No it's not. That's the first thing you could think of, searching for a
problem to solve with it.
If you search for it on GitHub, you'll see the most common scenario is to
derive a pair of keys for encryption and HMAC.
(yes, there are PHP projects using it)
Cheers,
Andrey.
Hi all,
There's a pending GitHub pull request of mine to include a HKDF
implementation into ext/hash.
Mostly anybody who saw it agreed that it probably doesn't require an RFC
vote, but I hadn't originally announced it here on the list either, so this
is what I'm doing now ...For technical details, I'd say it is best to read IETF RFC 5869, which
defines it, but here's the TL;DR version:
- HKDF stands for "HMAC-based Key Derivation Function"
- Useful in constructing encryption schemes, most notably to derive
separate keys for encryption, authentication using only a single input key.
Unless you're doing that, you probably don't care about it.- Unlike e.g. PBKDF2, it is supposed to be fast (as it's not a
password-based KDF), making it great for encryption/decryption on the fly
in web applicationsThere's one thing that may be contentious - whether to call it hkdf() or
hash_hkdf();
Should be definitely hash_hkdf as it's part of the hash extension (that's
why hash prefix) and we might add openssl_hkdf that will use OpenSSL
implementation added to 1.1 - it would also use OpenSSL implemention for
underlaying hash implementation that is more powerful in some ways (e.g.
ASM optimization of some main hash functions).
Cheers
Jakub