Repository: randombit/botan
Pull request #3064 (2022-11-04 16:41:22)

[TLS 1.3] FIX: Record Layer must handle multiple records in compatibility mode
```
RFC 8446 5.1
   legacy_record_version:  MUST be set to 0x0303 for all records
      generated by a TLS 1.3 implementation other than an initial
      ClientHello [...] where it MAY also be 0x0301 for compatibility
      purposes.
```

If said Client Hello spans multiple records the above rule applies to all of them. Hence, `Record_Layer` cannot handle this state internally but must depend on its upstream user.

This introduces `m_first_message_sent` and `m_first_message_received` at the TLS 1.3 channel level and disables the compatibility mode in the `Record_Layer` accordingly.

Furthermore, another "compatibility mode" issue is fixed. Clients must not send a dummy Change Cipher Spec message prior to their second flight if this record is an unprotected Alert. The BoGo test that would have revealed that was disabled by accident.

I found these issues when starting to run BoGo server tests against [the upcoming TLS 1.3 server implementation](https://github.com/randombit/botan/pull/3053).


Patch:
diff --git a/src/bogo_shim/config.json b/src/bogo_shim/config.json
index 071a4a2db..46a52c5f3 100644
--- a/src/bogo_shim/config.json
+++ b/src/bogo_shim/config.json
@@ -107,7 +107,7 @@
         "SupportedVersionSelection-TLS12": "We just ignore the version extension in this case",
 
         "Downgrade-*-Client-Ignore": "Not possible to ignore downgrade indicator",
-        "Downgrade-TLS12-*": "Not a downgrade when we don't support v1.3",
+        "Downgrade-TLS12-Server": "Not a downgrade when we don't support v1.3",
 
         "Agree-Digest-SHA1": "No SHA-1 in TLS 1.2",
         "ServerAuth-SHA1-Fallback-*": "No SHA-1 in TLS 1.2",
diff --git a/src/editors/vscode/scripts/bogo.py b/src/editors/vscode/scripts/bogo.py
index 4e2183fdc..7c1fed12f 100755
--- a/src/editors/vscode/scripts/bogo.py
+++ b/src/editors/vscode/scripts/bogo.py
@@ -1,6 +1,7 @@
 #!/usr/bin/env python3
 
 import os
+import sys
 from common import run_cmd, get_concurrency
 
 
@@ -23,8 +24,11 @@ def main():
     # make doubly sure we're on the correct branch
     run_cmd("git -C %s checkout %s" % (BORING_PATH, BORING_BRANCH))
 
-    run_cmd("go test -pipe -num-workers %d -shim-path %s -shim-config %s" %
-            (get_concurrency(), os.path.abspath(SHIM_PATH), os.path.abspath(SHIM_CONFIG)), BOGO_PATH)
+    extra_args = "-debug -test '%s'" % ';'.join(
+        sys.argv[1:]) if len(sys.argv) > 1 else ''
+
+    run_cmd("go test -pipe -num-workers %d -shim-path %s -shim-config %s %s" %
+            (get_concurrency(), os.path.abspath(SHIM_PATH), os.path.abspath(SHIM_CONFIG), extra_args), BOGO_PATH)
 
 
 if __name__ == '__main__':
diff --git a/src/lib/tls/tls13/tls_channel_impl_13.cpp b/src/lib/tls/tls13/tls_channel_impl_13.cpp
index 452d18213..06a36cd01 100644
--- a/src/lib/tls/tls13/tls_channel_impl_13.cpp
+++ b/src/lib/tls/tls13/tls_channel_impl_13.cpp
@@ -54,7 +54,9 @@ Channel_Impl_13::Channel_Impl_13(Callbacks& callbacks,
    m_handshake_layer(m_side),
    m_can_read(true),
    m_can_write(true),
-   m_opportunistic_key_update(false)
+   m_opportunistic_key_update(false),
+   m_first_message_sent(false),
+   m_first_message_received(false)
    {
    }
 
@@ -146,6 +148,15 @@ size_t Channel_Impl_13::received_data(const uint8_t input[], size_t input_size)
                      // Downgrade can only happen if the first received message is a Server_Hello_12. This was not the case.
                      m_downgrade_info.reset();
                      }
+
+                  // After the initial handshake message is received, the record
+                  // layer must be more restrictive.
+                  // See RFC 8446 5.1 regarding "legacy_record_version"
+                  if(!m_first_message_received)
+                     {
+                     m_record_layer.disable_receiving_compat_mode();
+                     m_first_message_received = true;
+                     }
                   }
                }
             else
