Repository: randombit/botan
Pull request #3380 (2023-03-15 11:15:52)

Backport some RSA fixes
In RSA-KEM, we should have been padding the raw shared key out to the modulus size. This is required by RFC 5990, and additionally was already done on the decryption side.

In addition to checking that inputs are less than the public modulus, check that the bytestrings are no longer than the byte count of the public modulus. Previously we would ignore leading 0 bytes.


Patch:
diff --git a/src/lib/pubkey/rsa/rsa.cpp b/src/lib/pubkey/rsa/rsa.cpp
index 43e4b052d..4bc0a0719 100644
--- a/src/lib/pubkey/rsa/rsa.cpp
+++ b/src/lib/pubkey/rsa/rsa.cpp
@@ -376,6 +376,8 @@ class RSA_Private_Operation
 
       secure_vector<uint8_t> raw_op(const uint8_t input[], size_t input_len)
          {
+         if(input_len > public_modulus_bytes())
+            throw Invalid_Argument("RSA private op - input is too long");
          const BigInt input_bn(input, input_len);
          if(input_bn >= m_public->get_n())
             throw Invalid_Argument("RSA private op - input is too large");
@@ -578,6 +580,8 @@ class RSA_Encryption_Operation final : public PK_Ops::Encryption_with_EME,
       secure_vector<uint8_t> raw_encrypt(const uint8_t input[], size_t input_len,
                                          RandomNumberGenerator&) override
          {
+         if(input_len > public_modulus_bytes())
+            throw Invalid_Argument("RSA encryption - input is too long");
          BigInt input_bn(input, input_len);
          return BigInt::encode_1363(public_op(input_bn), public_modulus_bytes());
          }
@@ -600,6 +604,8 @@ class RSA_Verify_Operation final : public PK_Ops::Verification_with_EMSA,
 
       secure_vector<uint8_t> verify_mr(const uint8_t input[], size_t input_len) override
          {
+         if(input_len > public_modulus_bytes())
+            throw Invalid_Argument("RSA verification - input is too long");
          BigInt input_bn(input, input_len);
          return BigInt::encode_locked(public_op(input_bn));
          }
@@ -623,8 +629,8 @@ class RSA_KEM_Encryption_Operation final : public PK_Ops::KEM_Encryption_with_KD
          const BigInt r = BigInt::random_integer(rng, 1, get_n());
          const BigInt c = public_op(r);
 
-         out_encapsulated_key = BigInt::encode_locked(c);
-         raw_shared_key = BigInt::encode_locked(r);
+         out_encapsulated_key = BigInt::encode_1363(c, public_modulus_bytes());
+         raw_shared_key = BigInt::encode_1363(r, public_modulus_bytes());
          }
    };
 


Test patch:
diff --git a/src/tests/unit_tls.cpp b/src/tests/unit_tls.cpp
index a255e3ace..4dd291d01 100644
--- a/src/tests/unit_tls.cpp
+++ b/src/tests/unit_tls.cpp
@@ -3,7 +3,6 @@
 *     2016 Matthias Gierlings
 *     2017 René Korthaus, Rohde & Schwarz Cybersecurity
 *     2017 Harry Reimann, Rohde & Schwarz Cybersecurity
-*     2023 René Meusel, Rohde & Schwarz Cybersecurity
 *
 * Botan is released under the Simplified BSD License (see license.txt)
 */
@@ -18,7 +17,6 @@
    #include <botan/tls_server.h>
    #include <botan/tls_policy.h>
    #include <botan/tls_extensions.h>
-   #include <botan/tls_exceptn.h>
    #include <botan/internal/tls_reader.h>
 
    #include <botan/ec_group.h>
@@ -418,31 +416,6 @@ class TLS_Handshake_Test final
       void go();
 
       const Test::Result& results() const { return m_results; }
-
-      void set_custom_client_tls_session_established_callback(std::function<void(const Botan::TLS::Session&)> clbk)
-         {
-         BOTAN_ASSERT_NONNULL(m_client_cb);
-         m_client_cb->set_custom_tls_session_established_callback(std::move(clbk));
-         }
-
-      void set_custom_server_tls_session_established_callback(std::function<void(const Botan::TLS::Session&)> clbk)
-         {
-         BOTAN_ASSERT_NONNULL(m_server_cb);
-         m_server_cb->set_custom_tls_session_established_callback(std::move(clbk));
-         }
-
-      void set_client_expected_handshake_alert(Botan::TLS::Alert alert)
-         {
-         BOTAN_ASSERT_NONNULL(m_client_cb);
-         m_client_cb->set_expected_handshake_alert(alert);
-         }
-
-      void set_server_expected_handshake_alert(Botan::TLS::Alert alert)
-         {
-         BOTAN_ASSERT_NONNULL(m_server_cb);
-         m_server_cb->set_expected_handshake_alert(alert);
-         }
-
    private:
 
       class Test_Extension : public Botan::TLS::Extension
