ICE does not resolve for `turns` relay candidates rooted in LetsEncrypt CA

• Component: WebRTC
• Status: RESOLVED FIXED

[Arne Gleditsch]

Created attachment 414836 [details]
wireshark screenshot

macOS 11.0.1, Safari Tech Preview 116

Safari is not able to resolve `turns` (TLS) ICE candidates when the TURN servers indicated present TLS certificates that are signed by e.g the Let's Encrypt CA. This seems to be because Safari relies on the upstream webrtc library's hardcoded, built-in SSL roots list. Apparently, one is supposed to override this TLS certification mechanism when integrating the webrtc library. The hard-coded list only contains CA roots required to connect to Google services. Any TURN servers relying on TLS CA vendors not on the hard-coded list will not work for `turns` relay under Safari.

To test, open two tabs towards https://whereby.com/turn-tls-test?turn=onlytls under Safari and observe that black video frames result. Firefox and Chrome (presumably) uses their own built-in TLS verification mechanism for `turns`, and are able to resolve ICE candidates for the same test. See appended Wireshark screenshot showing the Safari client aborting the TLS handshake with "Alert (Level: Fatal, Description: Unknown CA)"

There is extensive background discussion here https://groups.google.com/g/discuss-webrtc/c/4MmARU0XYqc/m/QppVNJiEAAAJ, and there's also a WebRTC tracking bug here https://bugs.chromium.org/p/webrtc/issues/detail?id=11710.

Thanks,
Arne.

[youenn fablet]

Created attachment 415015 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/71843196>

[Alex Christensen]

Comment on attachment 415015 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415015&action=review

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:66
> +    if (size < 4)
> +        return std::make_pair(0, 0);

