Copilot commented on code in PR #12864:
URL: https://github.com/apache/trafficserver/pull/12864#discussion_r2784954952
##########
src/iocore/net/ProxyProtocol.cc:
##########
@@ -547,6 +547,80 @@ ProxyProtocol::get_tlv(const uint8_t tlvCode) const
return std::nullopt;
}
+/*
+ * PP2_TYPE_SSL
+ * struct pp2_tlv_ssl {
+ * uint8_t client;
+ * uint32_t verify;
+ * struct pp2_tlv sub_tlv[0];
+ * };
+ */
+
+std::optional<std::string_view>
+ProxyProtocol::_get_tlv_ssl_subtype(int subtype) const
+{
+ if (auto v = tlv.find(PP2_TYPE_SSL); v != tlv.end() && v->second.length() !=
0) {
+ auto ssl = v->second;
+
+ // Is the client connected over TLS
+ if ((ssl.data()[0] & 0x01) == 0) {
+ // Not over TLS
+ return std::nullopt;
+ }
+
+ // Find the given subtype
+ uint16_t len = ssl.length();
+ const char *p = ssl.data() + 5; // Skip client (uint8_t) + verify
(uint32_t)
+ const char *end = p + len + 1;
+ while (p != end) {
Review Comment:
_get_tlv_ssl_subtype() computes the end pointer incorrectly (it uses `end =
p + len + 1` after already advancing `p` by 5). This makes the loop read past
the end of the PP2_TYPE_SSL TLV value and can cause out-of-bounds
reads/undefined behavior. Compute `end` from the start of the SSL value (e.g.,
`ssl.data() + ssl.size()`) and iterate while `p < end` (not `p != end`).
```suggestion
// The SSL TLV must contain at least the client (uint8_t) and verify
(uint32_t) fields.
if (ssl.length() < 5) {
Dbg(dbg_ctl_proxyprotocol_v2, "SSL TLV too short: %zu bytes (expected
at least 5)", static_cast<size_t>(ssl.length()));
return std::nullopt;
}
// Find the given subtype
uint16_t len = ssl.length();
const char *p = ssl.data() + 5; // Skip client (uint8_t) +
verify (uint32_t)
const char *end = ssl.data() + len; // End of the SSL TLV value
while (p < end) {
```
##########
doc/admin-guide/logging/formatting.en.rst:
##########
@@ -532,6 +532,10 @@ ppd Proxy Protocol Destination IP received via Proxy
Protocol context from the
Dest IP to the |TS|
ppa Proxy Protocol The Authority TLV from Proxy Protocol context from the LB
Authority to the |TS|
+pptc Proxy Protocol The TLS cipher from Proxy Protocol context from the LB
+ Authority to the |TS|
+pptv Proxy Protocol The TLS version from Proxy Protocol context from the LB
+ Authority to the |TS|
Review Comment:
The new log field table entries for `pptc`/`pptv` look like copy/paste from
`ppa`: the second column label is "Authority" for both, which doesn't match the
TLS cipher/version descriptions. Update the field name in the middle column to
something like "TLS Cipher" and "TLS Version" so the table is internally
consistent.
```suggestion
TLS Cipher to the |TS|
pptv Proxy Protocol The TLS version from Proxy Protocol context from the LB
TLS Version to the |TS|
```
##########
src/iocore/net/ProxyProtocol.cc:
##########
@@ -547,6 +547,80 @@ ProxyProtocol::get_tlv(const uint8_t tlvCode) const
return std::nullopt;
}
+/*
+ * PP2_TYPE_SSL
+ * struct pp2_tlv_ssl {
+ * uint8_t client;
+ * uint32_t verify;
+ * struct pp2_tlv sub_tlv[0];
+ * };
+ */
+
+std::optional<std::string_view>
+ProxyProtocol::_get_tlv_ssl_subtype(int subtype) const
+{
+ if (auto v = tlv.find(PP2_TYPE_SSL); v != tlv.end() && v->second.length() !=
0) {
+ auto ssl = v->second;
+
+ // Is the client connected over TLS
+ if ((ssl.data()[0] & 0x01) == 0) {
+ // Not over TLS
+ return std::nullopt;
+ }
+
+ // Find the given subtype
+ uint16_t len = ssl.length();
+ const char *p = ssl.data() + 5; // Skip client (uint8_t) + verify
(uint32_t)
Review Comment:
_get_tlv_ssl_subtype() reads `ssl.data()[0]` and skips 5 bytes without
verifying the TLV value is at least 5 bytes long. If PP2_TYPE_SSL is present
but truncated/malformed, this will access beyond the string_view. Add a
`ssl.size() >= 5` (and preferably `>= 5 + 3` before parsing sub-TLVs) guard
before dereferencing and parsing.
##########
include/iocore/net/ProxyProtocol.h:
##########
@@ -134,6 +136,8 @@ class ProxyProtocol
private:
std::string additional_data;
+
+ std::optional<std::string_view> _get_tlv_ssl_subtype(int subtype) const;
Review Comment:
ProxyProtocol::_get_tlv_ssl_subtype takes `int subtype`, but subtype IDs are
8-bit values on the wire (and `get_tlv()` uses `uint8_t`). Using `uint8_t` here
avoids accidental sign/width issues and better communicates the expected range.
```suggestion
std::optional<std::string_view> _get_tlv_ssl_subtype(uint8_t subtype)
const;
```
##########
src/proxy/logging/LogAccess.cc:
##########
@@ -1660,6 +1660,40 @@ LogAccess::marshal_proxy_protocol_authority(char *buf)
return 0;
}
+int
+LogAccess::marshal_proxy_protocol_tls_cipher(char *buf)
+{
+ int len = INK_MIN_ALIGN;
+
+ if (m_http_sm) {
+ if (auto cipher = m_http_sm->t_state.pp_info.get_tlv_ssl_cipher(); cipher)
{
+ len = padded_length(cipher->size() + 1);
+ if (buf) {
+ marshal_str(buf, cipher->data(), len);
+ buf[cipher->size()] = '\0';
+ }
+ }
+ }
+ return len;
+}
+
+int
+LogAccess::marshal_proxy_protocol_tls_version(char *buf)
+{
+ int len = INK_MIN_ALIGN;
+
+ if (m_http_sm) {
+ if (auto version = m_http_sm->t_state.pp_info.get_tlv_ssl_version();
version) {
+ len = padded_length(version->size() + 1);
+ if (buf) {
+ marshal_str(buf, version->data(), len);
+ buf[version->size()] = '\0';
+ }
+ }
+ }
Review Comment:
marshal_proxy_protocol_tls_cipher() returns `INK_MIN_ALIGN` when no cipher
is available but does not write anything into `buf` in that case. This can
leave uninitialized bytes in the log buffer. Consider always marshalling a
default string (e.g., "-") when `buf` is non-null, and return the padded length
that matches what was written.
```suggestion
int len = INK_MIN_ALIGN;
bool have_data = false;
if (m_http_sm) {
if (auto cipher = m_http_sm->t_state.pp_info.get_tlv_ssl_cipher();
cipher) {
have_data = true;
len = padded_length(cipher->size() + 1);
if (buf) {
marshal_str(buf, cipher->data(), len);
buf[cipher->size()] = '\0';
}
}
}
// When no cipher is available, ensure the buffer is initialized with a
default value.
if (buf && !have_data) {
buf[0] = '-';
// NUL-terminate and pad the remaining bytes in the field.
for (int i = 1; i < len; ++i) {
buf[i] = '\0';
}
}
return len;
}
int
LogAccess::marshal_proxy_protocol_tls_version(char *buf)
{
int len = INK_MIN_ALIGN;
bool have_data = false;
if (m_http_sm) {
if (auto version = m_http_sm->t_state.pp_info.get_tlv_ssl_version();
version) {
have_data = true;
len = padded_length(version->size() + 1);
if (buf) {
marshal_str(buf, version->data(), len);
buf[version->size()] = '\0';
}
}
}
// When no TLS version is available, ensure the buffer is initialized with
a default value.
if (buf && !have_data) {
buf[0] = '-';
// NUL-terminate and pad the remaining bytes in the field.
for (int i = 1; i < len; ++i) {
buf[i] = '\0';
}
}
```
##########
src/proxy/logging/LogAccess.cc:
##########
@@ -1660,6 +1660,40 @@ LogAccess::marshal_proxy_protocol_authority(char *buf)
return 0;
}
+int
+LogAccess::marshal_proxy_protocol_tls_cipher(char *buf)
+{
+ int len = INK_MIN_ALIGN;
+
+ if (m_http_sm) {
+ if (auto cipher = m_http_sm->t_state.pp_info.get_tlv_ssl_cipher(); cipher)
{
+ len = padded_length(cipher->size() + 1);
+ if (buf) {
+ marshal_str(buf, cipher->data(), len);
+ buf[cipher->size()] = '\0';
+ }
+ }
+ }
+ return len;
+}
+
+int
+LogAccess::marshal_proxy_protocol_tls_version(char *buf)
+{
+ int len = INK_MIN_ALIGN;
+
+ if (m_http_sm) {
+ if (auto version = m_http_sm->t_state.pp_info.get_tlv_ssl_version();
version) {
+ len = padded_length(version->size() + 1);
+ if (buf) {
+ marshal_str(buf, version->data(), len);
+ buf[version->size()] = '\0';
+ }
+ }
+ }
+ return len;
Review Comment:
marshal_proxy_protocol_tls_version() returns `INK_MIN_ALIGN` when the TLS
version TLV isn't present, but does not marshal any default value into `buf` in
that case. This can result in uninitialized log output for the field. Align the
behavior with other string marshalers by always writing a default string when
`buf` is provided and returning the corresponding padded length.
##########
src/proxy/logging/LogAccess.cc:
##########
@@ -1660,6 +1660,40 @@ LogAccess::marshal_proxy_protocol_authority(char *buf)
return 0;
}
+int
+LogAccess::marshal_proxy_protocol_tls_cipher(char *buf)
+{
+ int len = INK_MIN_ALIGN;
+
+ if (m_http_sm) {
+ if (auto cipher = m_http_sm->t_state.pp_info.get_tlv_ssl_cipher(); cipher)
{
+ len = padded_length(cipher->size() + 1);
+ if (buf) {
+ marshal_str(buf, cipher->data(), len);
+ buf[cipher->size()] = '\0';
+ }
+ }
+ }
+ return len;
+}
+
+int
+LogAccess::marshal_proxy_protocol_tls_version(char *buf)
+{
+ int len = INK_MIN_ALIGN;
+
+ if (m_http_sm) {
+ if (auto version = m_http_sm->t_state.pp_info.get_tlv_ssl_version();
version) {
+ len = padded_length(version->size() + 1);
+ if (buf) {
+ marshal_str(buf, version->data(), len);
+ buf[version->size()] = '\0';
+ }
Review Comment:
LogAccess::marshal_proxy_protocol_tls_version() has the same issue as the
cipher field: `version->data()` is a `std::string_view` that is not guaranteed
NUL-terminated, but marshal_str() will read until it finds a terminator. Use
marshal_mem() with the view length and padded length (or ensure NUL
termination) to avoid out-of-bounds reads.
##########
src/proxy/logging/LogAccess.cc:
##########
@@ -1660,6 +1660,40 @@ LogAccess::marshal_proxy_protocol_authority(char *buf)
return 0;
}
+int
+LogAccess::marshal_proxy_protocol_tls_cipher(char *buf)
+{
+ int len = INK_MIN_ALIGN;
+
+ if (m_http_sm) {
+ if (auto cipher = m_http_sm->t_state.pp_info.get_tlv_ssl_cipher(); cipher)
{
+ len = padded_length(cipher->size() + 1);
+ if (buf) {
+ marshal_str(buf, cipher->data(), len);
+ buf[cipher->size()] = '\0';
+ }
Review Comment:
LogAccess::marshal_proxy_protocol_tls_cipher() passes a non-NUL-terminated
`std::string_view` buffer (`cipher->data()`) into marshal_str(), which uses
`ink_strlcpy` and will read past the end of the view looking for a terminator.
Use marshal_mem() with the actual length (cipher->size()) and the padded
length, or otherwise ensure the input is NUL-terminated before calling
marshal_str().
--
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]