@@ -321,9 +332,22 @@ void Channel_Impl_13::send_record(uint8_t record_type, const std::vector<uint8_t
    BOTAN_STATE_CHECK(!is_downgrading());
    BOTAN_STATE_CHECK(m_can_write);
 
-   auto to_write = m_record_layer.prepare_records(static_cast<Record_Type>(record_type), record, m_cipher_state.get());
+   const auto type = static_cast<Record_Type>(record_type);
+   auto to_write = m_record_layer.prepare_records(type, record, m_cipher_state.get());
+
+   // After the initial handshake message is sent, the record layer must
+   // adhere to a more strict record specification. Note that for the
+   // server case this is a NOOP.
+   // See (RFC 8446 5.1. regarding "legacy_record_version")
+   if(!m_first_message_sent && type == Record_Type::HANDSHAKE)
+      {
+      m_record_layer.disable_sending_compat_mode();
+      m_first_message_sent = true;
+      }
 
-   if(prepend_ccs())
+   // The dummy CCS must not be prepended if the following record is
+   // an unprotected Alert record.
+   if(prepend_ccs() && (m_cipher_state || record_type != Record_Type::ALERT))
       {
       const auto ccs = m_record_layer.prepare_records(Record_Type::CHANGE_CIPHER_SPEC, {0x01}, m_cipher_state.get());
       to_write = concat(ccs, to_write);
diff --git a/src/lib/tls/tls13/tls_channel_impl_13.h b/src/lib/tls/tls13/tls_channel_impl_13.h
index eddeb4d05..f1a74dc50 100644
--- a/src/lib/tls/tls13/tls_channel_impl_13.h
+++ b/src/lib/tls/tls13/tls_channel_impl_13.h
@@ -262,6 +262,8 @@ class Channel_Impl_13 : public Channel_Impl
       bool m_can_write;
 
       bool m_opportunistic_key_update;
+      bool m_first_message_sent;
+      bool m_first_message_received;
    };
 }
 
diff --git a/src/lib/tls/tls13/tls_record_layer_13.cpp b/src/lib/tls/tls13/tls_record_layer_13.cpp
index 7ac9e9236..da443377e 100644
--- a/src/lib/tls/tls13/tls_record_layer_13.cpp
+++ b/src/lib/tls/tls13/tls_record_layer_13.cpp
@@ -136,8 +136,27 @@ Record_Layer::Record_Layer(Connection_Side side)
    : m_side(side)
    , m_outgoing_record_size_limit(MAX_PLAINTEXT_SIZE + 1 /* content type byte */)
    , m_incoming_record_size_limit(MAX_PLAINTEXT_SIZE + 1 /* content type byte */)
-   , m_first_sent_record(true)
-   , m_first_rcvd_record(true) {}
+
+   // RFC 8446 5.1
+   //    legacy_record_version: MUST be set to 0x0303 for all records
+   //       generated by a TLS 1.3 implementation other than an initial
+   //       ClientHello [...], where it MAY also be 0x0301 for compatibility
+   //       purposes.
+   //
+   // Additionally, older peers might send other values while requesting a
+   // protocol downgrade. I.e. we need to be able to tolerate/emit legacy
+   // values until we negotiated a TLS 1.3 compliant connection.
+   //
+   // As a client: we may initially emit the compatibility version and
+   //              accept a wider range of incoming legacy record versions.
+   // As a server: we start with emitting the specified legacy version of 0x0303
+   //              but must also allow a wider range of incoming legacy values.
+   //
+   // Once TLS 1.3 is negotiateed, the implementations will disable these
+   // compatibility modes accordingly or a protocol downgrade will transfer
+   // the marshalling responsibility to our TLS 1.2 implementation.
+   , m_sending_compat_mode(m_side == Connection_Side::CLIENT)
+   , m_receiving_compat_mode(true) {}
 
 
 void Record_Layer::copy_data(const uint8_t data[], size_t size)
