1642792 - (CVE-2020-6514) WebRTC data channel leaks internal address to peer

Status: RESOLVED FIXED

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

Description

[Taylor Brandstetter]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/83.0.4103.61 Safari/537.36

Steps to reproduce:

This was originally reported for Chrome here: https://bugs.chromium.org/p/chromium/issues/detail?id=1076703

But I looked at Firefox's code and found it was doing the same thing.

Quoted from the original reporter:

"When usrsctp is used with a custom transport, an address must be provided to usrsctp_conninput be used as the source and destination address of the incoming packet. WebRTC uses the address of the SctpTransport instance for this value. Unfortunately, this value is often transmitted to the peer, for example to validate signing of the cookie. This could allow an attacker access to the location in memory of the SctpTransport of a peer, bypassing ASLR.

To reproduce, place the following code on line 9529 of sctp_output.c. This will output the peer's address to the log:

    struct sctp_state_cookie cookie2;
    struct sctp_state_cookie* cookie3;
cookie3 = sctp_get_next_param(cookie, 4, &cookie2, sizeof(struct sctp_state_cookie));


LOGE("COOKIE INITACK ADDRESS %llx laddress %llx", *((long long*)cookie3->address), *((long long*)cookie3->address));

Or, view the SCTP packets sent by WebRTC before they are sent to the encryption layer. They are full of pointers.

This bug is subject to a 90 day disclosure deadline. After 90 days elapse,
the bug report will become visible to the public. The scheduled disclosure
date is 2020-Jul-28. Disclosure at an earlier date is possible if
agreed upon by all parties."

Later, they found a method to set this pointer instead of just retrieving it, making this a pretty severe issue.

The reason this pointer is being put in the sctp address structure is so that the sctp socket can be associated with the relevant object in the send threshold callback.

I added a method upstream in usrsctp that removes this necessity: https://github.com/sctplab/usrsctp/commit/df49eb6e19b84905d0b20dd79edf81ec7a76f133

So using this, there should be no reason to use a pointer as an address.

I would provide a link to the code, but DXR is currently down. Just look for usrsctp_conninput though. Or ask me later.

Comment 1

[Daniel Veditz [:dveditz]]

Byron: can you work with the Google folks to get access to their bug and patch. Or just apply the patch, I guess.

Comment 2

[Byron Campen [:bwc]]

Has anyone talked to Michael Tüxen about this? Should we cc him here?

Comment 3

[Taylor Brandstetter]

I haven't talked to him yet; I thought the addition of usrsctp_get_ulpinfo would be enough to avoid this problem, but recently realized that the conn_output callback doesn't have access to the socket handle or ulpinfo, only the socket address.

Here's an ugly workaround for chrome: https://webrtc-review.googlesource.com/c/src/+/176422

But I think this does need to be fixed upstream. Feel free to include Michael on this thread, I'll try to reach out to him myself, though I don't know if I have his current email address. And I wanted to avoid filing a bug in usrsctp to limit the visibility of this vulnerability until its public.

Comment 4

[Byron Campen [:bwc]]

^

Comment 5

[Michael Tüxen]

Please note, as indicated to Taylor in an e-mail earlier today, that the value of sconn_addr ist conceptually something like an address for the local and remote side (like sin_addr for IPv4 and sin6_addr for IPv6) and not intended to hold values you consider private. Therefore, if you use it to store a pointer and you don't want to share it, you might want to encrypt the value of the pointer before providing it to the usrsctp stack and decrypt it if you get it from the stack. When not using AF_CONN, but AF_INET or AF_INET6 you provide local or remote address information to be used on the wire.

However, you can provide whatever you want and are able to cast to a void pointer.

Comment 6

[Michael Tüxen]

Attachment: clear-afconn-addresses-in-cookie.patch

Comment 7

[Michael Tüxen]

Since Taylor also contacted me via e-mail, here is what I just responded to him:

OK, after thinking about this, we can leave the API as is and for AF_CONN I zero out the
addresses after computing the HMAC, but before providing the COOKIE to the lower layer when
sending. On the receive path, I reconstruct them from the addresses provided by the lower
layer in the COOKIE before doing the HMAC verification.

* This allows existing code to be kept running unchanged.
* Does not put the pointer on the wire.
* Keeps the HMAC checks.

The only limitation is that with this solution we can not handle certain multihoming scenarios,
but this is not supported for AFCONN right now anyway.

I'm attaching a patch I have tested. Please let me know if this would resolve the issue
for you.

In this case I added the patch to this bug report.

Just in case this patch is fine for Taylor and Natalie, please let me know if it would be OK for you if I commit the patch to the usrsctp repository or not (assuming it is fine with Taylor, which I don't know). I just need to know how to proceed.

Comment 8

[Byron Campen [:bwc]]

(In reply to Taylor Brandstetter from comment #0)

I would provide a link to the code, but DXR is currently down. Just look for usrsctp_conninput though. Or ask me later.

Just a heads-up, dxr has been replaced with searchfox. (https://searchfox.org/)

Comment 9

[Byron Campen [:bwc]]

Let me run that patch through our testing.

Comment 10

[Byron Campen [:bwc]]

Local testing seems ok.

Comment 11

[Michael Tüxen]

Thanks for testing and reporting back. Now we need to figure out how to proceed...

Comment 12

[Byron Campen [:bwc]]

FWIW, I am currently working on a patch that will stop using "real" pointers in our DataChannel code, as well as some other code cleanup, so we have two avenues to fix this.

Comment 13

[Michael Tüxen]

Are there any other places you put pointers on the wire?

Comment 14

[Michael Tüxen]

I'm planning to commit my patch, giving Natalie Silvanovich of Google Project Zero credit. Any objections?

Comment 15

[Byron Campen [:bwc]]

(In reply to Michael Tüxen from comment #14)

I'm planning to commit my patch, giving Natalie Silvanovich of Google Project Zero credit. Any objections?

That may paint a bullseye on this problem. Dan?

Comment 16

[Michael Tüxen]

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

(In reply to Michael Tüxen from comment #14)

I'm planning to commit my patch, giving Natalie Silvanovich of Google Project Zero credit. Any objections?

That may paint a bullseye on this problem. Dan?
That is why I'm bringing this up here. She found it, so she needs to be credited. I can delay the commit,
if you want... There will also be another commit, which improve the initialisation of the random
number generator.

Comment 17

[Byron Campen [:bwc]]

Bug 1645219 will work around this issue.

Comment 18

[Daniel Veditz [:dveditz]]

I'm planning to commit my patch, giving Natalie Silvanovich of Google Project Zero credit. Any objections?

That may paint a bullseye on this problem. Dan?

That is why I'm bringing this up here. She found it, so she needs to be credited.

Do you normally credit bug finders in the commit message of the fix? Assuming we can get this fix into Firefox 78 (release June 30, RC probably built the 22nd or 23rd) it's fine either way.

Byron: I assume we need to fix this on the ESR-68 branch as well?

Comment 19

[Michael Tüxen]

(In reply to Daniel Veditz [:dveditz] from comment #18)

I'm planning to commit my patch, giving Natalie Silvanovich of Google Project Zero credit. Any objections?

That may paint a bullseye on this problem. Dan?

That is why I'm bringing this up here. She found it, so she needs to be credited.

Do you normally credit bug finders in the commit message of the fix? Assuming we can get this fix into Firefox 78 (release June 30, RC probably built the 22nd or 23rd) it's fine either way.

Yes, I credit bug reporters, providers of fixes or any other help. Here is what I did yesterday:
https://github.com/sctplab/usrsctp/commit/ed764fee122049601bf25c5370043dff93abf689

Byron: I assume we need to fix this on the ESR-68 branch as well?

Comment 20

[Byron Campen [:bwc]]

(In reply to Daniel Veditz [:dveditz] from comment #18)

I'm planning to commit my patch, giving Natalie Silvanovich of Google Project Zero credit. Any objections?

That may paint a bullseye on this problem. Dan?

That is why I'm bringing this up here. She found it, so she needs to be credited.

Do you normally credit bug finders in the commit message of the fix? Assuming we can get this fix into Firefox 78 (release June 30, RC probably built the 22nd or 23rd) it's fine either way.

Byron: I assume we need to fix this on the ESR-68 branch as well?

That would be best, yes.

Comment 21

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

The severity field is not set for this bug.
:bwc, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 22

[Julien Cristau [:jcristau]]

Is this ready for sec-approval and landing?

Comment 23

[Julien Cristau [:jcristau]]

(In reply to Julien Cristau [:jcristau] from comment #22)

Is this ready for sec-approval and landing?

Comment 24

[Julien Cristau [:jcristau]]

Let's get this in 79, which ships right on July 28.

Comment 25

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Taylor Brandstetter from comment #3)

Here's an ugly workaround for chrome: https://webrtc-review.googlesource.com/c/src/+/176422

Fyi, adding Lennart, who just alerted me the above workaround is in the clear 😕

Comment 26

[Byron Campen [:bwc]]

So bug 1645219 works around this, and has landed on 79. Bug 1645219 is a bit risky for uplift to 78 though. Has this change landed in libusrsctp? If so, we can go ahead and update our import.

Comment 27

[Lennart Grahl [:lgrahl]]

The question is whether you potentially need to downgrade usrsctp due to bug 1625449, or, in the future, for any other reason. It's a bit of a risk factor. Because of that, it might be a good idea to keep the workaround indefinitely and uplift it.

Comment 28

[Byron Campen [:bwc]]

I don't think we have any intention of downgrading usrsctp, and we'll be keeping the workaround because it has other desirable properties. Uplifting the workaround is a bit risky, but maybe less risky than uplifting an updated import?

Comment 29

[Daniel Veditz [:dveditz]]

I'm confused: are we talking about uplifting the workaround to upstream libusrsctp (probably a good idea) or to Firefox 78 (not happening -- release candidates are in final testing)? Oh, or did you mean uplifting to the ESR-68 branch? uplifting a targeted fix to ESR-68.11 sounds less risky than updating an entire library.

TBH I'm not sure what you're asking me.

Comment 30

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Now that bug 1645219 is uplifted everywhere, where do we stand with this bug?

Comment 31

[Byron Campen [:bwc]]

The release of 79 and the disclosure date for this bug are the same day. I imagine that the people who upgrade will not necessarily do so same day, but maybe that's ok.

Comment 32

[Byron Campen [:bwc]]

So this should be addressed now, and we're handling an update of usrsctp over in bug 1646715, which includes the usrsctp patch attached here.

Comment 33

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

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 34

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Comment 35

[Frederik Braun [:freddy]]

Taylor, I'm currently writing our advisory for our release on Tuesday and do not have access to the original bug report.
Can you tell me whether you have a CVE id assigned for the issue? If not, we can assign one instead.

Comment 36

[Daniel Veditz [:dveditz]]

Yes, there's a Google CVE: CVE-2020-6514
Please be sure to add a feed: false to the advisory .yaml so we don't end up claiming to MITRE that we're the publisher.

I'm not sure it applies to the workaround we're shipping in Fx79, but definitely applies when we incorporate their patch in bug 1646715 in fx80. I think it's OK, though, because the underlying bug we're ameliorating is identical.

Comment 37

[Frederik Braun [:freddy]]

bwc, the fifth comment in the project-zero bugtracker says there are two patches. Did we take both?

Comment 38

[Byron Campen [:bwc]]

That second patch was in Chromium code, and we do not need to do anything similar.