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

practicalswift · 2017-10-17T14:44:49Z

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).~~

sipa · 2017-10-18T09:16:34Z

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

practicalswift · 2017-10-18T10:15:47Z

@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 :-)

laanwj · 2017-10-18T13:29:05Z

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.

practicalswift · 2017-10-18T15:26:21Z

src/crypto/hmac_sha256.cpp

-        memset(rkey + keylen, 0, 64 - keylen);
+        if (keylen != 64) {
+            memset(rkey + keylen, 0, 64 - keylen);
+        }


Removing this change as requested.

practicalswift · 2017-10-18T15:26:32Z

src/crypto/hmac_sha512.cpp

-        memset(rkey + keylen, 0, 128 - keylen);
+        if (keylen != 128) {
+            memset(rkey + keylen, 0, 128 - keylen);
+        }


Removing this change as requested.

…+/- 1 byte for HMAC-SHA256 and 128 bytes +/- 1 byte for HMAC-SHA512

practicalswift · 2017-10-18T15:30:17Z

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

laanwj · 2017-10-19T16:11:42Z

ACK a3f5657

practicalswift · 2017-10-30T10:40:48Z

Ready for merge? :-)

…56,512} key length boundaries a3f5657 Add test cases covering the relevant key length boundaries: 64 bytes +/- 1 byte for HMAC-SHA256 and 128 bytes +/- 1 byte for HMAC-SHA512 (practicalswift) Pull request description: * 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).~~ Tree-SHA512: 48ff9ab79d41aab97b5b8f6496cc08a39955a07eb424f74ada6440d3b168b6204d3527fa677e175c47e40142f9d62c7456ae162e5a2f5b557e90fb353beef1d0

TheBlueMatt · 2017-12-12T18:38:56Z

Post-merge was-here-looks-fine.

…C-SHA{256,512} key length boundaries a3f5657 Add test cases covering the relevant key length boundaries: 64 bytes +/- 1 byte for HMAC-SHA256 and 128 bytes +/- 1 byte for HMAC-SHA512 (practicalswift) Pull request description: * 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).~~ Tree-SHA512: 48ff9ab79d41aab97b5b8f6496cc08a39955a07eb424f74ada6440d3b168b6204d3527fa677e175c47e40142f9d62c7456ae162e5a2f5b557e90fb353beef1d0