Should this be nullopt because it doesn't have enough bytes to make a size?

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:146
> +            dispatch_data_apply(content, makeBlockPtr([&](dispatch_data_t, size_t, const void* buffer, size_t size) {
> +                processData(static_cast<const uint8_t*>(buffer), size);

Since we're putting all the data into a Vector anyways, I think it would make the code easier to reason about and less error prone if we just did that here.  Then we could also use dispatch_data_get_size and reserve the size up front and use an unchecked append function.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:220
> +void NetworkRTCSocketSocketCocoa::setOption(int, int)

Should this have FIXME: implement. ?

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:246
> +        std::memset(buffer.data() + size, 0, messageLengths.second - messageLengths.first);

Can the first be greater than the second?
I think a struct with named members would make this easier to understand than a std::pair

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:262
> +    if (size > 65536)

Should this be >= ?
Also, std::numeric_limits<uint16_t> might be nice.

[youenn fablet]

Created attachment 415621 [details]
Patch

[youenn fablet]

Thanks for the review, I updated accordingly.

(In reply to Alex Christensen from comment #3)
> Comment on attachment 415015 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415015&action=review
> 
> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:66
> > +    if (size < 4)
> > +        return std::make_pair(0, 0);
> 
> Should this be nullopt because it doesn't have enough bytes to make a size?
> 
> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:146
> > +            dispatch_data_apply(content, makeBlockPtr([&](dispatch_data_t, size_t, const void* buffer, size_t size) {
> > +                processData(static_cast<const uint8_t*>(buffer), size);
> 
> Since we're putting all the data into a Vector anyways, I think it would
> make the code easier to reason about and less error prone if we just did
> that here.  Then we could also use dispatch_data_get_size and reserve the
> size up front and use an unchecked append function.
> 
> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:220
> > +void NetworkRTCSocketSocketCocoa::setOption(int, int)
> 
> Should this have FIXME: implement. ?
> 
> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:246
> > +        std::memset(buffer.data() + size, 0, messageLengths.second - messageLengths.first);
> 
> Can the first be greater than the second?
> I think a struct with named members would make this easier to understand
> than a std::pair
> 
> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:262
> > +    if (size > 65536)
> 
> Should this be >= ?
> Also, std::numeric_limits<uint16_t> might be nice.

[Peng Liu]

Comment on attachment 415621 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415621&action=review

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.h:39
> +class NetworkRTCSocketSocketCocoa final : public NetworkRTCProvider::Socket {

This class name is interesting.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:60
> +    return !(messageType & 0xC000);

Nit. Can we use a macro here? Or probably add a comment to explain the constant 0xC000?

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:71
> +        return { };

Can we return WTF::nullopt here?

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:144
> +static inline void processIncomingData(RetainPtr<nw_connection_t>&& nwConnection, Function<Vector<uint8_t>(Vector<uint8_t>&&)>&& processData,     Vector<uint8_t>&& buffer = { })

Nit. Some extra spaces can be removed.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:251
> +    if (size >= std::numeric_limits<uint16_t>::max())

Maybe move this check up?

[youenn fablet]

Comment on attachment 415621 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415621&action=review

>> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:60
>> +    return !(messageType & 0xC000);
> 
> Nit. Can we use a macro here? Or probably add a comment to explain the constant 0xC000?

I tend to prefer function over macro.
I am not sure 0xC000 deserves a macro since it is the only place we are using it and it is encapsulated in a one line function.

>> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:71
>> +        return { };
> 
> Can we return WTF::nullopt here?

We could, I prefer { } as style though.

>> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:144
>> +static inline void processIncomingData(RetainPtr<nw_connection_t>&& nwConnection, Function<Vector<uint8_t>(Vector<uint8_t>&&)>&& processData,     Vector<uint8_t>&& buffer = { })
> 
> Nit. Some extra spaces can be removed.

Will fix it.

>> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:251
>> +    if (size >= std::numeric_limits<uint16_t>::max())
> 
> Maybe move this check up?

We could but this check is mostly important since we encode the size as two bytes below, while we are not doing so in the STUN case.

[Eric Carlson]

Comment on attachment 415621 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415621&action=review

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:181
> +                if (auto socket = NetworkRTCSocketSocketCocoa::createClientTCPSocket(identifier, *this, remoteAddress, options, m_ipcConnection.copyRef())) {

s/SocketSocket/Socket/

[Alex Christensen]

Comment on attachment 415621 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415621&action=review

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.h:44
> +    NetworkRTCSocketSocketCocoa(WebCore::LibWebRTCSocketIdentifier, NetworkRTCProvider&, const rtc::SocketAddress&, int options, Ref<IPC::Connection>&&);

It would be nice if we used OptionSet instead of int.

>>> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:60
>>> +    return !(messageType & 0xC000);
>> 
>> Nit. Can we use a macro here? Or probably add a comment to explain the constant 0xC000?
> 
> I tend to prefer function over macro.
> I am not sure 0xC000 deserves a macro since it is the only place we are using it and it is encapsulated in a one line function.

It could at least use a name, maybe a link to a spec that says what it means.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:183
> +    nw_connection_set_state_changed_handler(m_nwConnection.get(), makeBlockPtr([identifier = m_identifier, &rtcProvider, connection = m_connection.copyRef()](nw_connection_state_t state, _Nullable nw_error_t error) {

Should we check error?

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:246
> +        for (size_t cptr = 0 ; cptr < messageLengths->messageLengthWithPadding - messageLengths->messageLength; ++cptr)

Let's use size here to be consistent and in case something goes wrong and it's not equal to messageLength.
Let's also check that size is less than or equal to messageLengthWithPadding.  It's definitely worth a check.

[youenn fablet]

Created attachment 415862 [details]
Patch

[youenn fablet]

> >>> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:60
> >>> +    return !(messageType & 0xC000);
> >> 
> >> Nit. Can we use a macro here? Or probably add a comment to explain the constant 0xC000?
> > 
> > I tend to prefer function over macro.
> > I am not sure 0xC000 deserves a macro since it is the only place we are using it and it is encapsulated in a one line function.
> 
> It could at least use a name, maybe a link to a spec that says what it means.

I added pointers to spec.

> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:183
> > +    nw_connection_set_state_changed_handler(m_nwConnection.get(), makeBlockPtr([identifier = m_identifier, &rtcProvider, connection = m_connection.copyRef()](nw_connection_state_t state, _Nullable nw_error_t error) {
> 
> Should we check error?

Probably, I added an assert for now.
I would expect the state to transition to failed which is handled.

> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:246
> > +        for (size_t cptr = 0 ; cptr < messageLengths->messageLengthWithPadding - messageLengths->messageLength; ++cptr)
> 
> Let's use size here to be consistent and in case something goes wrong and
> it's not equal to messageLength.
> Let's also check that size is less than or equal to
> messageLengthWithPadding.  It's definitely worth a check.

I added a release check, not sure this is worth though.

[youenn fablet]

Created attachment 415863 [details]
Patch

[youenn fablet]

Created attachment 415866 [details]
Patch

[EWS]

Committed r270626: <https://trac.webkit.org/changeset/270626>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415866 [details].

[Shu Muto]

This bug seems to remain in iOS 14.6. WebRTC works with STUN, but still not with TURN.

Confirmed version is following:
Mozilla/5.0 (iPhone; CPU iPhone OS 14_6 like Mac OS X) AppleWebKit/605.1.15 (KHTML, Like Gecko) Version/14.1.1 Mobile/15E148 Safari/604.1

[youenn fablet]

(In reply to Shu Muto from comment #15)
> This bug seems to remain in iOS 14.6. WebRTC works with STUN, but still not
> with TURN.
> 
> Confirmed version is following:
> Mozilla/5.0 (iPhone; CPU iPhone OS 14_6 like Mac OS X) AppleWebKit/605.1.15
> (KHTML, Like Gecko) Version/14.1.1 Mobile/15E148 Safari/604.1

Can you provide a repro case?
https://whereby.com/turn-tls-test?turn=onlytls is working for me.

[Shu Muto]

(In reply to youenn fablet from comment #16)
> (In reply to Shu Muto from comment #15)
> > This bug seems to remain in iOS 14.6. WebRTC works with STUN, but still not
> > with TURN.
> > 
> > Confirmed version is following:
> > Mozilla/5.0 (iPhone; CPU iPhone OS 14_6 like Mac OS X) AppleWebKit/605.1.15
> > (KHTML, Like Gecko) Version/14.1.1 Mobile/15E148 Safari/604.1
> 
> Can you provide a repro case?
> https://whereby.com/turn-tls-test?turn=onlytls is working for me.

I found the another bug in safari before confirming this issue. I'll confirm this issue later. Thank you for telling me the test site!