fix HMAC_DRBG edge case #3165
fix HMAC_DRBG edge case #3165
Conversation
keks
requested review from
kostko,
peterjgilbert,
pro-wh and
Yawning
as code owners
Codecov Report
@@ Coverage Diff @@
## master #3165 +/- ##
==========================================
+ Coverage 68.48% 68.72% +0.24%
==========================================
Files 386 386
Lines 37962 37962
==========================================
+ Hits 25997 26091 +94
+ Misses 8658 8559 -99
- Partials 3307 3312 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thank you for your contribution! The fix seems to make sense, @Yawning can you also take a quick look?
A few minor nits also flagged by the linter:
- The Git commit messages should be formatted as specified in the guidelines.
- You should add a changelog fragment to
.changelogdescribing the change.
|
I duplicated the tests and in one of them use Finally, I'm not 100% sure if any part of Oasis uses an empty slice somewhere in it's code, so I left a TODO in the fragment. I can update it if you have a definite answer for me. |
No, we don't, and in fact, there is no way for the edge case to be triggered. Since the affected routines are package private, it's fairly easy to confirm this via manual review.
|
|
I briefly skimmed the output of I feel like the URLs are helpful in the commit messages so I'm hesitant to remove them in order to make the linter happy. How do you want to proceed? |
Good point, I just fixed the linter in #3168, once that's merged, you can rebase and the linter should pass. I'll also trigger the non-lint CI workflow again after your rebase. |
The NIST SP 800-90A[1] spec of HMAC_DRBG uses "!= Null" checks,
but "Null" is never defined in the document. In the current
implementation, this was interpreteted as nil.
However, the NIST examples document[2] uses "<empty>" values,
so []byte{} should be passed in the test cases. If that happens,
the nil checks do not work as expected. Therefore, instead of checking
equality with nil, it should be checked whether the length of
additional_info is zero.
As additional evidence I cite the openssl implementation[3], which checks
for zero length instead of NULL pointer equality.
[1]: https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-90Ar1.pdf
[2]: https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Standards-and-Guidelines/documents/examples/HMAC_DRBG.pdf
[3]: https://github.com/openssl/openssl/pull/6779/files#diff-c44c9681c4fc49b1d7e9e74578085212R77-R79
|
Sorry this took so long. I think the linter regex doesn't like the hash fragment. On line 10, it says: Note that the third bracket after Do you want to fix the regex or should we skip the more precise reference and use a link without hash fragment? |
|
Aw sorry, I only tested with the first URL. Feel free to fix the regexp in this PR, just in a separate commit. |
|
Thanks again! I also triggered the other CI workflow to run now. |
The NIST SP 800-90A spec of HMAC_DRBG uses "
!= Null" checks, but "Null" is never defined in the document. In the current implementation, it was interpreted asnil.However, the NIST examples document uses "<empty>" values, so[]byte{}should be passed in the test cases. If that happens, thenilchecks do not work as expected. Therefore, instead of checking equality withnil, it should be checked whether the length ofadditional_infois zero.As additional evidence I cite the openssl implementation, which checks for zero length instead of NULL pointer equality.
I do not expect this to have a concrete security impact, but still it would be nice if the implementation was correct.