7a5d181 Use the character based overload for std::string::find. (Alin Rus) c19401b Don't use pass by reference to const for cheaply-copied types (bool, char, etc.). (practicalswift) 4c5fe36 [Refactor] Remove unused fQuit var from checkqueue.h (donaloconnor) fda7a5f Cleanup: remove unneeded header includes (random-zebra) ac2476c Add test cases covering the relevant key length boundaries: 64 bytes +/- 1 byte for HMAC-SHA256 and 128 bytes +/- 1 byte for HMAC-SHA512 (practicalswift) a346262 Fix code constness in CBlockIndex::GetAncestor() overloads (Dan Raviv) 65e3f4e Refactor: More range-based for loops (random-zebra) dd3d3c4 Use range-based for loops (C++11) when looping over map elements (practicalswift) 5a7750a Move RPC registration out of AppInitParameterInteraction (Russell Yanofsky) 402b4c4 Use compile-time constants instead of unnamed enumerations (practicalswift) bbac2d0 [Trivial] Fix indentation in coins.cpp (random-zebra) e539c62 Small refactor of CCoinsViewCache::BatchWrite() (Dan Raviv) ec91759 Use MakeUnique<T>(...) instead of std::unique_ptr<T>(new T(...)) (random-zebra) 93487b1 Use unique_ptr for pcoinscatcher/pcoinsdbview/pcoinsTip/pblocktree (random-zebra) ff43d69 Use unique_ptr for pdbCopy (Db) and fix potential memory leak (practicalswift) b4d9641 Use unique_ptr for dbenv (DbEnv) (practicalswift) 36108b9 Use unique_ptr for pfilter (CBloomFilter) (practicalswift) ff1c454 Use unique_ptr for sem{Addnode,Outbound} (CSemaphore) (practicalswift) 93daf17 Use unique_ptr for httpRPCTimerInterface (HTTPRPCTimerInterface) (practicalswift) 020ac16 Init: Remove redundant exit(EXIT_FAILURE) instances and replace with (random-zebra) b9f5d1f Remove duplicate uriParts.size() > 0 check (practicalswift) 440d961 Remove redundant check (!ecc is always true) (practicalswift) bfd295b Remove redundant NULL checks after new (practicalswift) 97aad32 Make fUseCrypto atomic (MeshCollider) 2711f78 Consistent parameter names in txdb.h (MeshCollider) d40df3a Fix race for mapBlockIndex in AppInitMain (random-zebra) 03b7766 Cleanup: remove unused functions to Hash the concat of 4 or more objects (random-zebra) c520e0f Remove some unused functions and methods (Pieter Wuille) 508f1a1 range-based loops and const qualifications in net.cpp (Marko Bencun) 79b1e50 Refactor: implement CPubKey::data() (random-zebra) 614d26c Refactor: more &vec[0] to vec.data() (random-zebra) 02b6337 Changing &vec[0] to vec.data(), what 9804 missed (MeshCollider) c1c8b05 Ensure that data types are consistent (jjz) 732bb9d Fix potential null dereferences (MeshCollider) 80f35f9 Remove unreachable code (practicalswift) Pull request description: This is a collection of simple refactorings coming from upstream Bitcoin v0.16 (adapting/extending to PIVX-specific code where needed). Pull requests backported: - bitcoin#10845 (practicalswift) - bitcoin#11238 (MeshCollider) - bitcoin#11232 (jjz) - bitcoin#10793 (MeshCollider) - bitcoin#10888 (benma) - ~~bitcoin#11351 (danra)~~ [edit: removed - included in #2423] - bitcoin#11385 (sipa) - bitcoin#11107 (MeshCollider) - bitcoin#10898 (practicalswift) - bitcoin#11511 (donaloconnor) - bitcoin#11043 (practicalswift) - bitcoin#11353 (danra) - bitcoin#10749 (practicalswift) - bitcoin#11603 (ryanofsky) - bitcoin#10493 (practicalswift) - bitcoin#11337 (danra) - bitcoin#11516 (practicalswift) - bitcoin#10574 (practicalswift) - bitcoin#12108 (donaloconnor) - bitcoin#10839 (practicalswift) - bitcoin#12159 (kekimusmaximus) ACKs for top commit: Fuzzbawls: ACK 7a5d181 furszy: re-ACK 7a5d181 after rebase, no code changes. Merging.. Tree-SHA512: d92f5df47f443391a6531274a2efb9a4882c62d32eff628f795b3abce703f108c8b40ec80ac841cde6c5fdd5c9ff2b6056a31546ac2edda279f5f18fccc99c32

practicalswift changed the title ~~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. Oct 17, 2017

practicalswift changed the title ~~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. Oct 18, 2017

laanwj added the Refactoring label Oct 18, 2017

practicalswift commented Oct 18, 2017

View reviewed changes

Add test cases covering the relevant key length boundaries: 64 bytes …

a3f5657

…+/- 1 byte for HMAC-SHA256 and 128 bytes +/- 1 byte for HMAC-SHA512

practicalswift force-pushed the one-past-the-end branch from a4a983d to a3f5657 Compare October 18, 2017 15:27

practicalswift changed the title ~~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 Oct 18, 2017

laanwj merged commit a3f5657 into bitcoin:master Dec 12, 2017

practicalswift deleted the one-past-the-end branch April 10, 2021 19:32

random-zebra mentioned this pull request Jun 29, 2021

[Refactoring] Bitcoin 0.16 cherries PIVX-Project/PIVX#2462

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

practicalswift commented Oct 17, 2017 •

edited

Loading

Uh oh!

sipa commented Oct 18, 2017

Uh oh!

practicalswift commented Oct 18, 2017 •

edited

Loading

Uh oh!

laanwj commented Oct 18, 2017 •

edited

Loading

Uh oh!

practicalswift Oct 18, 2017

Uh oh!

practicalswift Oct 18, 2017

Uh oh!

practicalswift commented Oct 18, 2017

Uh oh!

laanwj commented Oct 19, 2017

Uh oh!

practicalswift commented Oct 30, 2017

Uh oh!

TheBlueMatt commented Dec 12, 2017

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

practicalswift commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Oct 18, 2017

Uh oh!

practicalswift commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift Oct 18, 2017

Choose a reason for hiding this comment

Uh oh!

practicalswift Oct 18, 2017

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Oct 18, 2017

Uh oh!

laanwj commented Oct 19, 2017

Uh oh!

practicalswift commented Oct 30, 2017

Uh oh!

TheBlueMatt commented Dec 12, 2017

Uh oh!

Uh oh!

practicalswift commented Oct 17, 2017 •

edited

Loading

practicalswift commented Oct 18, 2017 •

edited

Loading

laanwj commented Oct 18, 2017 •

edited

Loading