1922559 - TCP TURN and TCP Media via HTTP proxy is no longer working

Status: RESOLVED FIXED

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

Description

[Alan Ford]

Scenario:

• Configure a HTTP proxy
• Make a WebRTC connection with a configured TCP TURN server

What should happen:

• A HTTP CONNECT message is used to set up a channel to the TCP TURN server
• Then STUN/TURN is carried on this channel

What actually happens:

• A HTTP CONNECT message is used to set up a channel to the TCP TURN server
• A HTTPS connection is attempted over this channel (Client Hello + ALPN for http+h2 is seen)

Having tested various Firefox releases, I have confirmed this regression occurred between 128 and 129.

Comment 1

[Andreas Pehrson [:pehrsons]]

Thanks for filing, Alan. Would you mind testing with mozregression to nail the regressor down further? Let me know if you encounter any issues.

Comment 2

[Alan Ford]

Thanks for the pointer. I've identified the offending commit. mozregression says window: bcaf8d22, e4b41a9b.

Which leads to finding the fix for bug https://bugzilla.mozilla.org/show_bug.cgi?id=1885594 (https://phabricator.services.mozilla.com/D212429).

This issue doesn't just impact TURN, it actually impacts all media-over-HTTP-proxy. It affects both a TCP TURN server, and TCP ICE candidates. Before this commit, the raw STUN/TURN was sent after CONNECT. After this commit, it's all attempts at setting up HTTPS!

Comment 3

[Andreas Pehrson [:pehrsons]]

Thanks! Kershaw, could you take a look?

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1896625

Comment 5

[Kershaw Chang [:kershaw]]

Hi Alan,

Could you open about:config and set this pref media.webrtc.tls_tunnel_for_all_proxy to false and see if it works?

Thanks.

Comment 6

[Kershaw Chang [:kershaw]]

