fix: reject a traceparent that would slice a character in half - #690
Merged
Conversation
std.web installs its observe middleware on every app by default, and that middleware opens a server span through http.begin_server_span, which feeds any inbound `traceparent` header to trace.parse_traceparent. the parser checked the flags field's length in bytes and then sliced it by byte offset. len() counts bytes and substring() takes byte offsets, so the length check never proved the offset fell on a character boundary — and a slice through the middle of a multi-byte character is a runtime trap, not a recoverable error. one unauthenticated request carrying two extra bytes ended the server process with exit 1, and no catch could intercept it. every field of a traceparent is lowercase hex by the W3C spec, so validating it as hex is both correct and simpler than slicing: it proves the field is ascii, which proves every byte offset in it is a character boundary. a field that is not hex now makes the header fail to parse, so the server starts a fresh trace instead of joining a bogus one. std.net.grpc.bearer_token already guarded the same hazard the same way; this brings the traceparent parser in line with it. the sampled decision is now read as bit 0 of the flags byte, as the spec defines it, rather than as the second hex digit being exactly "1". a peer that sets another flag alongside sampled (`-03`) was previously read as unsampled.
the spec writes a traceparent's fields in lowercase, but an upstream that emits uppercase is easier to diagnose from a connected trace than from one that silently starts fresh. be liberal in what you accept: hex is now matched in either case. that does not weaken the safety property the hex check exists for. A-F is 0x41-0x46, so the accepted bytes are still every one below 0x80, while every byte of a multi-byte utf-8 sequence is 0x80 or above — the two sets stay disjoint, so proving a field is hex still proves it is all ascii, which still proves every byte offset in it falls on a character boundary. the ids are lowercased as they are parsed rather than stored as they arrived. accepting uppercase and re-emitting it verbatim would only move the interop problem one hop downstream, where a collector that rejects uppercase would then reject us. validation and normalisation happen in the same pass so the two cannot drift apart.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
one unauthenticated http request could end a pith web server process.
std.web.new()installs theobservemiddleware on every app. it opens aserver span through
http.begin_server_span, which reads the inboundtraceparentheader and hands it totrace.parse_traceparent. that parserchecked the flags field's length and then sliced it:
len()counts bytes andsubstring()takes byte offsets, so the length guardnever proved the offset fell on a character boundary. a multi-byte character in
the flags field was cut in half, and a cut through a character is a runtime
trap, not a recoverable error — the process exits 1 and no
catchcanintercept it.
the reachable chain is
web.observe(default middleware) ->http.begin_server_span->req.header("traceparent")->trace.parse_traceparent. so any app on the default observability middlewarewith tracing active — which is what
std.obs.init()turns on for any servicewith an OTLP endpoint configured — dies to two extra bytes from any client, with
no credentials and no valid route needed.
every field of a traceparent is lowercase hex by the W3C spec, so validating it
as hex is both correct and simpler than slicing: proving the field is hex proves
it is ascii, which proves every byte offset in it is a character boundary. a
field that is not hex now makes the header fail to parse, so the server starts a
fresh trace rather than joining a bogus one. the version, trace id and span id
are validated the same way — they were never sliced, but they were taken on
trust, and a 32-byte trace id containing a multi-byte character used to be
accepted and propagated verbatim into spans and exported telemetry.
std.net.grpc.bearer_tokenalready guards this exact hazard deliberately, witha comment saying so. this was the same hazard applied inconsistently, not a new
class of bug, and the fix follows that function's shape.
hex is matched in either case. the spec writes these fields in lowercase, but an
upstream that emits uppercase is easier to diagnose from a connected trace than
from one that silently starts fresh, so the header is joined — and the ids are
lowercased on the way in rather than stored as they arrived, because accepting
uppercase and re-emitting it verbatim would only move the interop problem one
hop downstream, where a collector that rejects uppercase would then reject us.
validation and normalisation run in the same pass so the two cannot drift apart.
accepting uppercase does not weaken the safety property:
A-Fis0x41-0x46,so the accepted bytes are still every one below
0x80, while every byte of amulti-byte utf-8 sequence is
0x80or above. the two sets stay disjoint, soproving a field is hex still proves it is all ascii.
one deliberate behaviour change comes with it: the sampled decision is now read
as bit 0 of the flags byte, as the spec defines it, rather than as the second
hex digit being exactly
"1"— a peer that set another flag alongside sampled(
-03) was previously read as unsampled. nothing that was well-formed under thespec parses differently.
what was tested
the fix was falsified before it was trusted. reverting only the flags handling
to the original line and running the parser directly reproduces the crash:
tests/cases/test_web_hostile_traceparent.pithis the end-to-end case, becausea unit test on the parser does not prove the middleware path is safe. it stands
up a real
web.new()app on a socket with tracing active and drives rawhttp/1.1 requests written by hand, so bytes no client would emit still arrive as
written. it sends a split character in the flags field, then in the trace id,
span id and version, then a burst of 48 more, checking after each round that the
server still answers — and finishes by checking a well-formed header is still
joined, since rejecting everything would pass every survival check and silently
break distributed tracing. with the guard reverted the run stops dead at the
first hostile request:
with the fix, on both backends:
joinedthere is the handler reporting that it ran under the trace id therequest carried, so propagation is proven end to end and not just at the parser.
five colocated tests in
std/trace.pithcover the parser itself: a multi-bytecharacter in each of the four fields, non-hex and wrong-width fields, uppercase
and mixed-case headers normalising to lowercase, both sampling decisions
round-tripping, and a parsed context still parenting the spans that follow it.
the multi-byte cases include a two-byte character (
é) in each field, which isthe one that fits the field's byte width exactly and so cannot be turned away by
a length check — only the hex check rejects it. that case is what proves
widening to uppercase did not accidentally admit a multi-byte character.
each guard was falsified independently, so no test passes for a reason other
than the one it claims:
normalisation is falsified end to end as well, not only in the parser: with the
lowercasing removed the server answers
uppercase: 200 freshinstead ofjoined, which is exactly the downstream interop break it exists to prevent.std/trace.pith: 23 passed 0 failed, and the end-to-end case green, under both
PITH_GREEN=1andPITH_GREEN=0. the regression suite runs clean.alongside this, the hpack decoder and
std.net.urlwere read for the sameshape, since both index by computed offsets rather than by delimiter positions.
hpack has no string slicing at all — it works in
Bytesthroughout, and its onebyte-to-string conversion returns a recoverable failure on invalid utf-8. all
six slice sites in
url.pithtake offsets that came from the position of anascii delimiter or from a
+1past a proven ascii byte, so every one of them isprovably on a character boundary. neither needed a change.