1677046 - (CVE-2021-23982) Apply existing outgoing port restrictions from Necko to WebRTC code

Status: RESOLVED FIXED

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

Description

[Frederik Braun [:freddy]]

This is related to bug 1676868 (and bug 1674735).

We need to find out how to make TURN servers provided for WebRTC / ICE are also subject to existing port restriction for outbound connections.

CCing Nils: Can you please assign someone to ensure that we can apply these restrictions from Necko's port block list into our WebRTC code (e.g., at https://hg.mozilla.org/mozilla-central/file/abe51ad662bc6efbe8c637c75cd2630788d49446/dom/media/webrtc/jsapi/MediaTransportHandler.cpp#l285)

Comment 1

[Frederik Braun [:freddy]]

The comment at https://bugzilla.mozilla.org/show_bug.cgi?id=1676868#c15 has a noteworthy component about using a slightly different set. This seems necessary, not to break existing functionality.

Comment 2

[Frederik Braun [:freddy]]

Dan, it looks like this has been untriaged for almost 3 weeks. Anything I can do to help this move along?

Comment 3

[Dan Minor [:dminor]]

Sorry about that. I think Byron and Michael are most knowledgeable about this. I'll take an initial guess at setting priority and severity.

Comment 4

[Byron Campen [:bwc]]

We are already doing this for local ports, here: https://searchfox.org/mozilla-central/rev/6bb59b783b193f06d6744c5ccaac69a992e9ee7b/dom/media/webrtc/transport/nr_socket_prsock.cpp#1641-1643

It should be fairly easy to do the same for remote ports too, but I have some questions:

1. How would JS fool our ICE code into emitting something that looks like a SIP packet to an ALG? I guess you could do that with the STUN username + realm + nonce attributes? My understanding of the attack is that these fields would need to both exceed the MTU, and contain the "SIP packet". So I guess if you max out the length of the username (which comes from JS, and is limited to 512 bytes), and then max out both the realm and nonce (which come from the 401 from the TURN server, and are each limited to 763 bytes, and that we validate decodes to at most 128 characters of UTF-8), I guess you could break MTU in our next request, and have the second fragment look enough like a SIP message to fool an ALG into opening a port, all without exceeding our total STUN message size of 2048. It seems plausible, if a bit tricky.

2. Should we do this for UDP as well as TCP? Probably.

3. Should we do this for TLS (STUNS/TURNS)? Probably not, since that would make it impossible to emit something that looks like SIP.

4. Should we do this for plain STUN (as opposed to just TURN)? I think we handle 401 with long-term auth for STUN requests just like we do for TURN, even though it would be unusual to see this in practice, so probably?

Nils, what do you think?

Comment 5

[Samy Kamkar]

Hi Byron!

Just some thoughts on my side here:

1. Please note the SIP overloading the MTU is just one example but there are other ALGs that do not require multiple packets and can be a single line in the middle of a packet with arbitrary data on either end, IRC DCC is an example of this.
2. I'd suggest doing it for UDP for any protocols that specifically support UDP such as DNS (UDP 53) and SIP (UDP 5060/5061).
3. Ben and Greg may have more thoughts. It seems unlikely to be an issue due to the avalanche effect of modern ciphers, but I would suggest blocking the ports if possible regardless as it's a small number of ports and avoids this potentially being revisited, especially if an attacker in the future can control other portions of the cryptographic headers. Another unlikely but potential future attack would be if there's ever a mechanism for the client to recover the AES/symmetric key used or to explicitly set the key, in the case of something like AES-ECB being used, the attacker could align the username portion of the packet to block size boundary, prepare and "decrypt" their ALG attack data (which will really just produce encrypted data), then include that encrypted data at the boundary. The TURNS/STUNS would then "encrypt" the data, in reality reversing the encryption and leaving the decrypted data in the packet while the rest is encrypted and triggering the vulnerability. Again, unlikely, but it's those unlikely things that are the most fun to investigate :)
4. I would highly suggest blocking known ALG ports (for their relevant protocol of TCP or UDP) for all outbound socket mechanisms unless explicitly required, for example allowing TCP 21 for ftp:// URIs, while blocking it everywhere else.

Thanks all!

Comment 6

[Nils Ohlmeier [:drno]]