@@ -232,9 +251,7 @@ std::vector<uint8_t> Record_Layer::prepare_records(const Record_Type type,
       //    MUST be set to 0x0303 for all records generated by a TLS 1.3
       //    implementation other than an initial ClientHello [...], where
       //    it MAY also be 0x0301 for compatibility purposes.
-      const auto use_compatibility_version = m_side == Connection_Side::CLIENT && m_first_sent_record;
-      const auto record_header = TLSPlaintext_Header(pt_type, ct_size, use_compatibility_version).serialized;
-      m_first_sent_record = false;
+      const auto record_header = TLSPlaintext_Header(pt_type, ct_size, m_sending_compat_mode).serialized;
 
       output.insert(output.end(), record_header.cbegin(), record_header.cend());
 
@@ -278,10 +295,10 @@ Record_Layer::ReadResult<Record> Record_Layer::next_record(Cipher_State* cipher_
    const auto header_begin = m_read_buffer.cbegin();
    const auto header_end   = header_begin + TLS_HEADER_SIZE;
 
-   // The first received record is likely a client or server hello. To be able to
-   // perform protocol downgrades we need must be less vigorous with the record's
-   // legacy version. Hence, `check_tls13_version` is `false` for the first record.
-   TLSPlaintext_Header plaintext_header({header_begin, header_end}, !m_first_rcvd_record);
+   // The first received record(s) are likely a client or server hello. To be able to
+   // perform protocol downgrades we must be less vigorous with the record's
+   // legacy version. Hence, `check_tls13_version` is `false` for the first record(s).
+   TLSPlaintext_Header plaintext_header({header_begin, header_end}, !m_receiving_compat_mode);
 
    // After the key exchange phase of the handshake is completed and record protection is engaged,
    // cipher_state is set. At this point, only protected traffic (and CCS) is allowed.
@@ -359,7 +376,6 @@ Record_Layer::ReadResult<Record> Record_Layer::next_record(Cipher_State* cipher_
       record.fragment.erase((end_of_content+1).base(), record.fragment.cend());
       }
 
-   m_first_rcvd_record = false;
    return record;
    }
 
diff --git a/src/lib/tls/tls13/tls_record_layer_13.h b/src/lib/tls/tls13/tls_record_layer_13.h
index 1ef156ed8..12aad09c3 100644
--- a/src/lib/tls/tls13/tls_record_layer_13.h
+++ b/src/lib/tls/tls13/tls_record_layer_13.h
@@ -100,6 +100,9 @@ class BOTAN_TEST_API Record_Layer
       void set_record_size_limits(const uint16_t outgoing_limit,
                                   const uint16_t incoming_limit);
 
+      void disable_sending_compat_mode() { m_sending_compat_mode = false; }
+      void disable_receiving_compat_mode() { m_receiving_compat_mode = false; }
+
    private:
       std::vector<uint8_t> m_read_buffer;
       Connection_Side      m_side;
@@ -110,10 +113,10 @@ class BOTAN_TEST_API Record_Layer
       uint16_t m_incoming_record_size_limit;
 
       // Those status flags are required for version validation where the initial
-      // record for sending and receiving is handled differently for backward
-      // compatibility reasons.
-      bool m_first_sent_record;
-      bool m_first_rcvd_record;
+      // records for sending and receiving is handled differently for backward
+      // compatibility reasons. (RFC 8446 5.1 regarding "legacy_record_version")
+      bool m_sending_compat_mode;
+      bool m_receiving_compat_mode;
    };
 
 }


Test patch:
diff --git a/src/tests/test_tls_record_layer_13.cpp b/src/tests/test_tls_record_layer_13.cpp
index d0064b12c..502ccf518 100644
--- a/src/tests/test_tls_record_layer_13.cpp
+++ b/src/tests/test_tls_record_layer_13.cpp
@@ -26,27 +26,24 @@ namespace TLS = Botan::TLS;
 
 using Records = std::vector<TLS::Record>;
 
-TLS::Record_Layer record_layer_client(const bool skip_initial_record=false)
+TLS::Record_Layer record_layer_client(const bool skip_client_hello=false)
    {
    auto rl = TLS::Record_Layer(TLS::Connection_Side::CLIENT);
 
    // this is relevant for tests that rely on the legacy version in the record
-   if(skip_initial_record)
-      rl.prepare_records(TLS::Record_Type::HANDSHAKE, {0, 0, 0});
+   if(skip_client_hello)
+      { rl.disable_sending_compat_mode(); }
 
    return rl;
    }
 
-TLS::Record_Layer record_layer_server(const bool skip_initial_record=false)
+TLS::Record_Layer record_layer_server(const bool skip_client_hello=false)
    {
    auto rl = TLS::Record_Layer(TLS::Connection_Side::SERVER);
 
    // this is relevant for tests that rely on the legacy version in the record
-   if(skip_initial_record)
-      {
-      rl.copy_data(Botan::hex_decode("16 03 01 00 03 00 00 00"));
-      rl.next_record();  // result is ignored
-      }
+   if(skip_client_hello)
+      { rl.disable_receiving_compat_mode(); }
 
    return rl;
    }
@@ -277,14 +274,28 @@ std::vector<Test::Result> basic_sanitization_parse_records(TLS::Connection_Side
             });
          }),
 