@@ -482,18 +455,9 @@ class TLS_Handshake_Test final
                m_results(results),
                m_expected_version(expected_version),
                m_outbound(outbound),
-               m_recv(recv_buf),
-               m_expected_handshake_alert(Botan::TLS::Alert::NULL_ALERT) // TODO: in Botan 3.0 this should be a std::optional
+               m_recv(recv_buf)
                {}
 
-            ~Test_Callbacks()
-               {
-               if(m_expected_handshake_alert.type() != Botan::TLS::Alert::NULL_ALERT)
-                  {
-                  m_results.test_failure("Expected: " + m_expected_handshake_alert.type_string() + " during handshake");
-                  }
-               }
-
             void tls_emit_data(const uint8_t bits[], size_t len) override
                {
                m_outbound.insert(m_outbound.end(), bits, bits + len);
@@ -504,24 +468,10 @@ class TLS_Handshake_Test final
                m_recv.insert(m_recv.end(), bits, bits + len);
                }
 
-            void tls_alert(Botan::TLS::Alert alert) override
+            void tls_alert(Botan::TLS::Alert /*alert*/) override
                {
                // TODO test that it is a no_renegotiation alert
-
-               if(m_expected_handshake_alert.type() != Botan::TLS::Alert::NULL_ALERT)
-                  {
-                  if(m_expected_handshake_alert.type() != alert.type())
-                     {
-                     m_results.test_failure("Got unexpected alert: " + alert.type_string() +
-                                            " expected: " + m_expected_handshake_alert.type_string());
-                     }
-                  else
-                     {
-                     // acknowledge that the expected Alert was detected
-                     m_results.test_note("saw expected alert: " + m_expected_handshake_alert.type_string());
-                     m_expected_handshake_alert = Botan::TLS::Alert::NULL_ALERT;
-                     }
-                  }
+               // ignore
                }
 
             void tls_modify_extensions(Botan::TLS::Extensions& extn, Botan::TLS::Connection_Side which_side) override
@@ -579,11 +529,6 @@ class TLS_Handshake_Test final
 
                m_results.test_note(session_report);
 
-               if(m_session_established_callback)
-                  {
-                  m_session_established_callback(session);
-                  }
-
                if(session.version() != m_expected_version)
                   {
                   m_results.test_failure("Expected " + m_expected_version.to_string() +
@@ -609,24 +554,11 @@ class TLS_Handshake_Test final
                return Botan::TLS::Callbacks::tls_decode_group_param(group_param);
                }
 
-            void set_custom_tls_session_established_callback(std::function<void(const Botan::TLS::Session&)> clbk)
-               {
-               m_session_established_callback = std::move(clbk);
-               }
-
-            void set_expected_handshake_alert(Botan::TLS::Alert alert)
-               {
-               m_expected_handshake_alert = alert;
-               }
-
          private:
             Test::Result& m_results;
             const Botan::TLS::Protocol_Version m_expected_version;
             std::vector<uint8_t>& m_outbound;
             std::vector<uint8_t>& m_recv;
-
-            std::function<void(const Botan::TLS::Session&)> m_session_established_callback;
-            Botan::TLS::Alert m_expected_handshake_alert;
          };
 
       const Botan::TLS::Protocol_Version m_offer_version;
@@ -695,6 +627,11 @@ void TLS_Handshake_Test::go()
       if(server_handshake_completed == false && m_server->is_active())
          server_handshake_completed = true;
 
+      if(client->is_closed() || m_server->is_closed())
+         {
+         break;
+         }
+
       if(client->is_active() && client_has_written == false)
          {
          m_results.test_eq("client ALPN protocol", client->application_protocol(), "test/3");
@@ -742,13 +679,8 @@ void TLS_Handshake_Test::go()
          std::vector<uint8_t> input;
          std::swap(m_c2s, input);
 
-         try
-            {
-            size_t needed = m_server->received_data(input.data(), input.size());
-            m_results.test_eq("full packet received", needed, 0);
-            }
-         catch(...)
-            { /* ignore exceptions */ }
+         size_t needed = m_server->received_data(input.data(), input.size());
+         m_results.test_eq("full packet received", needed, 0);
 
          continue;
          }
