What does the Squid request smuggling patch actually fix?

The advisory for CVE-2023-46846 carries a CVSS score of 9.3 and two sentences of technical content. The description:

Due to chunked decoder lenience Squid is vulnerable to Request/Response smuggling attacks when parsing HTTP/1.1 and ICAP messages.

and, under severity:

This problem allows a remote attacker to perform Request/Response smuggling past firewall and frontend security systems when the upstream server interprets the chunked encoding syntax differently from Squid.

That's the lot. The rest of SQUID-2023:1 is version tables and contact addresses. Everything from Squid 2.6 through 6.3 is affected and 6.4 has the fix. Anything older than 5.1 is untested and should be assumed vulnerable, which covers most Squid that has ever run. There is one workaround, ICAPS, and it only covers the ICAP half of the problem. For the HTTP half the advisory says plainly that there is none.

Which is a fair advisory to publish. It's just not one you can learn anything from. The finding is credited to Keran Mu and Jianjun Chen of Tsinghua University and Zhongguancun Laboratory, and the fix to Amos Jeffries of Treehouse Networks. The fix is public, it's a single commit, and it's small enough to read in ten minutes. So read it.

Chunked encoding#

If a server knows how long its response is, it sends Content-Length and the receiver counts bytes. If it doesn't know, because the page is being generated as it goes, HTTP/1.1 gives it Transfer-Encoding: chunked. The body then arrives as a sequence of chunks. Each chunk is a length in hexadecimal on its own line, then that many bytes, then a blank line. A zero-length chunk ends the body.

POST /upload HTTP/1.1
Host: example.com
Transfer-Encoding: chunked

5
hello
0

There's an optional extra. The size line may carry semicolon-separated parameters, called chunk extensions, which almost nobody uses and every parser must still handle. 5;name=value is a legal chunk header.

Request smuggling happens when two HTTP implementations on the same connection disagree about where one message ends and the next begins. A proxy reads a request, decides the body was five bytes long, and treats whatever follows as the start of a second request. The origin server reads the same bytes, decides the body was longer, and swallows what the proxy thought was a new request.

Now the two of them hold different opinions about the boundaries in a stream they are both parsing. An attacker who arranged that gets to write the front of somebody else's request. That's how you smuggle a request past a firewall, or poison a cache entry for a URL you don't control. The whole class is a disagreement bug. Causing one means finding any byte sequence the proxy is happy to interpret and the server behind it interprets differently, or the other way round.

A proxy that's lenient about framing is therefore in an awkward position. Being generous with malformed input is a virtue almost everywhere in HTTP, since the alternative is breaking somebody's twenty-year-old appliance. In the chunked decoder specifically it's a way to end up with a boundary nobody else agrees with.

Reading the commit#

The fix is 6cfa10d9, "RFC 9112: Improve HTTP chunked encoding compliance", committed on 13 October 2023, eight days before the advisory was published. Five files, 39 insertions. Three separate leniencies come out of the decoder.

The first is the one you can see immediately, because it's a brand new rejection:

static const SBuf bannedHexPrefixLower("0x");
static const SBuf bannedHexPrefixUpper("0X");
if (tok.skip(bannedHexPrefixLower) || tok.skip(bannedHexPrefixUpper))
    throw TextException("chunk starts with 0x", Here());

Squid parsed the chunk size with tok.int64(size, 16, false), its own tokenizer's integer routine. Look at what that routine does with base 16:

if (( base == 0 || base == 16) && *s == '0' && (s+1 < end ) &&
        tolower(*(s+1)) == 'x') {
    s += 2;
    base = 16;
}

It eats a C-style 0x prefix. Sensible behaviour for a general-purpose number parser, and wrong for this particular caller, because the chunked grammar says a chunk size is 1*HEXDIG and nothing else. So Squid read 0x10 as a sixteen-byte chunk. A stricter server reading the same bytes sees a chunk size of zero, 0, followed by garbage where it expected a chunk extension. Two answers, same input, and one of them says the body ended right here.

The second is the line terminator. Every chunk boundary in the grammar is CRLF. Squid's chunked parser delegated to a shared helper:

void
Http::One::Parser::skipLineTerminator(Tokenizer &tok) const
{
    if (tok.skip(Http1::CrLf()))
        return;

    if (Config.onoff.relaxed_header_parser && tok.skipOne(CharacterSet::LF))
        return;
    ...

A bare LF, with no CR, is accepted when relaxed_header_parser is on. That setting defaults to on, and its documentation explains why: Squid accepts "certain forms of non-compliant HTTP messages where it is unambiguous what the sending application intended", then normalises them on the way out. For request headers that's defensible and matches what everyone else does. Applied to chunk framing it means Squid and the server behind it can be reading the same stream with different opinions about which bytes are structure and which are payload.

The patch leaves that tolerance where it is. skipLineTerminator still returns happily on a bare LF when relaxed_header_parser is on. All the patch does to it is rewrite the rest of the function in terms of the new strict skip. What changes for chunk framing is that the decoder stops calling it. Both of its call sites in TeChunkedParser.cc become direct, strict skips:

tok.skipRequired("CRLF after [chunk-ext]", Http1::CrLf());
...
tok.skipRequired("chunk CRLF", Http1::CrLf());

The new Tokenizer::skipRequired throws unless the exact sequence is there. It also distinguishes "wrong bytes" from "not enough bytes yet", which is the distinction an incremental parser lives or dies on.

The third is quieter, and it's about that distinction. parseChunkExtensions walked the extension list committing as it went:

parseOneChunkExtension(tok);
buf_ = tok.remaining(); // got one extension

buf_ is the parser's checkpoint. When it runs out of data mid-message it rewinds there and waits for the next packet: the caller catches InsufficientInput and calls tok.reset(buf_). But the caller's tokenizer was handed down by reference through the whole extension walk. A nested parser could advance it over bytes it hadn't finished with before deciding it needed more data. The fix makes each level parse into its own copy. Each level assigns back to the caller's tokenizer only once the piece is consumed:

void
Http::One::TeChunkedParser::parseOneChunkExtension(Tokenizer &callerTok)
{
    auto tok = callerTok;

    ParseBws(tok);

    const auto extName = tok.prefix("chunk-ext-name", CharacterSet::TCHAR);
    callerTok = tok; // in case we determine that this is a valueless chunk-ext
    ...

Which of these three the researchers actually reported, the advisory doesn't say. All three are the same class of thing: a place where Squid's idea of a message boundary is derived from bytes that another implementation reads differently.

The ICAP mention in the advisory matters here too. ICAP is the protocol Squid uses to hand messages to an external content-scanning service. It borrows HTTP's chunked syntax and this same parser, so a hostile or compromised ICAP server gets the same primitive pointed back at the proxy. Hence the only workaround on offer: use ICAP services you trust, over TLS.

One thing worth keeping straight, because it happened at the same time. Joshua Rogers published a Squid audit in October 2023 titled "55 vulnerabilities and 35 0days", and a lot of coverage from that month blurs the two together. CVE-2023-46846 isn't in his list. The consecutive number, CVE-2023-46847, is his, for a buffer overflow in Digest authentication. Two unrelated pieces of work landing on the same maintainers in the same fortnight.