stream: reject over-long paths in pouch_uplink_stream_open

[fderepas]

Summary

write_stream_header() writes an application-supplied path into a freshly
allocated block with no length check. A path longer than the block's
remaining payload capacity overflows the block buffer; a path longer than 255
bytes additionally corrupts the on-wire length prefix. This PR adds a bounds
check at the public API entry (pouch_uplink_stream_open) so an over-long path
is rejected cleanly instead of overrunning the buffer.

The problem

src/stream.c:

static void write_stream_header(struct pouch_buf *block, uint16_t content_type, const char *path)
{
    size_t path_len = strlen(path);

    pouch_put_be16(content_type, buf_claim(block, sizeof(uint16_t)));
    *buf_claim(block, 1) = path_len;                 /* 1-byte length prefix */
    buf_write(block, (uint8_t *) path, path_len);     /* unchecked copy */
}

path originates from the public API pouch_uplink_stream_open(const char *path, …)
and flows straight into the header write on a block obtained from
block_alloc_stream(). That block has a fixed payload capacity
(MAX_PLAINTEXT_BLOCK_SIZE, src/block.h), of which the block header
(BLOCK_HEADER_SIZE = 3) plus this stream sub-header (2-byte content_type +
1-byte length prefix = 3) are already consumed. The buf_write of the path is
therefore memory-safe only while:

strlen(path) <= MAX_PLAINTEXT_BLOCK_SIZE - BLOCK_HEADER_SIZE - (sizeof(uint16_t) + 1)   /* = 509 in the shipped config */

There is no runtime enforcement of this bound. Two distinct issues result:

1. Buffer overflow (memory safety). An application passing a path longer
   than the remaining block payload overflows the block. Impact is bounded to
   the uplink write path and the path is application/local-supplied (not
   attacker-controlled downlink input), so this is a defensive-robustness /
   API-contract gap rather than a remotely triggerable bug — but it is still an
   out-of-bounds write on a caller mistake.

2. Silent length-prefix truncation (protocol correctness). The length is
   stored in a single byte (*buf_claim(block, 1) = path_len, path_len is
   size_t). Any path in the 256–509 byte range is memory-safe but silently
   truncates the recorded length, producing a corrupt frame.

The wire format's 1-byte length prefix means paths longer than 255 bytes are
not representable in this protocol at all, so a single cap at 255 addresses
both issues at once (255 ≤ 509, so it also precludes the overflow).

The fix

Guard the public API entry so an unrepresentable/over-long path is rejected
before any allocation, consistent with the function's existing NULL-on-failure
contract:

--- a/src/stream.c
+++ b/src/stream.c
@@
 struct pouch_stream *pouch_uplink_stream_open(const char *path,
                                               uint16_t content_type,
                                               pouch_timeout_t timeout)
 {
+    /* The stream header stores the path length in a single byte and the path
+     * must fit in the block payload after the block + stream headers. Reject
+     * paths that cannot be represented, rather than overflowing the block or
+     * truncating the length prefix. */
+    if (strlen(path) > UINT8_MAX)
+    {
+        return NULL;
+    }
+
     if (pouch_atomic_inc(&open_streams) >= POUCH_STREAMS_MAX)
     {
         pouch_atomic_dec(&open_streams);
         return NULL;
     }

UINT8_MAX (255) is the length the 1-byte prefix can hold and is below the
509-byte block-capacity bound, so this single check prevents both the
out-of-bounds write and the prefix truncation. <stdint.h> (for UINT8_MAX)
and <string.h> (for strlen) are already in the translation unit.

Alternative

If longer stream paths are a requirement, the alternative is to widen the
length prefix on the wire (e.g. a 2-byte length) and keep the memory-safety
bound at MAX_PLAINTEXT_BLOCK_SIZE - BLOCK_HEADER_SIZE - (sizeof(uint16_t) + 2).
That is a protocol-format change (broker-side decoder must match) and is out of
scope for this hardening fix; the guard above keeps the current format correct.

How this was found

Surfaced by a deductive proof coupled with Squeeze Loop strategy: proving
write_stream_header free of out-of-bounds accesses required the caller
precondition strlen(path) <= 509, i.e. the body has no such check. A mutation
probe confirmed the bound is load-bearing — removing it re-introduces the
unproved buf_write payload-bound obligations (38/38 → 34/38). Under the added
guard, write_stream_header and pouch_uplink_stream_open are proven
memory-safe.

Reproducer

ASAN was used to create a self-contained reproducer that reuses the exact
shipped semantics — the struct pouch_buf { …; size_t bytes; uint8_t buf[]; }
layout, buf_alloc (malloc(sizeof(struct pouch_buf) + capacity)), the
bounds-check-free buf_claim/buf_write, and a verbatim write_stream_header
on a block of the shipped MAX_PLAINTEXT_BLOCK_SIZE capacity — and opens a
stream with a 1024-byte path (> the 509-byte bound):

cc -g -fsanitize=address pouch-fi-1-asan-test.c -o fi1 && ./fi1

ASan reports the out-of-bounds write on the very buf_write/write_stream_header
path (WRITE of size 1024 into a block allocated by … buf_alloc):

path length = 1024, block payload capacity = 515
==…==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x… WRITE of size 1024 …
    #0 … in memcpy
    #1 … in buf_write            pouch-fi-1-asan-test.c:34
    #2 … in write_stream_header  pouch-fi-1-asan-test.c:50
allocated by thread T0 here:
    #0 … in malloc
    #1 … in buf_alloc            pouch-fi-1-asan-test.c:24
    #2 … in block_alloc_stream   pouch-fi-1-asan-test.c:40
SUMMARY: AddressSanitizer: heap-buffer-overflow … in buf_write

With the guard applied (strlen(path) > UINT8_MAX → reject before the write),
the same 1024-byte path is refused and no out-of-bounds access occurs — ASan
runs clean.

Testing

• Existing stream/uplink tests continue to pass.
• The ASan reproducer above fails (heap-buffer-overflow) on the current code and
  passes clean with the guard — suitable as a regression under a sanitizer CI job.
• Suggested unit regression: open a stream with a path of length 256 and length
  MAX_PLAINTEXT_BLOCK_SIZE and assert pouch_uplink_stream_open returns NULL
  (previously: buffer overflow / truncated prefix).

[hasheddan]

@fderepas I believe this same issue is present in entry.c with write_entry() -- could you apply the fix there as well? It's also worth considering if this fix may be better suited to write_stream_header() above instead. It would avoid accidental reintroduction by another caller of the write_stream_header(), though it would come at the expense of potentially doing unnecessary work in pouch_uplink_stream_open() (i.e. opening the stream when we could have already determined that the path was invalid). That being said, given that failure here is evidence of a programmer's bug (using too long of a path), I'd be inclined to trade-off the potential performance improvements for safety.

[hasheddan]

@fderepas thanks for the update to entry.c! Is it your preference to not move the check here to write_stream_header() as I suggest?

[trond-snekvik]

Dan has one pending suggestion, but it's a subjective one, so I'm approving and leaving it up to you @fderepas.