From 6ce983de118ef0a8a92ce2b0bdf38b03e670141c Mon Sep 17 00:00:00 2001 From: Diogo Martins Date: Tue, 13 Oct 2026 00:13:52 +0100 Subject: [PATCH] tls, quic, httpclient: a non-ASCII ALPN protocol name is refused at configuration instead of sent as other bytes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three ALPN wire builders did not send a non-ASCII name as configured. The TLS server's and the TLS client's BuildAlpnWire cast each UTF-16 unit to a byte, so U+0168 went out as 0x68 and the configured "Ũ2" was the real "h2": the connection negotiated, and CaptureAlpn / NegotiatedAlpn reported, a protocol nobody configured. QUIC's AlpnWire encoded with Encoding.ASCII, which substitutes '?', so "Ũ2" became "?2" and a client offering the literal "?2" was admitted and served. Each builder now refuses a name that is not ASCII with an ArgumentException, next to its existing 1..255 length check. Refusing rather than encoding as UTF-8 because every reader of the negotiated name (CaptureAlpn, ReadNegotiatedAlpn, QuicEngineConnection's read-back) decodes ASCII, and with ASCII the existing length check counts bytes exactly. The pending tests "alpn: a non-ascii protocol name must not negotiate as a different protocol" (Tls), "quic/alpn: a non-ascii allow-list token must not admit a protocol nobody configured" (E2E) and "tls client: a non-ascii alpn protocol must not be offered as a different protocol" (Http) are now Tests; each already accepted a refusal at configuration. --- .../ioxide.httpclient/Tls/TlsClientContext.cs | 5 ++++ src/ioxide/Tls/TlsService.cs | 5 ++++ .../ioxide.ngtcp2/Engine/QuicEngine.cs | 5 ++++ .../Ioxide.Tests.E2E/Protocols/H3AlpnTests.cs | 25 +++++++++---------- .../TlsClientPostureTests.cs | 20 ++++----------- .../Ioxide.Tests.Tls/AlpnNegotiationTests.cs | 16 +++++------- 6 files changed, 38 insertions(+), 38 deletions(-) diff --git a/src/clients/ioxide.httpclient/Tls/TlsClientContext.cs b/src/clients/ioxide.httpclient/Tls/TlsClientContext.cs index a11ea3f7..47344b58 100644 --- a/src/clients/ioxide.httpclient/Tls/TlsClientContext.cs +++ b/src/clients/ioxide.httpclient/Tls/TlsClientContext.cs @@ -178,6 +178,11 @@ internal static byte[] BuildAlpnWire(string[] protocols) { throw new ArgumentException($"ALPN protocol '{protocol}' must be 1..255 bytes.", nameof(protocols)); } + // Each char goes out as one byte, so any other name would be offered as a different protocol. + if (!System.Text.Ascii.IsValid(protocol)) + { + throw new ArgumentException($"ALPN protocol '{protocol}' must be ASCII.", nameof(protocols)); + } total += 1 + protocol.Length; } diff --git a/src/ioxide/Tls/TlsService.cs b/src/ioxide/Tls/TlsService.cs index ff8558d3..911647ad 100644 --- a/src/ioxide/Tls/TlsService.cs +++ b/src/ioxide/Tls/TlsService.cs @@ -1428,6 +1428,11 @@ private static byte[] BuildAlpnWire(string[] protocols) { throw new ArgumentException($"ALPN protocol '{protocol}' must be 1..255 bytes.", nameof(protocols)); } + // Each char goes out as one byte, so any other name would be sent as a different protocol. + if (!System.Text.Ascii.IsValid(protocol)) + { + throw new ArgumentException($"ALPN protocol '{protocol}' must be ASCII.", nameof(protocols)); + } total += 1 + protocol.Length; } diff --git a/src/protocols/ioxide.ngtcp2/Engine/QuicEngine.cs b/src/protocols/ioxide.ngtcp2/Engine/QuicEngine.cs index 51596c69..9984d093 100644 --- a/src/protocols/ioxide.ngtcp2/Engine/QuicEngine.cs +++ b/src/protocols/ioxide.ngtcp2/Engine/QuicEngine.cs @@ -362,6 +362,11 @@ private static byte[] AlpnWire(string[]? alpn) { throw new ArgumentException($"invalid ALPN token '{proto}'", nameof(alpn)); } + // Encoding.ASCII sends '?' for anything else, so the token would admit a protocol nobody configured. + if (!System.Text.Ascii.IsValid(proto)) + { + throw new ArgumentException($"ALPN token '{proto}' must be ASCII", nameof(alpn)); + } wire.Add((byte)bytes.Length); wire.AddRange(bytes); } diff --git a/tests/Ioxide.Tests.E2E/Protocols/H3AlpnTests.cs b/tests/Ioxide.Tests.E2E/Protocols/H3AlpnTests.cs index b1e47327..3c21ac36 100644 --- a/tests/Ioxide.Tests.E2E/Protocols/H3AlpnTests.cs +++ b/tests/Ioxide.Tests.E2E/Protocols/H3AlpnTests.cs @@ -169,18 +169,18 @@ public static void Register(Runner runner) }, "the ALPN backstop landed only in the pure-C# stack; Nghttp3Connection never reads " + "NegotiatedProtocol, so a permissive engine's 'echo' connection is answered 200"); - runner.Pending("quic/alpn: a non-ascii allow-list token must not admit a protocol nobody configured", () => + runner.Test("quic/alpn: a non-ascii allow-list token must not admit a protocol nobody configured", () => { - // QuicEngine.AlpnWire encodes each configured token with Encoding.ASCII, whose - // fallback substitutes '?' for anything non-ascii - so ["Ũ2"] goes on the wire as the - // allowlist entry "?2". Two consequences: the configured token itself can never - // negotiate (a client offering the actual bytes of "Ũ2" finds no match), and every - // non-ascii token collapses onto '?', so a client offering the literal "?2" is - // admitted, served, and reads back NegotiatedProtocol == "?2" - a protocol nobody - // configured. The TCP side's BuildAlpnWire has the same shape with a worse symptom - // (UTF-16 units cast to bytes turn "Ũ2" into the real "h2"); the fix on either side - // is to refuse a non-ascii token at construction, like the >255-byte one already is, - // or to encode it faithfully - both make this body pass. + // QuicEngine.AlpnWire encoded each configured token with Encoding.ASCII, whose + // fallback substitutes '?' for anything non-ascii - so ["Ũ2"] went on the wire as the + // allowlist entry "?2". Two consequences: the configured token itself could never + // negotiate (a client offering the actual bytes of "Ũ2" found no match), and every + // non-ascii token collapsed onto '?', so a client offering the literal "?2" was + // admitted, served, and read back NegotiatedProtocol == "?2" - a protocol nobody + // configured. The TCP side's BuildAlpnWire had the same shape with a worse symptom + // (UTF-16 units cast to bytes turn "Ũ2" into the real "h2"). A non-ascii token is now + // refused at construction, like the >255-byte one; encoding it faithfully would also + // make this body pass. (string certPath, string keyPath) = TestCert.Ensure(); QuicEngine engine; try @@ -205,7 +205,6 @@ public static void Register(Runner runner) Assert.True(!client.CompleteHandshake(timeoutMs: 5000), "a client offering '?2' completed the handshake against an allow list of 'Ũ2'"); } - }, "AlpnWire's ASCII '?' substitution puts \"?2\" on the wire for the configured \"Ũ2\", " - + "and a client offering the literal \"?2\" is admitted and served"); + }); } } diff --git a/tests/Ioxide.Tests.Http/TlsClientPostureTests.cs b/tests/Ioxide.Tests.Http/TlsClientPostureTests.cs index 57d842a2..ebfa6c15 100644 --- a/tests/Ioxide.Tests.Http/TlsClientPostureTests.cs +++ b/tests/Ioxide.Tests.Http/TlsClientPostureTests.cs @@ -15,14 +15,6 @@ namespace Ioxide.Tests; /// while one inside it is served - not that setting it was accepted. /// /// -/// Two live defects ride as Pending: -/// -/// - BuildAlpnWire casts each UTF-16 unit to a byte, so a non-ASCII protocol name is OFFERED as -/// a different protocol made of its chars' low bytes - the client twin of the server-side -/// defect in TlsService.BuildAlpnWire, proven there today; -/// - HandshakeTimeoutMs is only checked between handshake flights, so the one peer a handshake -/// timeout exists for - accepted the connect, then went silent - is not bounded by it at all. -/// /// Examined without a failing result, stated so they are not re-litigated: MinimumVersion = 0 is /// accepted (OpenSSL's "no floor"), but this box's system-wide MinProtocol makes the weakened /// floor unobservable, so no test can discriminate it here; a ServerName over 255 bytes makes the @@ -152,13 +144,13 @@ public static void Register(Runner runner) $"should name the verification failure, got: {body}"); }); - runner.Pending("tls client: a non-ascii alpn protocol must not be offered as a different protocol", () => + runner.Test("tls client: a non-ascii alpn protocol must not be offered as a different protocol", () => { // TlsClientContext.BuildAlpnWire writes (byte)protocol[i] - the LOW BYTE of each UTF-16 // unit. U+0168 has low byte 0x68, 'h', so a client configured to offer ONLY - // "Ũ" + "2" puts the exact bytes "h2" on the wire, the origin selects h2, and - // NegotiatedAlpn reports a protocol the caller never configured. The server side has - // the same cast and the same proven defect; this is the client half. + // "Ũ" + "2" put the exact bytes "h2" on the wire, the origin selected h2, and + // NegotiatedAlpn reported a protocol the caller never configured. The server side had + // the same cast; this is the client half. Create now refuses a non-ASCII name. string exotic = "Ũ" + "2"; using TlsTestOrigin origin = TlsTestOrigin.Start("h2"); // speaks ONLY h2 @@ -198,9 +190,7 @@ public static void Register(Runner runner) Assert.True(!body.EndsWith("hello over h2"), "a client configured to offer only 'U+0168 2' completed a conversation the origin " + $"negotiated as h2; body: {body}, origin saw alpn={origin.LastAlpn ?? "none"}"); - }, "BuildAlpnWire casts UTF-16 units to bytes, so the configured \"\\u0168 2\" goes on the " - + "wire as the bytes 'h2' and the client negotiates - and NegotiatedAlpn reports - a " - + "protocol nobody configured"); + }); runner.Pending("tls client: HandshakeTimeoutMs bounds a handshake whose peer accepted and went silent", () => { diff --git a/tests/Ioxide.Tests.Tls/AlpnNegotiationTests.cs b/tests/Ioxide.Tests.Tls/AlpnNegotiationTests.cs index 91361496..108375b7 100644 --- a/tests/Ioxide.Tests.Tls/AlpnNegotiationTests.cs +++ b/tests/Ioxide.Tests.Tls/AlpnNegotiationTests.cs @@ -17,9 +17,6 @@ namespace Ioxide.Tests; /// negotiation: what the client was told, and what a handler would branch on to pick an h2 loop /// over an h1 one. The select loop is additionally pinned by reflection for the malformed offers /// OpenSSL never forwards - that test says why end-to-end cannot deliver them. -/// -/// One live defect rides as Pending: BuildAlpnWire casts each UTF-16 unit to a byte, so a -/// non-ASCII protocol name goes on the wire as a DIFFERENT protocol made of its chars' low bytes. /// internal static class AlpnNegotiationTests { @@ -140,14 +137,15 @@ public static void Register(Runner runner) RegisterSelectLoopBounds(runner); - runner.Pending("alpn: a non-ascii protocol name must not negotiate as a different protocol", () => + runner.Test("alpn: a non-ascii protocol name must not negotiate as a different protocol", () => { // BuildAlpnWire writes (byte)protocol[i] - the LOW BYTE of each UTF-16 unit. U+0168 // has low byte 0x68, which is 'h', so a server configured to speak only "\u0168" + "2" - // puts the exact bytes "h2" on the wire and ACKs any client offering h2. Two distinct - // protocol identifiers compare equal, the client is told h2, and CaptureAlpn records + // put the exact bytes "h2" on the wire and ACKed any client offering h2. Two distinct + // protocol identifiers compared equal, the client was told h2, and CaptureAlpn recorded // h2 - a protocol the operator never listed. (The same cast is why the SAME non-ASCII - // string configured on both sides fails to match: SslApplicationProtocol sends UTF-8.) + // string configured on both sides failed to match: SslApplicationProtocol sends UTF-8.) + // BuildAlpnWire now refuses a non-ASCII name, so the start below is refused. string exotic = "\u0168" + "2"; // two chars whose low bytes are 'h','2' (string cert, string key) = TestCert.Ensure(); @@ -186,9 +184,7 @@ public static void Register(Runner runner) Assert.True(negotiated != "h2", "a server configured to speak only 'U+0168 2' ACKed h2 - the char-to-byte cast in " + "BuildAlpnWire makes two distinct protocol identifiers compare equal"); - }, "BuildAlpnWire casts UTF-16 units to bytes, so the configured \"\\u01682\" goes on the wire " - + "as the bytes 'h2' and the server negotiates - and CaptureAlpn records - a protocol " - + "nobody configured"); + }); } ///