1578230 - Crash [@ mozilla::net::SpdyConnectTransaction::MapStreamToHttpConnection] with HTTP2 Proxy

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P2)

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 88fe9a3d4522+ (build with --enable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --enable-fuzzing --disable-debug).

For detailed crash information, see attachment.

To reproduce the issue, perform the following steps:

1. Download the attached testcase, save as "test.bin".
2. Apply the patch from bug 1578186 to get the NetworkHttp2Proxy fuzzing target.
3. Build with --enable-fuzzing (requires Clang and ASan, also build gtests using ./mach gtest dontruntests).
4. Run MOZ_RUN_GTEST=1 LIBFUZZER=1 FUZZER=NetworkHttp2Proxy objdir/dist/bin/firefox test.bin

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Christian Holler (:decoder)]

The problem here is that mDrivingTransaction is nullptr in SpdyConnectTransaction::MapStreamToHttpConnection. I'm investigating further now.

Comment 4

[Christian Holler (:decoder)]

Attachment: Full log with nsFuzzing:5,nsFuzzingNecko:5,nsHttp:5,nsWebSocket:5,sync

So what is happening here is that MapStreamToHttpConnection is called twice on the same SpdyConnectTransaction:

[(null) 15962: Socket Thread]: I/nsHttp Http2Stream 0x613000050300 Tunnel Response code 100
[...]
[(null) 15962: Socket Thread]: I/nsHttp Http2Stream 0x613000050300 Tunnel Response code 206

and the first call sets mDrivingTransaction to nullptr here:

https://searchfox.org/mozilla-central/rev/9415ecf29ba1acbef9381335e0ecde5916ca4073/netwerk/protocol/http/TunnelUtils.cpp#1203

I don't know how to fix this because I don't know what the expected behavior here is in the first place (e.g. if MapStreamToHttpConnection should even be called twice in the first place).

NI? for Honza to get some input on this.

Comment 5

[Honza Bambas (:mayhemer)]

First though suggests this could be related somehow to 1564447. But I have to reproduce and see what's going on.

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: log from the fuzzing run

for consideration, this is the log I'm getting

the interesting object: SpdyConnectTransaction @613000050140

Comment 7

[Honza Bambas (:mayhemer)]

the thing here is that we receive response headers with HTTP code = 100 on the tunnel (handle it) and then again, another response headers with code 206 is received, again on the same tunnel. is it legal for a single stream to receive (and handle) two responses?

Comment 8

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #7)

the thing here is that we receive response headers with HTTP code = 100 on the tunnel (handle it) and then again, another response headers with code 206 is received, again on the same tunnel. is it legal for a single stream to receive (and handle) two responses?

1XX is only pre-responses - 100 is a "Continue". Anyway we should ignore the 100 response and for sure not crash.

Comment 9

[Christian Holler (:decoder)]

This bug is now 9 months old and still occurring in fuzzing. In order for fuzzing to progress and be effective, it would be good to fix this bug.

Dragana, can you assign someone to take care of this? Thanks!

Comment 10

[Dragana Damjanovic [:dragana]]

Nhi, can you find someone to fix this?

Comment 11

[Junior [inactive]]

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/100
We don't send Expect: 100-continue for the GET request and the CONNECT proxy request.
I believe it's undefined behavior to get the 100 response.

Here's some history.
In our first version of H2, we redact the headers if there's Expect: 100-continue, as we don't actually respond to
https://searchfox.org/mozilla-central/rev/a0d88062092f2489e3a13273988059ec3340f23e/netwerk/protocol/http/Http2Compression.cpp#887-905

Later on, H2 starts to support 1xx response code other than 101. Bug 1081341 handles this.
Fetch/xhr can send custom headers, but Expect is forbidden by looking at the wpt test
https://searchfox.org/mozilla-central/rev/eef39502e08bcd3c40573c65a6827828dce4a032/testing/web-platform/tests/fetch/api/basic/request-forbidden-headers.any.js#29

Moreover, we make the CONNECT string without the 'Expect' header
https://searchfox.org/mozilla-central/rev/eef39502e08bcd3c40573c65a6827828dce4a032/netwerk/protocol/http/nsHttpConnection.cpp#2224

Based on the discussion, I'd like to treat 100 as an illegal value.

Comment 12

[Junior [inactive]]

Anne, I'd like to have a final check to see if we're good to reject 100 response.
Is it possible for client to set Expect header, one of the forbidden request header, as a custom header?

Comment 13

[Anne (:annevk)]

https://tools.ietf.org/html/rfc7231#section-6.2 (potentially updated by https://httpwg.org/http-core/draft-ietf-httpbis-semantics-latest.html#status.1xx but it looks the same) says the following on 1xx responses:

A client MUST be able to parse one or more 1xx responses received prior to a final response, even if the client does not expect one. A user agent MAY ignore unexpected 1xx responses.

So unless I'm missing some subtlety, we should do what Dragana said in comment 8, i.e., ignore the response.

(I'm also not sure I agree with the idea that a 101 over HTTP/2 received by a client should result in an error. https://tools.ietf.org/html/rfc7540 doesn't explicitly state that and it would seem somewhat wrong for it to override the core protocol in this way. In any event, this is about 100 and I think that's unambiguous.)

Comment 14

[Junior [inactive]]

Thanks, Anne. I'll not treat 101 resulting error as a golden rule.

Comment 15

[Junior [inactive]]

FWIW, quote what :decoder said in mail:

The fuzzer has been renamed and is now called "NetworkHttp2ProxyHttp2".
Also, you cannot use ./mach to run this, you need to run it like this:

FUZZER=NetworkHttp2ProxyHttp2 objdir/dist/bin/firefox test.bin

Another note that we need to ./mach gtest dontruntests every time we build.

Thanks, :decoder

Comment 16

[Junior [inactive]]

Attachment: Bug 1578230 - skip 1xx pre-response for H2 CONNECT proxy, r=mayhemer

We already able to allow more headers after 1xx for H2.
https://searchfox.org/mozilla-central/rev/5e6c7717255ca9638b2856c2b2058919aec1d21d/netwerk/protocol/http/Http2Session.cpp#1547-1550

What we need to do is a special handle in H2 Tunneling:
Create the transport shims and decide if we need secondary TLS when we have a final response.

Comment 17

[Junior [inactive]]

Attachment: Bug 1578230 - do not process transactions blocked on tunnel before a final response, r=mayhemer

Depends on D80261

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/352726f9bd29
skip 1xx pre-response for H2 CONNECT proxy, r=mayhemer,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/3a12b1eccd8f
do not process transactions blocked on tunnel before a final response, r=mayhemer,necko-reviewers

Comment 19

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/352726f9bd29
https://hg.mozilla.org/mozilla-central/rev/3a12b1eccd8f