(In reply to Samy Kamkar from comment #5)

1. Please note the SIP overloading the MTU is just one example but there are other ALGs that do not require multiple packets and can be a single line in the middle of a packet with arbitrary data on either end, IRC DCC is an example of this.

Just to be extra clear here: if I'm not mistaken the attacks can work with any protocol which opens additional network ports, correct?
So we are talking about SIP, H323, PPTP, RTSP, IRC. Blocking the default destination ports for these protocols for TURN makes sense as the TURN username and password allow the JS to set the outgoing content.

2. I'd suggest doing it for UDP for any protocols that specifically support UDP such as DNS (UDP 53) and SIP (UDP 5060/5061).

I don't fully agree here. Yes blocking the SIP ports for TURN makes sense. Blocking DNS does not make much sense to me as I'm not aware of the DNS protocol containing any other port number, which would result in ALGs opening ports on their public side.

3. Ben and Greg may have more thoughts. It seems unlikely to be an issue due to the avalanche effect of modern ciphers, but I would suggest blocking the ports if possible regardless as it's a small number of ports and avoids this potentially being revisited, especially if an attacker in the future can control other portions of the cryptographic headers. Another unlikely but potential future attack would be if there's ever a mechanism for the client to recover the AES/symmetric key used or to explicitly set the key, in the case of something like AES-ECB being used, the attacker could align the username portion of the packet to block size boundary, prepare and "decrypt" their ALG attack data (which will really just produce encrypted data), then include that encrypted data at the boundary. The TURNS/STUNS would then "encrypt" the data, in reality reversing the encryption and leaving the decrypted data in the packet while the rest is encrypted and triggering the vulnerability. Again, unlikely, but it's those unlikely things that are the most fun to investigate :)

I agree with Byron here that blocking encrypted protocols doesn't make that much sense to me. But maybe we should talk to our crypto expert about this.

4. I would highly suggest blocking known ALG ports (for their relevant protocol of TCP or UDP) for all outbound socket mechanisms unless explicitly required, for example allowing TCP 21 for ftp:// URIs, while blocking it everywhere else.

In my opinion that whole approach of blocking ports is very limited on one hand and a little bit dangerous on the other.
At least most of the modern service also can be executed on non-default ports. And if the ALGs try to be/become smart about these as well there isn't much we can do about it.
Similarly if there are ALGs which simply look for pieces of known protocols in any packet they pass along there isn't anything we can do about that either.

The reason why I'm not in favor of blocking every outgoing connection is that I'm not aware of any way for the JS to control the content of outgoing STUN packets (binding requests etc) if they are not TURN. JS can control the destination of these requests via the ICE candidates, but not the content as out implementation chooses username and password.
So if I'm not mistaken long term credentials also only apply to the usage of TURN.

Comment 7

[Anne (:annevk)]

On the HTTP side we blocked the ports for the encrypted variants mainly out of caution as they could be misconfigured and it matches precedent for ports previously added to the list.

Is anyone in contact with the WebRTC folks at Apple and Google to coordinate a final set of ports to block for TCP and UDP? It seems per https://w3c.github.io/webrtc-pc/ (some discussion at https://github.com/w3c/webrtc-pc/issues/2613 too) WebRTC already reuses https://fetch.spec.whatwg.org/#port-blocking which would mean that the decision we arrive at in https://github.com/whatwg/fetch/security/advisories/GHSA-p5q9-g76c-hc6f would impact WebRTC as well, right? (I will copy Byron and Nils to that security issue shortly.)

Comment 8

[Nils Ohlmeier [:drno]]

(In reply to Anne (:annevk) from comment #7)

On the HTTP side we blocked the ports for the encrypted variants mainly out of caution as they could be misconfigured and it matches precedent for ports previously added to the list.

Is anyone in contact with the WebRTC folks at Apple and Google to coordinate a final set of ports to block for TCP and UDP? It seems per https://w3c.github.io/webrtc-pc/ (some discussion at https://github.com/w3c/webrtc-pc/issues/2613 too) WebRTC already reuses https://fetch.spec.whatwg.org/#port-blocking which would mean that the decision we arrive at in https://github.com/whatwg/fetch/security/advisories/GHSA-p5q9-g76c-hc6f would impact WebRTC as well, right? (I will copy Byron and Nils to that security issue shortly.)

I'm not sure what you mean by "WebTRC already reuses https://fetch.spec.whatwg.org/#port-blocking". Is that only from a spec perspective? Because Firefox WebRTC implementation currently does not block these target ports yet. I believe this is what this bug is about.

Comment 9

[Anne (:annevk)]

Yeah, I meant that the specification seems to require that.

Comment 10

[Michael Froman [:mjf]]

I don't have access to Bug 1676868 so I'm missing a bit of context. Can you add me to that bug?

Comment 11

[Anne (:annevk)]

I added you Michael.

Comment 12

[Michael Froman [:mjf]]

(In reply to Anne (:annevk) from comment #11)

I added you Michael.

Thank you!

Comment 13

[Michael Froman [:mjf]]

Attachment: Bug 1677046 - apply outgoing port restrictions in addNrIceServer. r?bwc!

Comment 14

[Michael Froman [:mjf]]

Good try run:
https://treeherder.mozilla.org/jobs?repo=try&revision=fe9b214810056238fbd4ea5502a19f9f25f6bd4f

Comment 15

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

https://hg.mozilla.org/integration/autoland/rev/8471b70b4df960d3599dcd951f0b05fb4f7bd420
https://hg.mozilla.org/mozilla-central/rev/8471b70b4df9

Comment 16

[Julien Cristau [:jcristau]]

Please request uplift to esr78 if you think we should fix this there.

Comment 17

[Michael Froman [:mjf]]

Comment on attachment 9202459 [details]
Bug 1677046 - apply outgoing port restrictions in addNrIceServer. r?bwc!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This improves our ability to block NAT bypass techniques by applying the Necko port block list to outgoing connections.
• User impact if declined: Potential exposure to NAT slipstreaming.
• Fix Landed on Version: 87
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a fairly small patch with very localized changes.
• String or UUID changes made by this patch: n/a

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9202459 [details]
Bug 1677046 - apply outgoing port restrictions in addNrIceServer. r?bwc!

Approved for 78.9esr.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/dbfd477ed098

Comment 20

[Tom Ritter [:tjr]]

Attachment: advisory.txt