./configure.py --terminate-on-asserts by reneme · Pull Request #2980 · randombit/botan

reneme · 2022-05-23T11:17:28Z

As discussed, this adds ./configure.py --terminate-on-asserts causing the library to std::abort when an assertion is not met.

The code base seems to use BOTAN_ASSERT* in some places where BOTAN_ARG_CHECK or BOTAN_STATE_CHECK would be more appropriate. Hence, I enabled the new option for the coverage CI run in the hope to catch most of them.

randombit

LGTM. Still a few CI failures which I assume are missing cases but once CI is green good to merge

src/lib/utils/assert.cpp

reneme · 2022-05-23T13:06:21Z

The assertion in the coverage build job seems like a bug, actually.

There are a few BoGo tests that send a prefix string to the TLS server under test. E.g. BoGo test HttpGET sends GET / HTTP/1.0\n" and expects the TLS server under test to notice that and fail. The TLS 1.2 record implementation detects this situation and throws a TLS_Exception:

botan/src/lib/tls/tls12/tls_record.cpp

Lines 361 to 368 in 1522871

    
           if(first5 == "GET /" || 
        
              first5 == "PUT /" || 
        
              first5 == "POST " || 
        
              first5 == "HEAD ") 
        
              { 
        
              throw TLS_Exception(Alert::PROTOCOL_VERSION, 
        
                                  "Client sent plaintext HTTP request instead of TLS handshake"); 
        
              }

The TLS channel then catches this exception and proceeds to sending a TLS alert in response. TLS_Channel::send_alert() used to swallow all exceptions that happened while trying to send an alert:

botan/src/lib/tls/tls12/tls_channel_impl_12.cpp

Lines 573 to 577 in 1522871

    
           try 
        
              { 
        
              send_record(ALERT, alert.serialize()); 
        
              } 
        
           catch(...) { /* swallow it */ }

But now, the sequence_numbers() accessor is called downstream, causing an assertion because the server didn't actually finish initializing and m_sequence_numbers is still a nullptr:

botan/src/lib/tls/tls12/tls_channel_impl_12.cpp

Lines 548 to 552 in 1522871

    
           void Channel_Impl_12::send_record(uint8_t record_type, const std::vector<uint8_t>& record) 
        
              { 
        
              send_record_array(sequence_numbers().current_write_epoch(), 
        
                                record_type, record.data(), record.size()); 
        
              }

I see two possible fixes:

initialize m_sequence_numbers before any data is processed, or
detect this situation and handle it

But is it actually legal for a TLS server to send a TLS alert as their first flight?

reneme · 2022-05-23T13:53:46Z

But is it actually legal for a TLS server to send a TLS alert as their first flight?

At least for Botan's (D)TLS 1.2 implementation it doesn't seem to be legal for the TLS server to send messages before it successfully received a Client Hello. Only after that do we know whether we speak TLS or DTLS.

Hence, let's just not send TLS alerts in the very early error case described above: 9c70d9d

reneme · 2022-05-24T09:00:59Z

src/lib/pubkey/dsa/dsa.cpp

+
+DSA_PublicKey::DSA_PublicKey(const AlgorithmIdentifier& alg_id,
+                             const std::vector<uint8_t>& key_bits) :
+   DL_Scheme_PublicKey(alg_id, key_bits, DL_Group_Format::ANSI_X9_57)
+   {
+   BOTAN_ARG_CHECK(group_q().bytes() > 0, "Q parameter must be set for DSA");
+   }
+
+
 DSA_PublicKey::DSA_PublicKey(const DL_Group& grp, const BigInt& y1)
   {
   m_group = grp;
   m_y = y1;
+
+   BOTAN_ARG_CHECK(grp.q_bytes() > 0, "Q parameter must be set for DSA");
   }


That is suppose to fix asserts in PK_Verifier for a DSA public key in the cert corpus/fuzzer. I left the assert untouched and instead introduce sanity checks at construction time of the DSA public key.
@randombit I could use a little help with this, as I'm not sure whether other things should also be validated here.

By the way: The currently still failing assert in the pkcs8 fuzzer is related to a DSA private key:

* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT * frame #0: 0x00007ff81db8e00e libsystem_kernel.dylib`__pthread_kill + 10 frame #1: 0x00007ff81dbc41ff libsystem_pthread.dylib`pthread_kill + 263 frame #2: 0x00007ff81db0fd24 libsystem_c.dylib`abort + 123 frame #3: 0x00000001009a9778 pkcs8`Botan::assertion_failure(char const*, char const*, char const*, char const*, int) + 696 frame #4: 0x000000010048ed39 pkcs8`Botan::Montgomery_Exponentation_State::exponentiation(Botan::BigInt const&, unsigned long) const + 1001 frame #5: 0x000000010048fc46 pkcs8`Botan::monty_execute(Botan::Montgomery_Exponentation_State const&, Botan::BigInt const&, unsigned long) + 22 frame #6: 0x0000000100651218 pkcs8`Botan::DL_Group_Data::power_g_p(Botan::BigInt const&, unsigned long) const + 72 frame #7: 0x000000010065127b pkcs8`Botan::DL_Group::power_g_p(Botan::BigInt const&, unsigned long) const + 59 frame #8: 0x0000000100660e4f pkcs8`Botan::DSA_PrivateKey::DSA_PrivateKey(Botan::AlgorithmIdentifier const&, std::__1::vector<unsigned char, Botan::secure_allocator<unsigned char> > const&) + 767 frame #9: 0x000000010080913c pkcs8`std::__1::__unique_if<Botan::DSA_PrivateKey>::__unique_single std::__1::make_unique<Botan::DSA_PrivateKey, Botan::AlgorithmIdentifier const&, std::__1::vector<unsigned char, Botan::secure_allocator<unsigned char> > const&>(Botan::AlgorithmIdentifier const&, std::__1::vector<unsigned char, Botan::secure_allocator<unsigned char> > const&) + 76 frame #10: 0x000000010080851b pkcs8`Botan::load_private_key(Botan::AlgorithmIdentifier const&, std::__1::vector<unsigned char, Botan::secure_allocator<unsigned char> > const&) + 1003 frame #11: 0x000000010083fe17 pkcs8`Botan::PKCS8::(anonymous namespace)::load_key(Botan::DataSource&, std::__1::function<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > ()> const&, bool) + 471 frame #12: 0x0000000100840587 pkcs8`Botan::PKCS8::load_key(Botan::DataSource&) + 231 frame #13: 0x0000000100002c8f pkcs8`fuzz(unsigned char const*, unsigned long) + 255 frame #14: 0x0000000100002b8a pkcs8`LLVMFuzzerTestOneInput + 26 frame #15: 0x0000000100003483 pkcs8`(anonymous namespace)::fuzz_files(char**) + 899 frame #16: 0x0000000100002eb7 pkcs8`main + 231 frame #17: 0x0000000101df551e dyld`start + 462

I think this assert would trigger if you load a DSA key with a secret key that is greater than the group order.

This likely affects other key types as well.

I think we need to rethink key validation somewhat. Right now all validation is dumped on check_key, because some of the checks (testing primality and ECC point validity) are expensive. But most are simple range checks and could be cheaply done during construction or deserialization.

This is resolved with 1735f95 right?

codecov-commenter · 2022-05-25T15:11:49Z

Codecov Report

Merging #2980 (efcc70b) into master (4c58fd5) will decrease coverage by 0.06%.
The diff coverage is 88.88%.

❗ Current head efcc70b differs from pull request most recent head 5f8bca9. Consider uploading reports for the commit 5f8bca9 to get more accurate results

@@            Coverage Diff             @@
##           master    #2980      +/-   ##
==========================================
- Coverage   92.58%   92.52%   -0.07%     
==========================================
  Files         596      577      -19     
  Lines       69779    66792    -2987     
  Branches     6617     6400     -217     
==========================================
- Hits        64608    61801    -2807     
+ Misses       5138     4958     -180     
  Partials       33       33

Impacted Files	Coverage Δ
src/lib/utils/assert.cpp	`42.10% <0.00%> (-57.90%)`	⬇️
src/lib/asn1/asn1_time.cpp	`91.78% <100.00%> (ø)`
src/lib/modes/aead/eax/eax.cpp	`97.77% <100.00%> (ø)`
src/lib/pubkey/dsa/dsa.cpp	`97.70% <100.00%> (+0.29%)`	⬆️
src/lib/pubkey/rsa/rsa.cpp	`97.00% <100.00%> (ø)`
src/lib/tls/tls12/tls_channel_impl_12.cpp	`89.59% <100.00%> (+0.54%)`	⬆️
src/tests/test_tls_utils.cpp	`0.00% <0.00%> (-83.34%)`	⬇️
src/lib/utils/thread_utils/semaphore.cpp	`69.23% <0.00%> (-30.77%)`	⬇️
src/fuzzer/ressol.cpp	`85.71% <0.00%> (-7.15%)`	⬇️
... and 70 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

reneme · 2022-05-25T15:15:04Z

