diff --git a/docs/telemetry.md b/docs/telemetry.md index c6347e6f..cd84834e 100644 --- a/docs/telemetry.md +++ b/docs/telemetry.md @@ -201,6 +201,19 @@ http.end_server_span(span, resp.status) if you're speaking a protocol the std clients don't cover, `format_traceparent` and `parse_traceparent` give you the raw header both ways. +an inbound header is untrusted input, so `parse_traceparent` validates it before +it trusts it: the version, trace id, span id, and flags must each be exactly +their spec width in hex, and anything else returns `none` and starts a fresh +trace rather than joining a bogus one. the sampled decision is read as bit 0 of +the flags byte, as the spec defines it, so a peer that sets another flag +alongside it (`-03`) is still understood as sampled. + +the spec writes the ids in lowercase, and `format_traceparent` emits them that +way, but an uppercase header is still accepted and joined — an upstream that +gets the case wrong is easier to diagnose from a connected trace than from a +silently broken one. the ids are lowercased as they are parsed, so what the +service propagates and exports is canonical whatever a peer sent. + ## OTLP export `std.obs.init()` reads the environment and wires everything up. the variables diff --git a/std/trace.pith b/std/trace.pith index dd976f04..776b927d 100644 --- a/std/trace.pith +++ b/std/trace.pith @@ -465,19 +465,75 @@ pub fn format_traceparent(ctx: SpanContext) -> String: flags = "01" return "00-" + ctx.trace_id + "-" + ctx.span_id + "-" + flags -# parse a W3C traceparent header into a context, or none when malformed. +HEX_LOWER := "0123456789abcdef" + +# the value of one hex digit in either case, or -1 when the character is not +# one. indexing a String yields one byte, so this is safe to call on any byte of +# any string, however the bytes were encoded. +fn hex_digit(character: String) -> Int: + code := bits.band(ord(character), 255) + if code >= 48 and code <= 57: + return code - 48 + if code >= 97 and code <= 102: + return code - 87 + if code >= 65 and code <= 70: + return code - 55 + return -1 + +# `field` as canonical lowercase hex, or none unless it is exactly `width` hex +# digits, as every field of a W3C traceparent must be. +# +# this is a safety check and not only a validation one: len() counts bytes and +# substring() takes byte offsets, so a length check alone never proves an offset +# falls on a character boundary, and slicing through the middle of a multi-byte +# character is a runtime trap that no catch can intercept. a traceparent arrives +# from the network unauthenticated, so proving the field is hex — hence all +# ascii, hence every byte offset is a boundary — is what keeps a hostile header +# from taking the process down. std.net.grpc.bearer_token guards the same hazard +# the same way. +# +# accepting uppercase does not weaken that: 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. +# +# the spec writes these fields in lowercase, so an uppercase one is joined but +# not passed on as it arrived: normalising here means what we propagate and +# export is canonical whatever a peer sent, rather than moving the interop +# problem one hop downstream. validating and normalising in the same pass keeps +# the two from drifting apart later. +fn hex_field(field: String, width: Int) -> String?: + if field.len() != width: + return none + mut canonical := "" + mut position := 0 + while position < width: + digit := hex_digit(field[position]) + if digit < 0: + return none + canonical = canonical + HEX_LOWER[digit] + position = position + 1 + return canonical + +# parse a W3C traceparent header into a context, or none when malformed. every +# field is validated as hex before it is read, so a malformed or hostile header +# is rejected rather than trusted or fatal, and the ids come back lowercased. pub fn parse_traceparent(header: String) -> SpanContext?: parts := header.split("-") if parts.len() < 4: return none + # the version is "00" exactly, which has no letter in it to case-fold. if parts[0] != "00": return none - trace_id := parts[1] - span_id := parts[2] - if trace_id.len() != 32 or span_id.len() != 16: + trace_id := hex_field(parts[1], 32) + span_id := hex_field(parts[2], 16) + flags := hex_field(parts[3], 2) + if trace_id == none or span_id == none or flags == none: return none - sampled := parts[3].len() >= 2 and parts[3].substring(1, 2) == "1" - return SpanContext(trace_id, span_id, sampled) + # the sampled decision is bit 0 of the flags byte, which is the low bit of + # the second hex digit. + flags_text := flags.value() + sampled := bits.band(hex_digit(flags_text[1]), 1) == 1 + return SpanContext(trace_id.value(), span_id.value(), sampled) test "spans nest through the per-thread current span": set_active(true) @@ -519,6 +575,118 @@ test "traceparent round-trips": assert(back.value().sampled) assert(parse_traceparent("garbage") == none) +test "a traceparent carrying a multi-byte character does not parse": + # a traceparent arrives from the network with no authentication behind it. + # len() counts bytes and substring() takes byte offsets, so the old length + # guard did not make the offsets safe: a multi-byte character in the flags + # field was cut in half, and a cut through a character is a runtime trap + # that ends the process — two bytes from any client. every field must now be + # rejected rather than sliced. + trace_id := "0af7651916cd43dd8448eb211c80319c" + span_id := "b7ad6b7169203331" + prefix := "00-" + trace_id + "-" + span_id + "-" + + # the flags field: the one that used to be sliced. + assert(parse_traceparent(prefix + "0€") == none) + assert(parse_traceparent(prefix + "€") == none) + assert(parse_traceparent(prefix + "€0") == none) + assert(parse_traceparent(prefix + "0é") == none) + # "é" is exactly two bytes, so it is the width the flags field wants and the + # hex check is the only thing that can turn it away. the same for a two-byte + # character sitting in a trace id or span id of exactly the right length. + assert(parse_traceparent(prefix + "é") == none) + assert(parse_traceparent("00-" + trace_id.substring(0, 30) + "é-" + span_id + "-01") == none) + assert(parse_traceparent("00-" + trace_id + "-" + span_id.substring(0, 14) + "é-01") == none) + + # the other fields, which were never sliced but were taken on trust. each of + # these is exactly the right byte length, so only a hex check rejects them. + bad_trace := trace_id.substring(0, 29) + "€" + bad_span := span_id.substring(0, 13) + "€" + assert_eq(bad_trace.len(), 32) + assert_eq(bad_span.len(), 16) + assert(parse_traceparent("00-" + bad_trace + "-" + span_id + "-01") == none) + assert(parse_traceparent("00-" + trace_id + "-" + bad_span + "-01") == none) + assert(parse_traceparent("0€-" + trace_id + "-" + span_id + "-01") == none) + +test "a traceparent field that is not hex does not parse": + trace_id := "0af7651916cd43dd8448eb211c80319c" + span_id := "b7ad6b7169203331" + prefix := "00-" + trace_id + "-" + span_id + "-" + assert(parse_traceparent(prefix + "zz") == none) + assert(parse_traceparent(prefix + "0") == none) + assert(parse_traceparent(prefix + "001") == none) + # "g" is the first letter past the hex alphabet in both cases. + assert(parse_traceparent(prefix + "0g") == none) + assert(parse_traceparent(prefix + "0G") == none) + assert(parse_traceparent("00-" + trace_id.substring(0, 31) + "z-" + span_id + "-01") == none) + assert(parse_traceparent("00-" + trace_id + "-" + span_id.substring(0, 15) + "z-01") == none) + +test "an uppercase traceparent is joined and normalised to lowercase": + # the spec writes these fields lowercase, but an upstream that emits + # uppercase is easier to diagnose joined than silently dropped. what we + # store is canonical either way, so what we propagate and export downstream + # does not carry the peer's spelling on to the next hop. + upper := parse_traceparent("00-0AF7651916CD43DD8448EB211C80319C-B7AD6B7169203331-01") + assert(upper != none) + assert_eq(upper.value().trace_id, "0af7651916cd43dd8448eb211c80319c") + assert_eq(upper.value().span_id, "b7ad6b7169203331") + assert(upper.value().sampled) + + # mixed case, and an uppercase flags digit carrying the sampled bit. + mixed := parse_traceparent("00-0Af7651916cD43dd8448Eb211c80319C-b7Ad6b7169203331-0B") + assert(mixed != none) + assert_eq(mixed.value().trace_id, "0af7651916cd43dd8448eb211c80319c") + assert_eq(mixed.value().span_id, "b7ad6b7169203331") + assert(mixed.value().sampled) + + # an uppercase flags digit with bit 0 clear is still not sampled. + even := parse_traceparent("00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-0A") + assert(even != none) + assert(not even.value().sampled) + + # a normalised id formats back out canonically, so the round trip is stable. + assert_eq(format_traceparent(upper.value()), "00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01") + +test "a well-formed traceparent still parses both sampling decisions": + trace_id := "0af7651916cd43dd8448eb211c80319c" + span_id := "b7ad6b7169203331" + + sampled := parse_traceparent("00-" + trace_id + "-" + span_id + "-01") + assert(sampled != none) + assert_eq(sampled.value().trace_id, trace_id) + assert_eq(sampled.value().span_id, span_id) + assert(sampled.value().sampled) + + plain := parse_traceparent("00-" + trace_id + "-" + span_id + "-00") + assert(plain != none) + assert_eq(plain.value().trace_id, trace_id) + assert_eq(plain.value().span_id, span_id) + assert(not plain.value().sampled) + + # sampled is bit 0 of the flags byte, not the whole second digit: a peer that + # sets another flag alongside it still reads as sampled. + both := parse_traceparent("00-" + trace_id + "-" + span_id + "-03") + assert(both != none) + assert(both.value().sampled) + other := parse_traceparent("00-" + trace_id + "-" + span_id + "-02") + assert(other != none) + assert(not other.value().sampled) + +test "a parsed traceparent still parents the spans that follow it": + set_active(true) + clear_context() + incoming := parse_traceparent("00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01") + assert(incoming != none) + with_context(incoming.value()) + + child := start("child") + assert_eq(child.trace_id, "0af7651916cd43dd8448eb211c80319c") + assert_eq(child.parent_id, "b7ad6b7169203331") + child.end() + + clear_context() + set_active(false) + test "typed attributes record their kind and value": set_active(true) clear_context() diff --git a/tests/cases/test_web_hostile_traceparent.pith b/tests/cases/test_web_hostile_traceparent.pith new file mode 100644 index 00000000..eb546168 --- /dev/null +++ b/tests/cases/test_web_hostile_traceparent.pith @@ -0,0 +1,152 @@ +# one unauthenticated http request used to be able to kill a pith web server. +# +# std.web installs its observe middleware on every app by default. it opens a +# server span through http.begin_server_span, which reads the inbound +# `traceparent` header and hands it to trace.parse_traceparent. that parser +# length-checked the flags field in bytes and then sliced it by byte offset — +# and a slice through the middle of a multi-byte character is a runtime trap, +# not a recoverable error, so the process exited 1 and no catch could stop it. +# two bytes from any client, no credentials. +# +# a unit test on the parser does not prove the middleware path is safe, so this +# drives the whole chain over a real socket: hostile headers first, then a +# well-formed one, then a plain request, checking after every hostile round that +# the server is still answering. it also checks that a valid traceparent is +# still joined, because rejecting everything would "fix" this and break tracing. + +import std.net.http as http +import std.net.tcp as tcp +import std.time as time +import std.trace as trace +import std.web as web + +PORT := 50933 + +TRACE_ID := "0af7651916cd43dd8448eb211c80319c" +SPAN_ID := "b7ad6b7169203331" + +# echo the trace id the request is running under, so the caller can tell a +# joined trace from a fresh one. +fn home(req: web.Request) -> http.HttpResponse: + return http.text(200, trace.current_context().trace_id) + +fn run_server(): + app := web.new().get("/", home) + app.listen("127.0.0.1", PORT) catch 0 + +# read until the 32-byte body has arrived. every response here carries a trace +# id, so the length is known and there is no need to hang waiting on EOF. +fn read_response(fd: Int) -> String: + mut buffer := "" + mut rounds := 0 + while rounds < 64: + cut := buffer.index_of("\r\n\r\n") + if cut >= 0 and buffer.len() - (cut + 4) >= 32: + break + chunk := tcp.read(fd, 4096) catch "" + if chunk == "": + break + buffer = buffer + chunk + rounds = rounds + 1 + return buffer + +# "200" for a normal answer, or a marker naming how the exchange failed. +fn status_of(response: String) -> String: + if response == "": + return "NO_RESPONSE" + space := response.index_of(" ") + if space < 0: + return "NO_STATUS" + rest := response.substring(space + 1, response.len()) + next := rest.index_of(" ") + if next < 0: + return "NO_STATUS" + return rest.substring(0, next) + +# the trace id the handler ran under: "joined" when it is the one we sent, +# "fresh" when the server started its own trace, and a marker otherwise. +fn trace_of(response: String) -> String: + cut := response.index_of("\r\n\r\n") + if cut < 0: + return "NO_BODY" + body := response.substring(cut + 4, response.len()) + if body == TRACE_ID: + return "joined" + if body.len() == 32: + return "fresh" + return "NO_TRACE" + +# one raw http/1.1 request, with `header` sent verbatim as the traceparent. it +# is written by hand rather than through the http client so that bytes no client +# would produce still reach the server exactly as written. +fn probe(header: String) -> String: + fd_r := tcp.connect("127.0.0.1", PORT) + if fd_r.is_err: + return "CONNECT_ERR" + fd := fd_r.ok + tcp.set_timeout(fd, 5000) + mut request := "GET / HTTP/1.1\r\nHost: 127.0.0.1\r\n" + if header != "": + request = request + "traceparent: " + header + "\r\n" + request = request + "\r\n" + written := tcp.write_all(fd, request) catch 0 + if written == 0: + tcp.close(fd) + return "WRITE_ERR" + response := read_response(fd) + tcp.close(fd) + return status_of(response) + " " + trace_of(response) + +# the spawned server is up once a plain request comes back answered. polling the +# thing under test is the readiness signal; a sleep would only be a guess. +fn wait_until_serving(timeout_ms: Int) -> String: + mut waited := 0 + while waited < timeout_ms: + got := probe("") + if got != "CONNECT_ERR": + return got + time.delay(25) + waited = waited + 25 + return "CONNECT_ERR" + +fn main() -> Int: + # the parser is only reached when tracing is on, which is what std.obs.init + # does for any service with an OTLP endpoint configured. + trace.set_active(true) + spawn run_server() + + print("ready: " + wait_until_serving(10000)) + + good := "00-" + TRACE_ID + "-" + SPAN_ID + "-" + + # the flags field cut mid-character: the header that used to end the process. + print("split flags: " + probe(good + "0€")) + print("still up: " + probe("")) + + # the same hazard in the other fields, each the right byte length. + print("split trace id: " + probe("00-" + TRACE_ID.substring(0, 29) + "€-" + SPAN_ID + "-01")) + print("split span id: " + probe("00-" + TRACE_ID + "-" + SPAN_ID.substring(0, 13) + "€-01")) + print("split version: " + probe("0€-" + TRACE_ID + "-" + SPAN_ID + "-01")) + print("still up: " + probe("")) + + # a burst of them, in case the damage were cumulative rather than immediate. + mut i := 0 + while i < 16: + probe(good + "€") + probe(good + "€0") + probe("€-" + TRACE_ID + "-" + SPAN_ID + "-01") + i = i + 1 + print("still up: " + probe("")) + + # rejecting everything would pass all of the above and silently break + # distributed tracing, so a valid header must still be joined. + print("sampled: " + probe(good + "01")) + print("unsampled: " + probe(good + "00")) + + # an upstream that emits uppercase is out of spec but joinable, and the id + # the handler runs under is the canonical lowercase one — "joined" here is + # the server comparing equal to the lowercase id it never received. + upper := "00-0AF7651916CD43DD8448EB211C80319C-B7AD6B7169203331-01" + print("uppercase: " + probe(upper)) + + return 0 diff --git a/tests/expected/test_web_hostile_traceparent.txt b/tests/expected/test_web_hostile_traceparent.txt new file mode 100644 index 00000000..5f66410b --- /dev/null +++ b/tests/expected/test_web_hostile_traceparent.txt @@ -0,0 +1,11 @@ +ready: 200 fresh +split flags: 200 fresh +still up: 200 fresh +split trace id: 200 fresh +split span id: 200 fresh +split version: 200 fresh +still up: 200 fresh +still up: 200 fresh +sampled: 200 joined +unsampled: 200 joined +uppercase: 200 joined