Repository: randombit/botan (cpp)

Issue:
AV on certain AEAD OCB decryption input calls under Windows.
We use Botan as cryptography backend for RNP OpenPGP library. And recently, after updating CI runners to Botan 3.1.1, started to receive failures, only for Windows platform. After isolating the issue and enabling sanitizers got the following call stack. Suspecting something in RNP which may corrupt the memory (however sanitizers doesn't repor that), I was able to isolate the issue to pure botan calls with some certain input.

Do you have any idea why this happens?
Should I provide some more input, like test data file/order of decryption calls causing the failure?


```
2: ==6280==ERROR: AddressSanitizer: access-violation on unknown address 0x00004e000017 (pc 0x7ff9c7f0f990 bp 0x00e1edaf5fb0 sp 0x00e1edaf5eb0 T0)
2: ==6280==The signal is caused by a READ memory access.
2:     #0 0x7ff9c7f0f98f in Botan::L_computer::compute_offsets(unsigned __int64, unsigned __int64) C:\vcpkg\buildtrees\botan\src\3.1.1-d5e04c5040.clean\src\lib\modes\aead\ocb\ocb.cpp:68
2:     #1 0x7ff9c7f1024e in Botan::OCB_Decryption::decrypt(unsigned char *const, unsigned __int64) C:\vcpkg\buildtrees\botan\src\3.1.1-d5e04c5040.clean\src\lib\modes\aead\ocb\ocb.cpp:413
2:     #2 0x7ff9c7f10617 in Botan::OCB_Decryption::finish_msg(class std::vector<unsigned char, class Botan::secure_allocator<unsigned char>> &, unsigned __int64) C:\vcpkg\buildtrees\botan\src\3.1.1-d5e04c5040.clean\src\lib\modes\aead\ocb\ocb.cpp:451
2:     #3 0x7ff9c7e59139 in Botan::Cipher_Mode::finish C:\vcpkg\buildtrees\botan\x64-windows-rel\build\include\botan\cipher_mode.h:146
2:     #4 0x7ff9c7e59139 in `botan_cipher_update'::`2'::<lambda_1>::operator() C:\vcpkg\buildtrees\botan\src\3.1.1-d5e04c5040.clean\src\lib\ffi\ffi_cipher.cpp:159
2:     #5 0x7ff9c7e4ab83 in std::_Func_class<int>::operator() C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\functional:869
2:     #6 0x7ff9c7e4ab83 in Botan_FFI::ffi_guard_thunk(char const *, class std::function<(void)> const &) C:\vcpkg\buildtrees\botan\src\3.1.1-d5e04c5040.clean\src\lib\ffi\ffi.cpp:119
2:     #7 0x7ff9c7e5a9a2 in botan_cipher_update C:\vcpkg\buildtrees\botan\src\3.1.1-d5e04c5040.clean\src\lib\ffi\ffi_cipher.cpp:141
**[ botan calls start here ]**
2:     #8 0x7ff667daed44 in pgp_cipher_aead_finish(struct pgp_crypt_t *, unsigned char *, unsigned char const *, unsigned __int64) D:\a\rnp\rnp\src\lib\crypto\symmetric.cpp:589
2:     #9 0x7ff667e15435 in encrypted_src_read_aead_part D:\a\rnp\rnp\src\librepgp\stream-parse.cpp:652
2:     #10 0x7ff667e14ed3 in encrypted_src_read_aead D:\a\rnp\rnp\src\librepgp\stream-parse.cpp:728
2:     #11 0x7ff667de6234 in src_read(struct pgp_source_t *, void *, unsigned __int64, unsigned __int64 *) D:\a\rnp\rnp\src\librepgp\stream-common.cpp:115
2:     #12 0x7ff667e1b5ed in partial_pkt_src_read D:\a\rnp\rnp\src\librepgp\stream-parse.cpp:273
2:     #13 0x7ff667de6234 in src_read(struct pgp_source_t *, void *, unsigned __int64, unsigned __int64 *) D:\a\rnp\rnp\src\librepgp\stream-common.cpp:115
2:     #14 0x7ff667e12e8e in compressed_src_read D:\a\rnp\rnp\src\librepgp\stream-parse.cpp:399
2:     #15 0x7ff667de6234 in src_read(struct pgp_source_t *, void *, unsigned __int64, unsigned __int64 *) D:\a\rnp\rnp\src\librepgp\stream-common.cpp:115
2:     #16 0x7ff667e1b5ed in partial_pkt_src_read D:\a\rnp\rnp\src\librepgp\stream-parse.cpp:273
2:     #17 0x7ff667de6234 in src_read(struct pgp_source_t *, void *, unsigned __int64, unsigned __int64 *) D:\a\rnp\rnp\src\librepgp\stream-common.cpp:115
2:     #18 0x7ff667e1b27b in literal_src_read D:\a\rnp\rnp\src\librepgp\stream-parse.cpp:344
2:     #19 0x7ff667de6234 in src_read(struct pgp_source_t *, void *, unsigned __int64, unsigned __int64 *) D:\a\rnp\rnp\src\librepgp\stream-common.cpp:115
2:     #20 0x7ff667e1c192 in process_pgp_source(struct pgp_parse_handler_t *, struct pgp_source_t &) D:\a\rnp\rnp\src\librepgp\stream-parse.cpp:2979
2:     #21 0x7ff667da3225 in rnp_op_verify_execute D:\a\rnp\rnp\src\lib\rnp.cpp:3358
2:     #22 0x7ff667d08905 in cli_rnp_process_file(class cli_rnp_t *) D:\a\rnp\rnp\src\rnp\fficli.cpp:3193
2:     #23 0x7ff667ce3468 in rnp_cmd D:\a\rnp\rnp\src\rnp\rnp.cpp:264
2:     #24 0x7ff667ce7656 in main D:\a\rnp\rnp\src\rnp\rnp.cpp:707
2:     #25 0x7ff667eb0193 in invoke_main D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:78
2:     #26 0x7ff667eb0193 in __scrt_common_main_seh D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
2:     #27 0x7ff9f9667ac3  (C:\Windows\System32\KERNEL32.DLL+0x180017ac3)
2:     #28 0x7ff9fbf4a4e0  (C:\Windows\SYSTEM32\ntdll.dll+0x18005a4e0)
```

Pull request:
FIX: Access Violation due to stale reference in OCB
This contains a proposal for a fix of the crash in the OCB mode reported in #3812. Personally, I'm not very familiar with OCB, so this is the simplest fix I could come up with that likely won't break any optimization assumptions.

@randombit I'd be glad if you could take another look at this. The regression test in the commit should work reliably on typical platforms.

Closes #3812.

Patch:
diff --git a/src/lib/modes/aead/ocb/ocb.cpp b/src/lib/modes/aead/ocb/ocb.cpp
index 8995a8df2..8c4310353 100644
--- a/src/lib/modes/aead/ocb/ocb.cpp
+++ b/src/lib/modes/aead/ocb/ocb.cpp
@@ -23,6 +23,17 @@ class L_computer final {
          m_L_star.resize(m_BS);
          cipher.encrypt(m_L_star);
          m_L_dollar = poly_double(star());
+
+         // Preallocate the m_L vector to the maximum expected size to avoid
+         // re-allocations during runtime. This had caused a use-after-free in
+         // earlier versions, due to references into this buffer becoming stale
+         // in `compute_offset()`, after calling `get()` in the hot path.
+         //
+         // Note, that the list member won't be pre-allocated, so the expected
+         // memory overhead is negligible.
+         //
+         // See also https://github.com/randombit/botan/issues/3812
+         m_L.reserve(31);
          m_L.push_back(poly_double(dollar()));
 
          while(m_L.size() < 8) {


Test patch:
diff --git a/src/tests/test_ocb.cpp b/src/tests/test_ocb.cpp
index 657249c0d..d71e8fc4e 100644
--- a/src/tests/test_ocb.cpp
+++ b/src/tests/test_ocb.cpp
@@ -13,6 +13,8 @@
    #include <botan/internal/poly_dbl.h>
 #endif
 
+#include <array>
+
 namespace Botan_Tests {
 
 namespace {
@@ -329,6 +331,111 @@ BOTAN_REGISTER_TEST("modes", "ocb_long", OCB_Long_KAT_Tests);
 
    #endif
 
+/**
+ * Extremely cheap toy cipher for the OCB regression test for
+ * the issue explained in GitHub #3812.
+ */
+class OCB_Null_Cipher final : public Botan::BlockCipher {
+   public:
+      explicit OCB_Null_Cipher(size_t bs, size_t parallelism) :
+            m_bs(bs), m_parallelism(parallelism), m_has_key(false) {}
+
+      std::string name() const override { return "OCB_Null_Cipher"; }
+
+      size_t block_size() const override { return m_bs; }
+
+      void clear() override {}
+
+      std::unique_ptr<Botan::BlockCipher> new_object() const override {
+         return std::make_unique<OCB_Null_Cipher>(m_bs, m_parallelism);
+      }
+
+      bool has_keying_material() const override { return m_has_key; }
+
+      void key_schedule(std::span<const uint8_t>) override { m_has_key = true; }
+
+      Botan::Key_Length_Specification key_spec() const override { return Botan::Key_Length_Specification(m_bs); }
+
+      void encrypt_n(const uint8_t in[], uint8_t out[], size_t blocks) const override {
+         if(in != out) {
+            Botan::copy_mem(out, in, blocks * m_bs);
+         }
+      }
+
+      void decrypt_n(const uint8_t in[], uint8_t out[], size_t blocks) const override {
+         if(in != out) {
+            Botan::copy_mem(out, in, blocks * m_bs);
+         }
+      }
+
+      size_t parallelism() const override { return m_parallelism; }
+
+   private:
+      size_t m_bs;
+      size_t m_parallelism;
+      bool m_has_key;
+};
+
+/**
+ * A regression test for a crash found in OCB in November 2023.
+ * See here for details: https://github.com/randombit/botan/issues/3812
+ */
+Test::Result test_ocb_crash_regression() {
+   Test::Result result("OCB crash regression");
+
+   constexpr size_t cipherparallelism = 4;
+   constexpr size_t blocksize = 16;
+   constexpr size_t tagsize = 8;
+   constexpr size_t chunksize = 32 * 1024;
+   constexpr size_t preamblesize = blocksize * cipherparallelism;
+
+   // 1 MiB plus the preamble should be "just" enough to hit a block_index
+   // that causes m_L to generate for i == 16. We likely hit a re-alloc before.
+   constexpr size_t datasize = 1 * 1024 * 1024 + preamblesize;
+   static_assert((datasize - preamblesize) % chunksize == 0);
+
+   std::vector<uint8_t> data(chunksize);
+   std::array<uint8_t, 8> iv = {0};
+   std::array<uint8_t, 16> key = {0};
+
+   Botan::OCB_Encryption enc(std::make_unique<OCB_Null_Cipher>(blocksize, cipherparallelism), tagsize);
+   enc.set_key(key);
+   enc.start(iv);
+
+   // Bring the cipher mode into a state where it is at risk to re-allocate its
+   // m_L vector just the right way to cause the crash.
+   std::vector<uint8_t> preamble(blocksize * 4);
+   enc.update(preamble, 0);
+
+   // Now run the encryption for a while, hoping to cause a re-allocation at
+   // the right code path to cause the crash.
+   for(size_t bytes = preamble.size(); bytes < datasize; bytes += chunksize) {
+      data.resize(chunksize);
+      enc.update(data, 0);
+   }
+
+   std::vector<uint8_t> tag;
+   enc.finish(tag, 0);
+
+   // Repeat the experiment with the decryption code paths.
+   Botan::OCB_Decryption dec(std::make_unique<OCB_Null_Cipher>(16, cipherparallelism), 8);
+   dec.set_key(key);
+   dec.start(iv);
+
+   dec.update(preamble, 0);
+
+   for(size_t bytes = 0; bytes < datasize; bytes += chunksize) {
+      data.resize(chunksize);
+      dec.update(data, 0);
+   }
+
+   dec.finish(tag, 0);
+
+   return result;
+}
+
+BOTAN_REGISTER_TEST_FN("modes", "ocb_lazy_alloc", test_ocb_crash_regression);
+
 #endif
 
 }  // namespace