Fix out-of-bounds reads in TLV parsing and v1 header creation

[kosmas-valianos]

Fixes four memory-safety / robustness bugs found with AddressSanitizer + UBSan (some via fuzzing the create path):

1. Parsing (pp_parse_hdr) — a US-ASCII TLV (e.g. NETNS, SSL sub-TLVs) copied length + 1 bytes from its value, reading one byte past the input buffer when the TLV was the last thing in the buffer. tlv_new() now takes a separate copy length so it allocates length + 1 for the trailing NUL but copies only length bytes. Reachable from a crafted network header.

2. Parsing (US-ASCII TLV length inflation) — the trailing NUL added for the getters was also written into the on-wire length field, so parsed US-ASCII TLVs reported a length one byte too big and, on re-serialisation, emitted the NUL on the wire and grew the header — inflation that compounded by one byte on every parse → create hop. The stored length is now the true value length (NUL no longer counted), matching the documented getter contract; the buffer stays NUL-terminated.

3. Creating (pp_create_hdr, v1) — addresses were copied into fixed 40-byte buffers and sprintf'd. A 45-char IPv4-mapped IPv6 address (e.g. ffff:...:255.255.255.255) overflowed those buffers and the line buffer, and produced a header too long for the v1 parser to read back. Addresses are now normalised via inet_ntop() to the canonical (<=39-char) form; the line length is bounded before formatting (the non-Windows _sprintf uses unbounded vsprintf, as ANSI C has no snprintf) and the _sprintf return value is validated so a sprintf_s failure on Windows cannot drive an oversized malloc/memcpy.

4. Creating (pp_create_hdr, v2 alignment) — pp2_info.alignment_power is a caller-set uint8_t. Values >= 16 made 1 << alignment_power overflow the uint16_t alignment (1 << 16 wraps to 0 → division by zero; 1 << 31 is signed-integer overflow). Separately, rounding the header length up to the alignment could exceed UINT16_MAX even for an in-range power, wrapping the uint16_t padded length to a tiny value and driving an undersized malloc followed by a TLV memcpy past the allocation (heap overflow, confirmed under ASan with alignment_power = 12). The power is now capped at 15 and the padded length is computed in a wider type and rejected with -ERR_PP2_LENGTH if it would exceed UINT16_MAX.

Also tidied the v1 header code: the repeated family → IP-error-code ternary is now a small helper, the redundant post-sprintf bound check was dropped (the length is already bounded before formatting), and the v1 IPv4-mapped IPv6 regression asserts against hard-coded RFC 5952 canonical literals instead of recomputing them with the same inet_pton/inet_ntop calls under test.

Added regression tests for all of the above; the full suite passes under ASan/UBSan and the library compiles clean under -std=c89 -pedantic-errors.

[Copilot]

Pull request overview

This PR addresses two ASan-confirmed memory-safety issues in the PROXY protocol implementation: (1) a 1-byte out-of-bounds read while parsing US-ASCII TLVs at the end of a v2 header buffer, and (2) potential buffer overflows when creating v1 headers from long (non-canonical) IPv6 textual forms by normalizing addresses into their canonical inet_ntop() representation.

Changes:

• Fix US-ASCII TLV parsing to allocate length + 1 but copy only length bytes, then append the trailing NUL.
• Fix v1 header creation to normalize src_addr/dst_addr with inet_pton() + inet_ntop() to ensure canonical bounded textual forms.
• Add regression tests covering both issues.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/proxy_protocol.c	Fixes TLV copy semantics for US-ASCII values and normalizes v1 addresses via inet_ntop() to prevent buffer overflow.
tests/test.c	Adds regression tests for the US-ASCII TLV-at-end parsing case and v1 creation/round-trip with long IPv4-mapped IPv6 inputs.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/proxy_protocol.c:816

• In the v2 create padding logic, pp2_hdr_len_padded is computed in a uint16_t. For large headers with alignment_power == 15, the next multiple can be 65536, which wraps to 0 in uint16_t and can lead to incorrect padding_bytes/len calculations and potentially out-of-bounds writes. Compute the padded length in a wider type and reject results > UINT16_MAX (also after the extra += alignment for the minimum NOOP TLV size).

        uint16_t alignment = 1 << pp_info->pp2_info.alignment_power;
        if (*pp2_hdr_len % alignment)
        {
            uint16_t pp2_hdr_len_padded = (*pp2_hdr_len / alignment + 1) * alignment;
            /* The NOOP TLV needs to be at least 3 bytes because a TLV can not be smaller than that */

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.