Copilot commented on code in PR #13117:
URL: https://github.com/apache/trafficserver/pull/13117#discussion_r3134597973
##########
src/iocore/net/TLSCertCompression.h:
##########
@@ -27,6 +27,11 @@
#include <vector>
#include <string>
+// RFC 8879 uses uint24 for uncompressed_length, allowing up to ~16 MB.
+// Real certificate chains are well under 64 KB — cap allocation to
+// prevent a decompression bomb from a malicious peer.
Review Comment:
The comment states certificate chains are "well under 64 KB", but the cap is
128 KB and certificate chains can exceed 64 KB in practice. Consider rewording
to avoid an absolute size claim (e.g., describe this as a typical/expected
range) and/or briefly justify why 128 KB is the chosen cap.
```suggestion
// Certificate chains are typically much smaller than this, but can exceed
64 KB
// in practice. Cap allocation at 128 KB to allow larger real-world chains
while
// still limiting exposure to a decompression bomb from a malicious peer.
```
##########
src/iocore/net/TLSCertCompression_zstd.cc:
##########
@@ -60,6 +61,12 @@ compression_func_zstd(SSL * /* ssl */, CBB *out, const
uint8_t *in, size_t in_le
int
decompression_func_zstd(SSL * /* ssl */, CRYPTO_BUFFER **out, size_t
uncompressed_len, const uint8_t *in, size_t in_len)
{
+ if (uncompressed_len > MAX_CERT_UNCOMPRESSED_LEN) {
+ *out = nullptr;
+ Metrics::Counter::increment(ssl_rsb.cert_decompress_zstd_failure);
+ return 0;
+ }
Review Comment:
This introduces a new failure mode (rejecting cert decompression when
uncompressed_len exceeds MAX_CERT_UNCOMPRESSED_LEN), but there doesn't appear
to be a test exercising the oversize path. Please add coverage (e.g., extend
the existing TLS cert compression gold test or add a focused test) to ensure
the handshake fails and the *_failure metric is incremented when the cap is
exceeded.
##########
src/iocore/net/TLSCertCompression_zlib.cc:
##########
@@ -53,6 +54,12 @@ compression_func_zlib(SSL * /* ssl */, CBB *out, const
uint8_t *in, size_t in_le
int
decompression_func_zlib(SSL * /* ssl */, CRYPTO_BUFFER **out, size_t
uncompressed_len, const uint8_t *in, size_t in_len)
{
+ if (uncompressed_len > MAX_CERT_UNCOMPRESSED_LEN) {
+ *out = nullptr;
+ Metrics::Counter::increment(ssl_rsb.cert_decompress_zlib_failure);
+ return 0;
+ }
Review Comment:
This introduces a new failure mode (rejecting cert decompression when
uncompressed_len exceeds MAX_CERT_UNCOMPRESSED_LEN), but there doesn't appear
to be a test exercising the oversize path. Please add coverage (e.g., extend
the existing TLS cert compression gold test or add a focused test) to ensure
the handshake fails and the *_failure metric is incremented when the cap is
exceeded.
##########
src/iocore/net/TLSCertCompression_brotli.cc:
##########
@@ -55,6 +56,12 @@ compression_func_brotli(SSL * /* ssl */, CBB *out, const
uint8_t *in, size_t in_
int
decompression_func_brotli(SSL * /* ssl */, CRYPTO_BUFFER **out, size_t
uncompressed_len, const uint8_t *in, size_t in_len)
{
+ if (uncompressed_len > MAX_CERT_UNCOMPRESSED_LEN) {
+ *out = nullptr;
+ Metrics::Counter::increment(ssl_rsb.cert_decompress_brotli_failure);
+ return 0;
+ }
Review Comment:
This introduces a new failure mode (rejecting cert decompression when
uncompressed_len exceeds MAX_CERT_UNCOMPRESSED_LEN), but there doesn't appear
to be a test exercising the oversize path. Please add coverage (e.g., extend
the existing TLS cert compression gold test or add a focused test) to ensure
the handshake fails and the *_failure metric is incremented when the cap is
exceeded.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
