Cryptopp: Keccak256 and SHA3 crash when input is 0 length

[akovachev]

Keccak256 and SHA3 not working as intended

• State the operating system and version (Ubutnu 17 x86_64, Windows 7 Professional x64, etc)
  Mac OS

• State the version of the Crypto++ library (Crypto++ 5.6.5, Master, etc)
  Crypto++ 7.0.0

• State how you built the library (Makefile, Cmake, distro, etc)
  Bazel

• Show a typical command line (the output of the compiler for cryptlib.cpp)

• Show the link command (the output of the linker for libcryptopp.so or cryptest.exe)
• Show the exact error message you are receiving (copy and paste it); or

Assertion failed: external/cryptopp/keccak.cpp(255): Update
Trace/BPT trap: 5

Assertion failed: external/cryptopp/sha3.cpp(254): Update
Trace/BPT trap: 5

• Clearly state the undesired behavior (and state the expected behavior)

Both SHA3 and Keccak256 have valid test vectors with 0-length input as follows:

SHA3("") = A7FFC6F8BF1ED76651C14756A061D662F580FF4DE43B49FA82D80A4B80F8434A
Keccak256("") = C5D2460186F7233C927E7DB2DCC703C0E500B653CA82273B7BFAD8045D85A470

Please note that when compiled in release test vectors succeed, however in debug the assert added due to some vulnerability is changing the behavior of these two functions.

Looking at the history I see that the asserts were added due to some vulnerability listed here:
https://github.com/weidai11/cryptopp/commit/399a1546de71f41598c15edada28e7f0d616f541

Commit was made by @noloader

[noloader]

Thanks @akovachev.

I'm not sure what to do with this report. I think things are working as expected.

Debug builds are intended to be run under a debugger. If there's no debugger attached then the debug version of the program should install its own trap handler like shown in test.cpp : 130:

// See misc.h and trap.h for comments and usage
#if defined(CRYPTOPP_DEBUG) && defined(UNIX_SIGNALS_AVAILABLE)
static const SignalHandler<SIGTRAP, false> s_dummyHandler;
#endif

The 0-length string is one of those corner cases that you deal with by pressing c when debugging with GDB. Otherwise, we lose the alerts associated with the assert. It effectively throws the baby out with the bath water.

It might be prudent to back up a bit and say we don't run debug builds in production. It is a different philosophy than some free/open source projects like Debian. Our feeling is, the time for debugging is over when you enter production.

---

The vulnerability you are referring to was a lack of documentation stating "you should use release builds in production and define NDEBUG". Or put another way, "you should not use debug builds in production and define NDEBUG". We did not state it and we took a CVE because of it.

After the CVE we stopped depending on NDEBUG to remove asserts. We stopped depending on it because too many developers were running fast and loose with their project flags and not defining NDEBUG for release/production. Effectively, we made it harder to shoot yourself in the foot.

(We did not realize we needed to say it. The project supplies a Makefile that puts the library in a recommended configuration with recommended options. Others were packaging the library with other build systems and using a different configuration and not using the project's recommended options. How that became our issue is beyond me).

[akovachev]

I think it's fine to check that the input pointer is not null. But it should be OK to accept 0-length input as it is valid.

The trouble is that we have some tests that we run both in debug and release.

Also - I noticed (after I have posted already) that you haven't written the assert, you just updated it to use the safe CRYPTOPP_ASSERT

Please note that SHA256 and RIPEMD160 work just fine with 0-length input.

This is a problem in the Keccak256 and SHA3 code really.

[akovachev]

The other option which we are going to do in the meantime is to not call Update when the input is 0 length and just call Finalize. Which is a fine solution, but should probably be reflected in the documentation that Update should not be called with 0-length.

[noloader]

@akovachev,

How does asserting on a nullptr sound? Are there any situations when a nullptr is considered a valid input?

---

Please note that SHA256 and RIPEMD160 work just fine with 0-length input.

Yeah, we should probably add some instrumentation to those files, too.

The idea of the asserts are to create self debugging programs so you don't have to trace a program under the debugger. The program should debug itself and snap a debugger on a bad input.

---

I noticed (after I have posted already) that you haven't written the assert, you just updated it to use the safe CRYPTOPP_ASSERT

CRYPTOPP_ASSERT actually calls raise(SIGTRAP). That's the 5 you are seeing. Previously the code used Posix assert, and the Posix version called raise(SIGABRT).

You can verify it in trap.h:

#if defined(CRYPTOPP_DEBUG) && defined(UNIX_SIGNALS_AVAILABLE)
#  define CRYPTOPP_ASSERT(exp) {                                  \
    if (!(exp)) {                                                 \
      std::ostringstream oss;                                     \
      oss << "Assertion failed: " << (char*)(__FILE__) << "("     \
          << (int)(__LINE__) << "): " << (char*)(__func__)        \
          << std::endl;                                           \
      std::cerr << oss.str();                                     \
      raise(SIGTRAP);                                             \
    }                                                             \
}

