Repository: randombit/botan
Pull request #3116 (2022-12-14 10:11:51)

[TLS 1.3] Server honors the Record Size Limit Extension
## Pull Request Dependencies

* #3062
* #3053

## Description

This reacts on a negotiated `record_size_limit` extension ([RFC 8449](https://www.rfc-editor.org/rfc/rfc8449.html)) on the server side. It causes all encrypted records to adhere to the record size limit requested by the client.

I piggy-backed a test for this on the `tls_socket` CLI test. This was the closest "full roundtrip integration test". Unfortunately, BoGo does not include coverage for `record_size_limit`.

Edit/TODO: Probably it would be better to use the `test_tls.cpp` for the integration test. Though, this one first needs adaptions for TLS 1.3.

codecov-commenter: # [Codecov](https://codecov.io/gh/randombit/botan/pull/3116?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd) Report
Base: **87.91**% // Head: **91.78**% // Increases project coverage by **`+3.87%`** :tada:
> Coverage data is based on head [(`86f08d3`)](https://codecov.io/gh/randombit/botan/pull/3116?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd) compared to base [(`08c54f5`)](https://codecov.io/gh/randombit/botan/commit/08c54f5160b116a69804904a7600ea2fe4a0a057?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd).
> Patch coverage: 90.51% of modified lines in pull request are covered.

> :exclamation: Current head 86f08d3 differs from pull request most recent head 208ce89. Consider uploading reports for the commit 208ce89 to get more accurate results

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #3116      +/-   ##
==========================================
+ Coverage   87.91%   91.78%   +3.87%     
==========================================
  Files         599      600       +1     
  Lines       66039    72824    +6785     
  Branches     6609     6673      +64     
==========================================
+ Hits        58059    66845    +8786     
- Misses       5190     5979     +789     
+ Partials     2790        0    -2790     
```


| [Impacted Files](https://codecov.io/gh/randombit/botan/pull/3116?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd) | Coverage Δ | |
|---|---|---|
| [src/lib/tls/tls\_session.cpp](https://codecov.io/gh/randombit/botan/pull/3116/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2xpYi90bHMvdGxzX3Nlc3Npb24uY3Bw) | `89.43% <0.00%> (-3.10%)` | :arrow_down: |
| [src/lib/tls/tls\_client.cpp](https://codecov.io/gh/randombit/botan/pull/3116/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2xpYi90bHMvdGxzX2NsaWVudC5jcHA=) | `86.56% <42.85%> (-3.44%)` | :arrow_down: |
| [src/bogo\_shim/bogo\_shim.cpp](https://codecov.io/gh/randombit/botan/pull/3116/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2JvZ29fc2hpbS9ib2dvX3NoaW0uY3Bw) | `87.88% <66.66%> (+14.67%)` | :arrow_up: |
| [src/lib/tls/tls\_server.cpp](https://codecov.io/gh/randombit/botan/pull/3116/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2xpYi90bHMvdGxzX3NlcnZlci5jcHA=) | `70.96% <74.28%> (+3.40%)` | :arrow_up: |
| [src/cli/tls\_client.cpp](https://codecov.io/gh/randombit/botan/pull/3116/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2NsaS90bHNfY2xpZW50LmNwcA==) | `75.00% <75.00%> (+15.00%)` | :arrow_up: |
| [src/lib/tls/tls13/tls\_server\_impl\_13.cpp](https://codecov.io/gh/randombit/botan/pull/3116/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2xpYi90bHMvdGxzMTMvdGxzX3NlcnZlcl9pbXBsXzEzLmNwcA==) | `83.19% <83.19%> (ø)` | |
| [src/lib/tls/tls13/tls\_extensions\_key\_share.cpp](https://codecov.io/gh/randombit/botan/pull/3116/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2xpYi90bHMvdGxzMTMvdGxzX2V4dGVuc2lvbnNfa2V5X3NoYXJlLmNwcA==) | `88.16% <83.60%> (+5.23%)` | :arrow_up: |
| [src/lib/tls/tls13/msg\_encrypted\_extensions.cpp](https://codecov.io/gh/randombit/botan/pull/3116/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2xpYi90bHMvdGxzMTMvbXNnX2VuY3J5cHRlZF9leHRlbnNpb25zLmNwcA==) | `93.10% <88.23%> (+2.19%)` | :arrow_up: |
| [src/tests/test\_tls\_stream\_integration.cpp](https://codecov.io/gh/randombit/botan/pull/3116/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL3Rlc3RzL3Rlc3RfdGxzX3N0cmVhbV9pbnRlZ3JhdGlvbi5jcHA=) | `92.50% <89.07%> (-3.85%)` | :arrow_down: |
| [src/lib/tls/msg\_client\_hello.cpp](https://codecov.io/gh/randombit/botan/pull/3116/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2xpYi90bHMvbXNnX2NsaWVudF9oZWxsby5jcHA=) | `92.39% <93.03%> (+11.26%)` | :arrow_up: |
| ... and [605 more](https://codecov.io/gh/randombit/botan/pull/3116/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd) | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/randombit/botan/pull/3116?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd).


Patch:
diff --git a/src/lib/tls/tls13/tls_server_impl_13.cpp b/src/lib/tls/tls13/tls_server_impl_13.cpp
index cbcd2c792..937c26dbd 100644
--- a/src/lib/tls/tls13/tls_server_impl_13.cpp
+++ b/src/lib/tls/tls13/tls_server_impl_13.cpp
@@ -164,8 +164,32 @@ void Server_Impl_13::handle_reply_to_client_hello(const Server_Hello_13& server_
                                         policy(),
                                         callbacks(),
                                         rng())))
-      .add(m_handshake_state.sending(Finished_13(m_cipher_state.get(), m_transcript_hash.current())))
-      .send();
+      .add(m_handshake_state.sending(Finished_13(m_cipher_state.get(), m_transcript_hash.current())));
+
+   if(client_hello.extensions().has<Record_Size_Limit>() &&
+         m_handshake_state.encrypted_extensions().extensions().has<Record_Size_Limit>())
+      {
+      // RFC 8449 4.
+      //    When the "record_size_limit" extension is negotiated, an endpoint
+      //    MUST NOT generate a protected record with plaintext that is larger
+      //    than the RecordSizeLimit value it receives from its peer.
+      //    Unprotected messages are not subject to this limit.
+      //
+      // Hence, the limit is set just before we start sending encrypted records.
+      //
+      // RFC 8449 4.
+      //     The record size limit only applies to records sent toward the
+      //     endpoint that advertises the limit.  An endpoint can send records
+      //     that are larger than the limit it advertises as its own limit.
+      //
+      // Hence, the "outgoing" limit is what the client requested and the
+      // "incoming" limit is what we will request in the Encrypted Extensions.
+      const auto outgoing_limit = client_hello.extensions().get<Record_Size_Limit>();
+      const auto incoming_limit = m_handshake_state.encrypted_extensions().extensions().get<Record_Size_Limit>();
+      set_record_size_limits(outgoing_limit->limit(), incoming_limit->limit());
+      }
+
+   flight.send();
 
    m_cipher_state->advance_with_server_finished(m_transcript_hash.current());
    }


Test patch:
diff --git a/src/scripts/test_cli.py b/src/scripts/test_cli.py
index a0734b9e5..a47dfd40c 100755
--- a/src/scripts/test_cli.py
+++ b/src/scripts/test_cli.py
@@ -851,16 +851,18 @@ MCACAQUTBnN0cmluZzEGAQH/AgFjBAUAAAAAAAMEAP///w==
     test_cli("asn1print", "--pem -", expected, input_pem)
 
 def cli_tls_socket_tests(tmp_dir):
+    # pylint: disable=too-many-locals
     if not run_socket_tests() or not check_for_command("tls_client") or not check_for_command("tls_server"):
         return
 
-    client_msg = b'Client message %d\n' % (random.randint(0, 2**128))
+    client_msg = b'Client message %d with extra stuff to test record_size_limit: %s\n' % (random.randint(0, 2**128), b'oO' * 64)
     server_port = random_port_number()
 
     priv_key = os.path.join(tmp_dir, 'priv.pem')
     ca_cert = os.path.join(tmp_dir, 'ca.crt')
     crt_req = os.path.join(tmp_dir, 'crt.req')
     server_cert = os.path.join(tmp_dir, 'server.crt')
+    tls_policy = os.path.join(tmp_dir, 'test_policy.txt')
 
     test_cli("keygen", ["--algo=ECDSA", "--params=secp256r1", "--output=" + priv_key], "")
 
@@ -884,9 +886,25 @@ def cli_tls_socket_tests(tmp_dir):
     time.sleep(wait_time)
 
     for tls_version in ['1.2', '1.3']:
+        with open(tls_policy, 'w', encoding='utf8') as f:
+            # At the moment, TLS 1.2 does not implement record_size_limit.
+            # Therefore, clients can offer it only with TLS 1.2 being disabled.
+            # Otherwise, a server negotiating TLS 1.2 and using the record_size_limit
+            # would not work for us.
+            #
+            # TODO: Remove this crutch after implementing record_size_limit for TLS 1.2
+            #       and extend the test to use it for both TLS 1.2 and 1.3.
+            if tls_version == '1.3':
+                f.write("allow_tls12=false\n")
+                f.write("allow_tls13=true\n")
+                f.write("record_size_limit=64\n")
+            else:
+                f.write("allow_tls12=true\n")
+                f.write("allow_tls13=true\n")
+
         tls_client = subprocess.Popen([CLI_PATH, 'tls_client', 'localhost',
                                        '--port=%d' % (server_port), '--trusted-cas=%s' % (ca_cert),
-                                       '--tls-version=%s' % (tls_version)],
+                                       '--tls-version=%s' % (tls_version), '--policy=%s' % (tls_policy)],
                                       stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
 
         time.sleep(wait_time)