Semantics of Assertions in Botan

[reneme]

Generally speaking, I personally read assertions as "explicitly articulated assumptions" on the program state at some particular code location. If any such assumption is violated in normal program flow the behaviour will be undefined going forward. Therefore, mitigation steps are to be taken. Such as program termination or raising an exception.

Botan has different types of assertions (see assert.h):

• BOTAN_ASSERT_* (e.g. _NONNULL for pointers, _EQUAL, _IMPLICATION, ...)
  Reads like a typical assertion as described above. Throws an Internal_Error exception with a descriptive error message about the affected code location and potentially some user-defined error string.
• BOTAN_STATE_CHECK
  Checks some assumption and throw an Invalid_State exception if not fulfilled
• BOTAN_ARG_CHECK
  Checks some assumption and throw an Invalid_Argument exception with a defined error message

While playing around with Botan's fuzzing targets I realized that violated assertions would not lead to a bug being detected by the fuzzer because all exceptions are simply ignored. This clashes with my personal mental model of assertions as stated in the first paragraph. In my opinion the fuzzer should pick up on "violated assumptions on the program state", as this should point to an issue originating somewhere down in the call stack.

To come to the point: I think it would be beneficial to provide ./configure.py --terminate-on-asserts that replaces throw with std::abort for (at least) BOTAN_ASSERT_*-style assertions. That way, the fuzzers could leverage the domain knowledge encoded in the assertions spread around the code base to find bugs.

Actually switching to std::abort for my fuzzing experiments resulted in crashes very quickly. It seems that assertions are used for generic argument bounds checks (e.g. see asn1_time.cpp). This might be intentional or accidental. Though, it surely makes fuzzing less useful in the library and we would benefit from a common understanding of what assertions are meant to express in the code base.

What are your thoughts on that?

[reneme]

For the record: changing the mentioned location in asn1_time.cpp from BOTAN_ASSERT to BOTAN_ARG_CHECK does unblock the fuzzer and let it make further progress. Currently, I only changed BOTAN_ASSERT_* to std::abort.

[randombit]

BOTAN_ASSERT should only be used to assert logical invariants that should never fail. However right now it is in areas being abused for argument checking and the like. These uses should be replaced by a different macro.

[reneme]

So I would suggest to introduce a std::abort mode for fuzzing and actually treat misuses as bugs. I.e. replace them with BOTAN_ARG_CHECK as stated above.

[randombit]

Sounds good also yes I'm fine with adding a mode that terminates on assertion failures