Closed Bug 1553927 Opened 5 months ago Closed 5 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [no-nag][necko-triaged][secure-proxy-mvp])

Attachments

(1 file)

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

Priority: -- → P1
Whiteboard: [no-nag] → [no-nag][necko-triagged]
Whiteboard: [no-nag][necko-triagged] → [no-nag][necko-triaged]

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.

Status: UNCONFIRMED → NEW
Ever confirmed: true

This is bug in detect portal! Investigating.

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...

Flags: needinfo?(valentin.gosu)

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?

Flags: needinfo?(valentin.gosu)

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?)

Flags: needinfo?(valentin.gosu)

(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.

Flags: needinfo?(valentin.gosu)
Status: NEW → ASSIGNED
Pushed by honzab.moz@firemni.cz:
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
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Might be good to uplift.

Blocks: 1545411
Whiteboard: [no-nag][necko-triaged] → [no-nag][necko-triaged][secure-proxy-mvp]
You need to log in before you can comment on or make changes to this bug.