1553927 - DoS attack on http2 proxy when secure connection fails (e.g. certificate mismatch)

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

We keep Http2Session::MaybeReTunnel in a circle DoS'ing the proxy. I need to confirm this happens in an otherwise tuned up environment.

Comment 1

[Honza Bambas (:mayhemer)]

Confirming. When I remove web-id trust from the proxy's CA (or remove exception, if there were any) we shot at the proxy with high cadence. The moment I reestablish the trust, all works as expected. Tested only with my test proxy. I can see we are creating sessions with the proxy - proxy.on('session') is triggered on the proxy, but I don't know the nodejs h2 impl that well to know in what phase it fires this event.

Anyway, easily reproducible.

Comment 2

[Honza Bambas (:mayhemer)]

This is bug in detect portal! Investigating.

Comment 3

[Honza Bambas (:mayhemer)]

The problem is here:
nsHttpChannel.cpp - mozsearch

When the error is a sec error (nss) we immediately retry, creating a nice loop. Introduced in 816866 - Certificate errors frequently caused by captive portals should trigger captive portal detection 3 years ago!

Valentin, you reviewed that patch, do you remember how this has to work? This way it's definitely pretty much broken...

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Oh, that's interesting indeed.
What's supposed to happen is that if we encounter a cert error, as it is common in some captive portals, we immediately recheck if there's a captive portal. However, I never considered what would happen if it failed for a http2 proxy :)

I guess what we might want to do here is to put some rate limiting on RecheckCaptivePortal - if we last checked in the last 3 seconds (pref?) maybe we don't recheck at all. We already have mLastChecked that we can use for this.

What do you think?

Comment 5

[Honza Bambas (:mayhemer)]

Ah! If some website has a cert error, we check if there is a captive portal suddenly. OK. There should definitely be rate limiting inside the service ;) But, seriously - if the channel is a plain request, then any cert error must come from a proxy, right? I think we could just disable this check when the channel's uri schema is "http" or when the channel is a detect-portal check (I wonder if that would ever become https uri?)

Comment 6

[Valentin Gosu [:valentin] (he/him)]

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

I think we could just disable this check when the channel's uri schema is "http" or when the channel is a detect-portal check

I think that's a good fix too. Note: I don't know if we have a good way of detecting if the channel is a detect-portal check

(I wonder if that would ever become https uri?)

Users can technically set the URL to be a https one. I assume it rarely happens, as it kinda defeats the purpose. Some captive portals simply time out or block HTTPS connections, so it would completely fail to detect those networks.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: Bug 1553927 - Do not do immediate captive portal check on certificate/ssl errors when connecting a plain URL to prevent loops, r=valentin!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f75360f234a9
Do not do immediate captive portal check on certificate/ssl errors when connecting a plain URL to prevent loops, r=valentin

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f75360f234a9

Comment 10

[Honza Bambas (:mayhemer)]

Might be good to uplift.