-      Botan_Tests::CHECK("initial received record version might be 0x03XX ", [&](auto& result)
+      Botan_Tests::CHECK("initial received record versions might be 0x03XX ", [&](auto& result)
          {
-         auto rl = record_layer_client(true);
+         auto rl = record_layer_client();
          rl.copy_data({0x16, 0x03, 0x00, 0x00, 0x01, 0x42});
-         result.test_no_throw("0x03 0x00 should be fine for first record", [&] { rl.next_record(); });
+         result.test_no_throw("0x03 0x00 should be fine for first records", [&] { rl.next_record(); });
 
-         rl.copy_data({0x16, 0x03, 0x00, 0x00, 0x01, 0x42});
-         result.test_throws("0x03 0x00 not okay for any other record", [&] { rl.next_record(); });
+         rl.copy_data({0x16, 0x03, 0x01, 0x00, 0x01, 0x42});
+         result.test_no_throw("0x03 0x01 should be fine for first records", [&] { rl.next_record(); });
+
+         rl.copy_data({0x16, 0x03, 0x02, 0x00, 0x01, 0x42});
+         result.test_no_throw("0x03 0x02 should be fine for first records", [&] { rl.next_record(); });
+
+         rl.copy_data({0x16, 0x03, 0x03, 0x00, 0x01, 0x42});
+         result.test_no_throw("0x03 0x03 should be fine for first records", [&] { rl.next_record(); });
+
+         rl.disable_receiving_compat_mode();
+
+         rl.copy_data({0x16, 0x03, 0x03, 0x00, 0x01, 0x42});
+         result.test_no_throw("0x03 0x03 is okay regardless", [&] { rl.next_record(); });
+
+         rl.copy_data({0x16, 0x03, 0x01, 0x00, 0x01, 0x42});
+         result.test_throws("0x03 0x01 not okay once client hello was received", [&] { rl.next_record(); });
          }),
 
       Botan_Tests::CHECK("malformed change cipher spec", [&](auto& result)
@@ -761,8 +772,18 @@ std::vector<Test::Result> legacy_version_handling()
    auto has_version = [](const auto& record, const uint16_t version) -> bool
       {
       TLS::TLS_Data_Reader dr("header reader", record);
-      dr.discard_next(1);
-      return dr.get_uint16_t() == version;
+
+      while(dr.has_remaining())
+         {
+         dr.discard_next(1);  // record type
+         if(dr.get_uint16_t() != version)
+            return false;
+         const auto record_size = dr.get_uint16_t();
+         dr.discard_next(record_size);
+         }
+
+      dr.assert_done();
+      return true;
       };
 
    auto parse_record = [](auto& record_layer, const std::vector<uint8_t>& data)
@@ -779,10 +800,24 @@ std::vector<Test::Result> legacy_version_handling()
          auto rec = rl.prepare_records(TLS::Record_Type::HANDSHAKE, std::vector<uint8_t>(5));
          result.confirm("first record has version 0x0301", has_version(rec, 0x0301));
 
+         rl.disable_sending_compat_mode();
+
          rec = rl.prepare_records(TLS::Record_Type::HANDSHAKE, std::vector<uint8_t>(5));
          result.confirm("next record has version 0x0303", has_version(rec, 0x0303));
          }),
 
+      Botan_Tests::CHECK("client side starts with version 0x0301 (even if multiple reconds are required)", [&](Test::Result& result)
+         {
+         auto rl = record_layer_client();
+         auto rec = rl.prepare_records(TLS::Record_Type::HANDSHAKE, std::vector<uint8_t>(5 * Botan::TLS::MAX_PLAINTEXT_SIZE));
+         result.confirm("first record has version 0x0301", has_version(rec, 0x0301));
+
+         rl.disable_sending_compat_mode();
+
+         rec = rl.prepare_records(TLS::Record_Type::HANDSHAKE, std::vector<uint8_t>(5 * Botan::TLS::MAX_PLAINTEXT_SIZE));
+         result.confirm("next record has version 0x0303", has_version(rec, 0x0303));
+         }),
+
       Botan_Tests::CHECK("server side starts with version 0x0303", [&](Test::Result& result)
          {
          auto rl = record_layer_server(true);
@@ -815,15 +850,16 @@ std::vector<Test::Result> legacy_version_handling()
          result.test_no_throw("parsing initial record", [&] { parse_record(rl, first_record);});
          }),
 
-      Botan_Tests::CHECK("server side does not accept version 0x0301 for the second record", [&](Test::Result& result)
+      Botan_Tests::CHECK("server side does not accept version 0x0301 after receiving client hello", [&](Test::Result& result)
          {
          const auto record = Botan::hex_decode("16 03 01 00 05 00 00 00 00 00");
          auto rl = record_layer_server();
          result.test_no_throw("parsing initial record", [&] { parse_record(rl, record);});
+         rl.disable_receiving_compat_mode();
          result.test_throws("parsing second record", [&] { parse_record(rl, record);});
          }),
 
-      Botan_Tests::CHECK("server side does not accept other versions (after the first record)", [&](Test::Result& result)
+      Botan_Tests::CHECK("server side does not accept other versions (after receiving client hello)", [&](Test::Result& result)
          {
          auto rl = record_layer_server(true);
          result.test_throws("does not accept 0x0300", [&] { parse_record(rl, Botan::hex_decode("16 03 00 00 05 00 00 00 00 00"));});