(In reply to Andreas Pehrson [:pehrsons] from comment #3)

Thanks! Kershaw, could you take a look?

Following the changes made in bug 1896625, we've decided always setting upTLS tunnel when using an HTTP proxy. However, it seems that this is causing some issues with certain use cases.

Byron, could you suggest what necko should do here? Is there a way to differentiate when a TLS tunnel should be set up and when it shouldn't?

Thanks.

Comment 7

[Alan Ford]

Could you open about:config and set this pref media.webrtc.tls_tunnel_for_all_proxy to false and see if it works?

Sorry to say that doesn't seem to make any difference. Still seeing TLS+HTTP attempts on the proxy with this set to false.

Comment 8

[Kershaw Chang [:kershaw]]

(In reply to Alan Ford from comment #7)

Could you open about:config and set this pref media.webrtc.tls_tunnel_for_all_proxy to false and see if it works?

Sorry to say that doesn't seem to make any difference. Still seeing TLS+HTTP attempts on the proxy with this set to false.

Thanks for testing. Could you try to record a http log with that preference (media.webrtc.tls_tunnel_for_all_proxy) off?
In about:logging, please select logging to file and send the log to [e-mail].

Thanks!

Comment 9

[Alan Ford]

(In reply to Kershaw Chang [:kershaw] from comment #8)

Thanks for testing. Could you try to record a http log with that preference (media.webrtc.tls_tunnel_for_all_proxy) off?
In about:logging, please select logging to file and send the log to [e-mail].

Done!

Comment 10

[Byron Campen [:bwc]]

(In reply to Kershaw Chang [:kershaw] from comment #6)

(In reply to Andreas Pehrson [:pehrsons] from comment #3)

Thanks! Kershaw, could you take a look?

Following the changes made in bug 1896625, we've decided always setting upTLS tunnel when using an HTTP proxy. However, it seems that this is causing some issues with certain use cases.

Byron, could you suggest what necko should do here? Is there a way to differentiate when a TLS tunnel should be set up and when it shouldn't?

Thanks.

We need to be looking at the scheme for the turn url; turn: should not use TLS, turns: should. We end up translating that here:

https://searchfox.org/mozilla-central/source/dom/media/webrtc/transport/ipc/WebrtcTCPSocket.cpp#169

I would expect to see us using "http:" here in the TURN case, but maybe we're doing TLS anyway because of https only mode?

Comment 11

[Kershaw Chang [:kershaw]]

We need to be looking at the scheme for the turn url; turn: should not use TLS, turns: should. We end up translating that here:

https://searchfox.org/mozilla-central/source/dom/media/webrtc/transport/ipc/WebrtcTCPSocket.cpp#169

I would expect to see us using "http:" here in the TURN case, but maybe we're doing TLS anyway because of https only mode?

Yeah, that's probably the reason, since I can see the HTTP upgrade happened in the log.

Alan, could you see if the pref dom.security.https_first or dom.security.https_only_mode is true? If yes, could you set it to false and try again?
Thanks.

Comment 12

[Alan Ford]

(In reply to Kershaw Chang [:kershaw] from comment #11)

Alan, could you see if the pref dom.security.https_first or dom.security.https_only_mode is true? If yes, could you set it to false and try again?

Sorry for the delay in replying. Both these are false.

Comment 13

[Kershaw Chang [:kershaw]]

(In reply to Alan Ford from comment #12)

(In reply to Kershaw Chang [:kershaw] from comment #11)

Alan, could you see if the pref dom.security.https_first or dom.security.https_only_mode is true? If yes, could you set it to false and try again?

Sorry for the delay in replying. Both these are false.

Thanks for testing.
Unfortunately, it's not clear to me what's the reason of HTTPS upgrade.
Could you try to download a build from this try push and capture the http log again?

Please see this comment about how to download the build.

Comment 14

[Alan Ford]

(In reply to Kershaw Chang [:kershaw] from comment #13)

Unfortunately, it's not clear to me what's the reason of HTTPS upgrade.
Could you try to download a build from this try push and capture the http log again?

Please see this comment about how to download the build.

I have run this test and emailed you logs direct.

Comment 15

[Kershaw Chang [:kershaw]]

Thanks for testing this again.
The reason the TURN request was upgraded is due to the CSP header (Content-Security-Policy: upgrade-insecure-requests).
Before bug 1896625, this wasn’t an issue because we never attempted to set up a TLS channel for connectOnly requests (even for HTTPS). After bug 1896625, since we now always set up a TLS channel for HTTPS requests, we encountered this bug.

Since the root cause of this issue is the HTTPS upgrade of the TURN request, I’m wondering if it makes sense to disallow HTTPS upgrades for TURN requests.

Byron, what do you think? Are there any security concerns with this approach?

Comment 16

[Byron Campen [:bwc]]

For TURN, we're going to ask for exactly what we want; it isn't that big of a deal if the packet flow to a TURN/STUN server is unencrypted, because the important stuff is encrypted end-to-end.

Comment 17

[Greg Tatum [:gregtatum]]

:bwc We're triaging the regressions, would you mind setting a severity and priority on this so we can understand the user impact?

Comment 18

[Byron Campen [:bwc]]

Probably pretty rare in practice.

Comment 19

[Alan Ford]

(In reply to Byron Campen [:bwc] from comment #18)

Probably pretty rare in practice.

Apologies, maybe I'm missing a nuance here, but in our deployments here this means nobody can use Firefox and TCP Media with WebRTC if they have a HTTP Proxy configured. This seems a fairly serious regression? We've had to recommend customers moving to Chrome as a workaround.

Comment 20

[Byron Campen [:bwc]]

Is the fix something that needs to be done on the necko side of things? Or is there some API for turning https upgrade off on a particular channel?

Comment 21

[Kershaw Chang [:kershaw]]

(In reply to Byron Campen [:bwc] from comment #20)

Is the fix something that needs to be done on the necko side of things? Or is there some API for turning https upgrade off on a particular channel?

Yeah, this needs some change on necko side.
I'll submit a patch soon.

Comment 22

[Kershaw Chang [:kershaw]]

Attachment: Bug 1922559 - Make it possible to skip HTTPS upgrade for TURN requests, r=bwc

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9985db52a35
Make it possible to skip HTTPS upgrade for TURN requests, r=bwc,necko-reviewers

Comment 24

[Alan Ford]

Hi all, I pulled the debug build from https://treeherder.mozilla.org/jobs?repo=autoland&revision=b9985db52a3577c35467ba72765bd9cd919d6802&selectedTaskRun=PqLxIt5BQnmDoF3_QvQf_w.0 and can confirm this has now fixed the problem for me! Thanks Kershaw!

Comment 25

[Cristina Horotan [:chorotan]]

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

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:bwc, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox132 to wontfix.

For more information, please visit BugBot documentation.

Comment 27

[Byron Campen [:bwc]]

How do we feel about an uplift here? This is pretty low risk, right?

Comment 28

[Byron Campen [:bwc]]

Comment on attachment 9431687 [details]
Bug 1922559 - Make it possible to skip HTTPS upgrade for TURN requests, r=bwc

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Corporate users (or other users behind a web proxy) might not be able to use certain web conferencing services.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This adds a flag that can be used to disable automatic http->https upgrade on a particular channel, and the webrtc TURN code is the only thing that uses it, and even then only when using a web proxy.
• String changes made/needed:
• Is Android affected?: Yes

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9431687 [details]
Bug 1922559 - Make it possible to skip HTTPS upgrade for TURN requests, r=bwc

It's behind a pref, so we should be able to disable pretty easily if any problems come up post-release. Approved for 132.0rc1.

Comment 30

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/d0aa6e459c11

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Is this something we should uplift to ESR128 also?