[akovachev]

Thank you, @noloader

I noticed that RIPEMD and SHA256 actually use iterhash and their Update has no assert whatsoever:

https://github.com/weidai11/cryptopp/blob/master/iterhash.cpp#L13
(Which should probably have a similar assert)

Also, just realized that if you want 0-length, the condition as written:

    CRYPTOPP_ASSERT((input && length) || !(input || length));

will be fine if the pointer is also nullptr. In other words - the library supports 0-length but you have to specifically also pass nullptr when doing so, otherwise it will assert.

It's probably best to leave as is, but consider adding the assert in the iterhash as well.

Thanks again!

[noloader]

@akovachev,

A quick question... Are you a maintainer or packager? If so, then feel free to ping me by email at noloader, gmail account. I can also provide my home number and cell phone number if you want/need a call.

Also we recently added another wiki page targeting distros, maintainers and packagers at BASE+SIMD. BASE+SIMD is a recent change at Crypto++ 6.0 and it caused some confusion due to the extra flags required for *-simd.cpp files.

[noloader]

@akovachev,

It's probably best to leave as is, but consider adding the assert in the iterhash as well.

Yes, I'm with you. The more instrumentation the better.

Do you recommend a similar assert to SHA3::Update? It is shown below, and it says "give me a good pointer and a non-0 length" or "give me a null pointer and 0 length". It is trying to alert to conditions that could point to a problem.

CRYPTOPP_ASSERT((input && length) || !(input || length));

[akovachev]

Yes, SHA3 is based on Keccak, so that's why it expects it this way. My issue is that this makes hash_tranformation derived classes inconsistent. Some assert and some do not.

I'm fine if they all have that assert but also have it reflected in the documentaiton.

Thanks again! For the record - we are just passing nullptr with 0 for these two now in our wrapper.

[akovachev]

@noloader,

Yes, I'm with you. The more instrumentation the better.

Great! I would then add the same assert to iterhash but also reflect in the documentation across the board that 0-length should be done with nullptr.

[noloader]

Cleared at PR #654

[noloader]

@akovachev,

I'm looking at the results of applying the assert to more hashes. I think it makes for a noisy experience because the empty string is a valid input. The library is a big offender.

I'm going to change the asserts to:

CRYPTOPP_ASSERT(!(input == NULLPTR && length != 0));

It says, "don't give us a NULL pointer and a non-0 length". I wanted to go with a NULL pointer alone, but it was still too noisy.

I think that is going to break your debug test cases. Would you mind making a quick testing pass on your code?

Also see Commit 315996980866.

[akovachev]

@noloader that was my original request, so yes, I am onboard with that.

I was hesitant to change the existing logic as it may break tests in other builds which already expect that behavior.

I think CRYPTOPP_ASSERT(!(input == NULLPTR && length != 0)); is quite reasonable and makes sense to use that across the board. Also, it won't break our tests, since we pass NULLPTR as well as 0-length.