crypto: Add test cases covering the relevant HMAC-SHA{256,512} key length boundaries #11516

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:one-past-the-end changing 1 files +41 −0
  1. practicalswift commented at 2:44 PM on October 17, 2017: contributor
    • Add test cases covering the relevant key length boundaries: 64 bytes +/- 1 byte for HMAC-SHA256 and 128 bytes +/- 1 byte for HMAC-SHA512.
    • Avoid creating a one-past-the-end pointer in the case of key length 64 (HMAC-SHA256) and key length 128 (HMAC-SHA512).
    • Avoid performing a noop memset call (zero length argument) in the case of key length 64 (HMAC-SHA256) and key length 128 (HMAC-SHA512).
  2. practicalswift renamed this:
    crypto: Avoid creating of one-past-the-end pointer and performing a noop memset call (HMAC-SHA{256,512})
    crypto: Add HMAC-SHA{256,512} test cases for relevant boundaries. Avoid creating of one-past-the-end pointer and performing a noop memset call.
    on Oct 17, 2017
  3. practicalswift renamed this:
    crypto: Add HMAC-SHA{256,512} test cases for relevant boundaries. Avoid creating of one-past-the-end pointer and performing a noop memset call.
    crypto: Add HMAC-SHA{256,512} test cases for relevant key length boundaries. Avoid creating of one-past-the-end pointer and performing a noop memset call.
    on Oct 17, 2017
  4. practicalswift renamed this:
    crypto: Add HMAC-SHA{256,512} test cases for relevant key length boundaries. Avoid creating of one-past-the-end pointer and performing a noop memset call.
    crypto: Avoid creating of one-past-the-end pointer and performing a noop memset call. Add HMAC-SHA{256,512} test cases.
    on Oct 17, 2017
  5. practicalswift renamed this:
    crypto: Avoid creating of one-past-the-end pointer and performing a noop memset call. Add HMAC-SHA{256,512} test cases.
    crypto: Avoid creating an one-past-the-end pointer and performing a noop memset call. Add HMAC-SHA{256,512} test cases.
    on Oct 18, 2017
  6. sipa commented at 9:16 AM on October 18, 2017: member

    Is this necessary? There is no problem with creating a one-past-end pointer; only dereferencing them, which doesn't happen here.

  7. practicalswift commented at 10:15 AM on October 18, 2017: contributor

    @sipa Both zero length memset:s and creating of one-past-end pointers are legal, so from a language legality sense this change is not needed.

    However, I've seen multiple static analyzers flag this line of code as a potential mistake. This small change makes it apparent for both static analyzers and humans what the real intention of the code is.

    My experience is that code that looks like potential mistakes to static analyzers tend to confuse humans too. I think there is a good case for helping static analyzers (and perhaps humans too?) in this case. It also has the added benefit of getting rid of an unnecessary memset call and getting three additional test cases that cover the key length boundaries.

    In summary I think this change is worth it :-)

  8. laanwj commented at 1:29 PM on October 18, 2017: member

    Pointers to the end of a range are absolutely valid C/C++. This is commonly used for iteration end-exclusive ranges. So is a zero-length memset.

    I tend towards NACK adding unnecessary checks here. I think minor harmless changes to fix static analyzer false-positives are sometimes okay, but at some point it goes too far, we don't write our code to appease static analyzers. This kind of ruffling the code can introduce bugs.

    ACK on the tests though.

  9. laanwj added the label Refactoring on Oct 18, 2017
  10. in src/crypto/hmac_sha256.cpp:16 in a4a983db74 outdated 
      10 | @@ -11,7 +11,9 @@ CHMAC_SHA256::CHMAC_SHA256(const unsigned char* key, size_t keylen)
  11 |      unsigned char rkey[64];
  12 |      if (keylen <= 64) {
  13 |          memcpy(rkey, key, keylen);
  14 | -        memset(rkey + keylen, 0, 64 - keylen);
  15 | +        if (keylen != 64) {
  16 | +            memset(rkey + keylen, 0, 64 - keylen);
  17 | +        }
    practicalswift commented at 3:26 PM on October 18, 2017:

    Removing this change as requested.

  11. Add test cases covering the relevant key length boundaries: 64 bytes +/- 1 byte for HMAC-SHA256 and 128 bytes +/- 1 byte for HMAC-SHA512 a3f56578ab
  12. in src/crypto/hmac_sha512.cpp:16 in a4a983db74 outdated 
      10 | @@ -11,7 +11,9 @@ CHMAC_SHA512::CHMAC_SHA512(const unsigned char* key, size_t keylen)
  11 |      unsigned char rkey[128];
  12 |      if (keylen <= 128) {
  13 |          memcpy(rkey, key, keylen);
  14 | -        memset(rkey + keylen, 0, 128 - keylen);
  15 | +        if (keylen != 128) {
  16 | +            memset(rkey + keylen, 0, 128 - keylen);
  17 | +        }
    practicalswift commented at 3:26 PM on October 18, 2017:

    Removing this change as requested.

  13. practicalswift force-pushed on Oct 18, 2017
  14. practicalswift renamed this:
    crypto: Avoid creating an one-past-the-end pointer and performing a noop memset call. Add HMAC-SHA{256,512} test cases.
    crypto: Add test cases covering the relevant key length boundaries: 64 bytes +/- 1 byte for HMAC-SHA256 and 128 bytes +/- 1 byte for HMAC-SHA512
    on Oct 18, 2017
  15. practicalswift renamed this:
    crypto: Add test cases covering the relevant key length boundaries: 64 bytes +/- 1 byte for HMAC-SHA256 and 128 bytes +/- 1 byte for HMAC-SHA512
    crypto: Add test cases covering the relevant HMAC-SHA{256,512} key length boundaries
    on Oct 18, 2017
  16. practicalswift commented at 3:30 PM on October 18, 2017: contributor

    @sipa @laanwj PR updated - now keeping only the new test cases covering the key length boundaries :-)

  18. practicalswift commented at 10:40 AM on October 30, 2017: contributor

    Ready for merge? :-)

  19. laanwj merged this on Dec 12, 2017
  20. laanwj closed this on Dec 12, 2017
  21. laanwj referenced this in commit 0e722e8879 on Dec 12, 2017
  22. TheBlueMatt commented at 6:38 PM on December 12, 2017: member

    Post-merge was-here-looks-fine.

  23. PastaPastaPasta referenced this in commit 6ae9e79dd8 on Jan 17, 2020
  24. PastaPastaPasta referenced this in commit 8673df90f2 on Jan 22, 2020
  25. PastaPastaPasta referenced this in commit ff5f85a8ad on Jan 22, 2020
  26. PastaPastaPasta referenced this in commit 1bc010ccf8 on Jan 29, 2020
  27. PastaPastaPasta referenced this in commit 70936fdd40 on Jan 29, 2020
  28. PastaPastaPasta referenced this in commit 7737eb48e7 on Jan 29, 2020
  29. PastaPastaPasta referenced this in commit b695b67d5f on Jan 31, 2020
  30. ckti referenced this in commit 33c310f075 on Mar 28, 2021
  31. practicalswift deleted the branch on Apr 10, 2021
  32. furszy referenced this in commit 60d36292bc on Jul 26, 2021
  33. DrahtBot locked this on Aug 16, 2022
Contributors
practicalswiftsipalaanwjTheBlueMatt
Labels
Refactoring
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-16 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me