src/lib/pubkey/dsa/dsa.cpp

                               const secure_vector<uint8_t>& key_bits) :
   DL_Scheme_PrivateKey(alg_id, key_bits, DL_Group_Format::ANSI_X9_57)
   {
+   BOTAN_ARG_CHECK(m_x.bits() <= m_group.q_bits(), "x must not be larger than q");


I have a hunch that this check should better be constant time, because:

botan/src/lib/math/numbertheory/monty_exp.cpp

Lines 102 to 105 in 1522871

BigInt Montgomery_Exponentation_State::exponentiation(const BigInt& scalar, size_t max_k_bits) const

{

BOTAN_DEBUG_ASSERT(scalar.bits() <= max_k_bits);

// TODO add a const-time implementation of above assert and use it in release builds

... some guidance on how to do that would be appreciated.

I think this is safe enough to not be constant time in this context. bits itself is constant time, and if we reject that means the key itself is just manifest invalid. So there is no real info leak.

We should check x <= q though rather than just the bit size, since x == q would be accepted, but isn't a valid DSA key.

randombit

LGTM, modulo changing DSA key check to use value instead of bit length.

reneme · 2022-06-01T13:25:42Z

src/lib/pubkey/dsa/dsa.cpp

   DL_Scheme_PrivateKey(alg_id, key_bits, DL_Group_Format::ANSI_X9_57)
   {
+   BOTAN_ARG_CHECK(m_x > 0, "x must be greater than zero");
+   BOTAN_ARG_CHECK(m_x < m_group.get_q(), "x must not be larger than q");


I added a check that x > 0 because the failing fuzzer test (pkcs8) caused negative numbers to be passed into this constructor.

On that note: I feel that DL_Group should also validate that it is always instantiated with positive numbers, no?

On that note: I feel that DL_Group should also validate that it is always instantiated with positive numbers, no?

It should. I think if p or q is zero or negative that will cause Modular_Reducer to throw an exception. I don't think g would be caught. Also we are not checking other invariants like g < p

Nevertheless, I propose this is something for a follow-up PR. I wanted to keep this to the minimum to just start enforcing assertions in the first place.

We cannot send alerts over a connection that fails even before a readable Client Hello was received. In that case we cannot determine whether the client speaks TLS or DTLS, hence, we cannot generate a valid TLS alert record.

reneme · 2022-08-08T07:04:47Z

I rebased to master to let the CI run once again after this was stale for more than a month. Once that comes back clean, I'll merge.

randombit approved these changes May 23, 2022

View reviewed changes

src/lib/utils/assert.cpp Outdated Show resolved Hide resolved

reneme force-pushed the terminate_on_assert branch from 7787d67 to 9c70d9d Compare May 23, 2022 13:51

reneme commented May 24, 2022

View reviewed changes

reneme commented May 25, 2022

View reviewed changes

randombit approved these changes Jun 1, 2022

View reviewed changes

reneme force-pushed the terminate_on_assert branch from 1735f95 to efcc70b Compare June 1, 2022 13:21

reneme commented Jun 1, 2022

View reviewed changes

reneme added 8 commits August 8, 2022 09:03

Introduce ./configure.py --terminate-on-asserts

3838491

use --terminate-on-asserts for coverage and fuzzing in CI

36b8ad2

FIX: replace BOTAN_ASSERT to pass all unit tests

10c21b8

FIX: premature sending of TLS alerts in TLS 1.2 server

4d49b11

We cannot send alerts over a connection that fails even before a readable Client Hello was received. In that case we cannot determine whether the client speaks TLS or DTLS, hence, we cannot generate a valid TLS alert record.

FIX: don't instantiate an RSA pubkey with bogus N

f06531c

FIX: validate DSA keys during construction

2b47ca7

FIX: validate X <= Q before exponentiation

69a20a1

review comments

5f8bca9

reneme force-pushed the terminate_on_assert branch from efcc70b to 5f8bca9 Compare August 8, 2022 07:03

reneme merged commit a78ea35 into master Aug 8, 2022

reneme deleted the terminate_on_assert branch August 9, 2022 08:57

lz101010 mentioned this pull request Aug 23, 2022

abort botan tests when an assertion is not met google/oss-fuzz#8320

Open

	BigInt Montgomery_Exponentation_State::exponentiation(const BigInt& scalar, size_t max_k_bits) const
	{
	BOTAN_DEBUG_ASSERT(scalar.bits() <= max_k_bits);
	// TODO add a const-time implementation of above assert and use it in release builds

Conversation

reneme commented May 23, 2022

Uh oh!

randombit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

reneme commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reneme commented May 23, 2022

Uh oh!

reneme May 24, 2022

Choose a reason for hiding this comment

Uh oh!

randombit Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

reneme May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

randombit Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

randombit left a comment

Choose a reason for hiding this comment

Uh oh!

reneme Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

randombit Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

reneme Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

reneme commented Aug 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

reneme commented May 23, 2022 •

edited

Loading

codecov-commenter commented May 25, 2022 •

edited

Loading

reneme May 25, 2022 •

edited

Loading