@@ -758,13 +690,8 @@ void TLS_Handshake_Test::go()
          std::vector<uint8_t> input;
          std::swap(m_s2c, input);
 
-         try
-            {
-            size_t needed = client->received_data(input.data(), input.size());
-            m_results.test_eq("full packet received", needed, 0);
-            }
-         catch(...)
-            { /* ignore exceptions */ }
+         size_t needed = client->received_data(input.data(), input.size());
+         m_results.test_eq("full packet received", needed, 0);
 
          continue;
          }
@@ -999,80 +926,6 @@ class TLS_Unit_Tests final : public Test
          return test_with_policy(test_descr, results, client_ses, server_ses, creds, versions, policy, client_auth);
          }
 
-      void test_session_established_abort(std::vector<Test::Result>& results,
-                                          Botan::Credentials_Manager& creds,
-                                          Botan::RandomNumberGenerator& rng)
-         {
-         std::vector<Botan::TLS::Protocol_Version> versions =
-            {
-            Botan::TLS::Protocol_Version::TLS_V12,
-            Botan::TLS::Protocol_Version::DTLS_V12
-            };
-
-         Test_Policy policy;
-         Botan::TLS::Session_Manager_Noop noop_session_manager;
-
-         auto client_aborts = [&](std::exception_ptr ex, Botan::TLS::Alert expected_server_alert)
-            {
-            for(const auto version : versions)
-               {
-               TLS_Handshake_Test test(
-                  "Client aborts in tls_session_established with " +
-                  expected_server_alert.type_string() + ": " + version.to_string(),
-                  version, creds, policy, policy, rng, noop_session_manager, noop_session_manager, false);
-               test.set_custom_client_tls_session_established_callback([=](const Botan::TLS::Session&)
-                  {
-                  std::rethrow_exception(ex);
-                  });
-               test.set_server_expected_handshake_alert(expected_server_alert);
-
-               test.go();
-               results.push_back(test.results());
-               }
-            };
-
-         auto server_aborts = [&](std::exception_ptr ex, Botan::TLS::Alert expected_server_alert)
-            {
-            for(const auto version : versions)
-               {
-               TLS_Handshake_Test test(
-                  "Server aborts in tls_session_established with " +
-                  expected_server_alert.type_string() + ": " + version.to_string(),
-                  version, creds, policy, policy, rng, noop_session_manager, noop_session_manager, false);
-               test.set_custom_server_tls_session_established_callback([=](const Botan::TLS::Session&)
-                  {
-                  std::rethrow_exception(ex);
-                  });
-               test.set_client_expected_handshake_alert(expected_server_alert);
-
-               test.go();
-               results.push_back(test.results());
-               }
-            };
-
-         client_aborts(std::make_exception_ptr(
-                           Botan::TLS::TLS_Exception(Botan::TLS::Alert::ACCESS_DENIED,
-                                                     "some test TLS exception")),
-                       Botan::TLS::Alert::ACCESS_DENIED);
-         client_aborts(std::make_exception_ptr(
-                           Botan::Invalid_Authentication_Tag("some symmetric crypto failed :o)")),
-                       Botan::TLS::Alert::BAD_RECORD_MAC);
-         client_aborts(std::make_exception_ptr(
-                           std::runtime_error("something strange happened")),
-                       Botan::TLS::Alert::INTERNAL_ERROR);
-
-         server_aborts(std::make_exception_ptr(
-                           Botan::TLS::TLS_Exception(Botan::TLS::Alert::ACCESS_DENIED,
-                                                     "some server test TLS exception")),
-                       Botan::TLS::Alert::ACCESS_DENIED);
-         server_aborts(std::make_exception_ptr(
-                           Botan::Invalid_Authentication_Tag("some symmetric crypto failed in the server :o)")),
-                       Botan::TLS::Alert::BAD_RECORD_MAC);
-         server_aborts(std::make_exception_ptr(
-                           std::runtime_error("something strange happened in the server")),
-                       Botan::TLS::Alert::INTERNAL_ERROR);
-         }
-
    public:
       std::vector<Test::Result> run() override
          {
@@ -1272,11 +1125,6 @@ class TLS_Unit_Tests final : public Test
          test_modern_versions("AES-256/GCM secp112r1", results, *client_ses, *server_ses, *creds, "ECDH", "AES-256/GCM", "AEAD",
                               { { "groups", "0xFEE1" }, { "minimum_ecdh_group_size", "112" } });
 
-         // Test connection abort by the application
-         // by throwing in Callbacks::tls_session_established()
-
-         test_session_established_abort(results, *creds, rng);
-
          return results;
          }