Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DTLS Fingerprints in SDP Offer/Answer are not verified #1708

Closed
gaukas opened this issue Mar 17, 2021 · 9 comments
Closed

DTLS Fingerprints in SDP Offer/Answer are not verified #1708

gaukas opened this issue Mar 17, 2021 · 9 comments

Comments

@gaukas
Copy link
Member

gaukas commented Mar 17, 2021 •
edited
Loading

Your environment.

  • Version: pion/webrtc v3.0.14
  • Browser: N/A
  • Other Information - reproducable with example/data-channels-create & example/data-channels

What did you do?

  • Run both data-channels-create and data-channels from example.
  • Once the SDP offer has been generated, decode it with base64.
  • Randomly edit the DTLS fingerprint value in the SDP offer
  • Copy & paste the base64-encoded SDP offer into the waiting data-channels
  • Copy & paste the SDP answer generated by data-channels into data-channels-create

What did you expect?

The built-in fingerprints verification should throw an error and therefore prevent the data-channel from being established.

What happened?

The data channel was created as usual.

ICE Connection State has changed: checking
ICE Connection State has changed: connected
Data channel 'data'-'824635660400' open. Random messages will now be sent to any connected DataChannels every 5 seconds
Sending 'SBfBWYaFzaFZDiV'
Message from DataChannel 'data': 'EtaKTfGglNgpNNn'
@Sean-Der
Copy link
Member

Hey @gaukas

We do have a test for this TestInvalidFingerprintCausesFailed that is properly catching this. I will confirm this works and update as appropriate thanks for filing this!

@Sean-Der
Copy link
Member

Hey @gaukas what browser are you using to test?

I believe the remote peer isn't the one properly asserting.

thanks

@gaukas
Copy link
Member Author

gaukas commented Mar 17, 2021

I am not using any browser. The offerer is data-channels-create and the answerer is data-channels. Both are go binaries.

@gaukas
Copy link
Member Author

gaukas commented Mar 17, 2021

It may not be that simple because I tried to mess with both fingerprints in offer and answer. The data channel was still created successfully.

@gaukas
Copy link
Member Author

gaukas commented Mar 17, 2021

@Sean-Der but thank you for the fast response! I really appreciate it. Please let me know if there's anything else I could help with.

@Sean-Der
Copy link
Member

Sean-Der commented Mar 17, 2021 •
edited
Loading

Ah I see. So the issue is that we set PeerConnectionState to failed, but we don't actually tear down the connections.

I will fix that and tag a new release. This probably even warrants a CVE! If you are interested, good for resume and helps people update quicker.

@gaukas
Copy link
Member Author

gaukas commented Mar 17, 2021

Thank you @Sean-Der! I'm glad that this issue could be helpful to the project.
Just for your reference, attached is the log from both instances, in which 2 bytes from the fingerprint in the offer (generated by data-channels-create) have been altered.

$ ./data-channels-create
eyJ0eXBlIjoib2ZmZXIiLCJzZHAiOiJ2PTBcclxubz0tIDgzNDg1NzQ2MDI2Njc1NjA4MDggMTYxNjAyMzA1NiBJTiBJUDQgMC4wLjAuMFxyXG5zPS1cclxudD0wIDBcclxuYT1maW5nZXJwcmludDpzaGEtMjU2IDgwOkJCOjI0OkU0OjE4OkIyOkQ4OjAxOjZDOkFCOjdCOjY0OjFGOkYwOkNFOjFFOjUyOjE3OkFEOjkyOkRBOjk2OjdFOjQ5OkY1OkVBOkVFOkUwOjFDOkZEOjhGOkFFXHJcbmE9Z3JvdXA6QlVORExFIDBcclxubT1hcHBsaWNhdGlvbiA5IFVEUC9EVExTL1NDVFAgd2VicnRjLWRhdGFjaGFubmVsXHJcbmM9SU4gSVA0IDAuMC4wLjBcclxuYT1zZXR1cDphY3RwYXNzXHJcbmE9bWlkOjBcclxuYT1zZW5kcmVjdlxyXG5hPXNjdHAtcG9ydDo1MDAwXHJcbmE9aWNlLXVmcmFnOnJqYkFzS2pkZnl1aWtVbENcclxuYT1pY2UtcHdkOllSaFhDU21RUUhLUVp5Rk9rZlRFbUtweUFPVE5talJFXHJcbmE9Y2FuZGlkYXRlOjMwMzE3NjQ2MTkgMSB1ZHAgMjEzMDcwNjQzMSAxOTIuMTY4LjI0LjEzMiA1MTczNiB0eXAgaG9zdFxyXG5hPWNhbmRpZGF0ZTozMDMxNzY0NjE5IDIgdWRwIDIxMzA3MDY0MzEgMTkyLjE2OC4yNC4xMzIgNTE3MzYgdHlwIGhvc3RcclxuYT1jYW5kaWRhdGU6MjMzNzYyMTM5IDEgdWRwIDIxMzA3MDY0MzEgMTcyLjE3LjAuMSA1MTI2NiB0eXAgaG9zdFxyXG5hPWNhbmRpZGF0ZToyMzM3NjIxMzkgMiB1ZHAgMjEzMDcwNjQzMSAxNzIuMTcuMC4xIDUxMjY2IHR5cCBob3N0XHJcbmE9Y2FuZGlkYXRlOjk0MDc2MDk2NyAxIHVkcCAxNjk0NDk4ODE1IDczLjI0My4xLjExIDUzNzAyIHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDM3NzU3XHJcbmE9Y2FuZGlkYXRlOjk0MDc2MDk2NyAyIHVkcCAxNjk0NDk4ODE1IDczLjI0My4xLjExIDUzNzAyIHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDM3NzU3XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNcclxuIn0=
eyJ0eXBlIjoiYW5zd2VyIiwic2RwIjoidj0wXHJcbm89LSA3MzM4NDEwODE4NDE3NTg5Njg2IDE2MTYwMjMwODAgSU4gSVA0IDAuMC4wLjBcclxucz0tXHJcbnQ9MCAwXHJcbmE9ZmluZ2VycHJpbnQ6c2hhLTI1NiAwOToyMzpFODpEQTpGRDpCRDpFNzoyNjpDNjpBMDpGQzo2QjozQTo0RDo2QTo0NTo5Njo4MTpCRDo4QTo4MTpFMDoyNTo4QzpFNToyRjo2NDozMjo1Qjo2NzpDNzo3MlxyXG5hPWdyb3VwOkJVTkRMRSAwXHJcbm09YXBwbGljYXRpb24gOSBVRFAvRFRMUy9TQ1RQIHdlYnJ0Yy1kYXRhY2hhbm5lbFxyXG5jPUlOIElQNCAwLjAuMC4wXHJcbmE9c2V0dXA6YWN0aXZlXHJcbmE9bWlkOjBcclxuYT1zZW5kcmVjdlxyXG5hPXNjdHAtcG9ydDo1MDAwXHJcbmE9aWNlLXVmcmFnOkNyQlhMa0pTU1N0UkNYeE1cclxuYT1pY2UtcHdkOm1HWGdDSHNiVlNiRFZMWmFxUWJZbmRra1ZQSk9OeEdYXHJcbmE9Y2FuZGlkYXRlOjMwMzE3NjQ2MTkgMSB1ZHAgMjEzMDcwNjQzMSAxOTIuMTY4LjI0LjEzMiA1NzcyMiB0eXAgaG9zdFxyXG5hPWNhbmRpZGF0ZTozMDMxNzY0NjE5IDIgdWRwIDIxMzA3MDY0MzEgMTkyLjE2OC4yNC4xMzIgNTc3MjIgdHlwIGhvc3RcclxuYT1jYW5kaWRhdGU6MjMzNzYyMTM5IDEgdWRwIDIxMzA3MDY0MzEgMTcyLjE3LjAuMSAzOTMxOSB0eXAgaG9zdFxyXG5hPWNhbmRpZGF0ZToyMzM3NjIxMzkgMiB1ZHAgMjEzMDcwNjQzMSAxNzIuMTcuMC4xIDM5MzE5IHR5cCBob3N0XHJcbmE9Y2FuZGlkYXRlOjk0MDc2MDk2NyAxIHVkcCAxNjk0NDk4ODE1IDczLjI0My4xLjExIDU1NDY3IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDQ4NTc2XHJcbmE9Y2FuZGlkYXRlOjk0MDc2MDk2NyAyIHVkcCAxNjk0NDk4ODE1IDczLjI0My4xLjExIDU1NDY3IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDQ4NTc2XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNcclxuIn0=

ICE Connection State has changed: checking
ICE Connection State has changed: connected
Data channel 'data'-'824635376524' open. Random messages will now be sent to any connected DataChannels every 5 seconds
Sending 'AUBiqltUsGmfLdH'
Message from DataChannel 'data': 'oOPNoAVEGkOJytm'
Sending 'ONDrJkdvURrBMuc'
Message from DataChannel 'data': 'OVgMvURKIMJLKJY'
^C

$ ./data-channels
eyJ0eXBlIjoib2ZmZXIiLCJzZHAiOiJ2PTBcclxubz0tIDgzNDg1NzQ2MDI2Njc1NjA4MDggMTYxNjAyMzA1NiBJTiBJUDQgMC4wLjAuMFxyXG5zPS1cclxudD0wIDBcclxuYT1maW5nZXJwcmludDpzaGEtMjU2IDgwOkJCOjI0OkU0OjE4OkIyOkQ4OjAxOjZDOkFCOjdCOjY0OjFGOkYwOkNFOjFFOjUyOjE3OkFEOjkyOkFEOjY5OjdFOjQ5OkY1OkVBOkVFOkUwOjFDOkZEOjhGOkFFXHJcbmE9Z3JvdXA6QlVORExFIDBcclxubT1hcHBsaWNhdGlvbiA5IFVEUC9EVExTL1NDVFAgd2VicnRjLWRhdGFjaGFubmVsXHJcbmM9SU4gSVA0IDAuMC4wLjBcclxuYT1zZXR1cDphY3RwYXNzXHJcbmE9bWlkOjBcclxuYT1zZW5kcmVjdlxyXG5hPXNjdHAtcG9ydDo1MDAwXHJcbmE9aWNlLXVmcmFnOnJqYkFzS2pkZnl1aWtVbENcclxuYT1pY2UtcHdkOllSaFhDU21RUUhLUVp5Rk9rZlRFbUtweUFPVE5talJFXHJcbmE9Y2FuZGlkYXRlOjMwMzE3NjQ2MTkgMSB1ZHAgMjEzMDcwNjQzMSAxOTIuMTY4LjI0LjEzMiA1MTczNiB0eXAgaG9zdFxyXG5hPWNhbmRpZGF0ZTozMDMxNzY0NjE5IDIgdWRwIDIxMzA3MDY0MzEgMTkyLjE2OC4yNC4xMzIgNTE3MzYgdHlwIGhvc3RcclxuYT1jYW5kaWRhdGU6MjMzNzYyMTM5IDEgdWRwIDIxMzA3MDY0MzEgMTcyLjE3LjAuMSA1MTI2NiB0eXAgaG9zdFxyXG5hPWNhbmRpZGF0ZToyMzM3NjIxMzkgMiB1ZHAgMjEzMDcwNjQzMSAxNzIuMTcuMC4xIDUxMjY2IHR5cCBob3N0XHJcbmE9Y2FuZGlkYXRlOjk0MDc2MDk2NyAxIHVkcCAxNjk0NDk4ODE1IDczLjI0My4xLjExIDUzNzAyIHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDM3NzU3XHJcbmE9Y2FuZGlkYXRlOjk0MDc2MDk2NyAyIHVkcCAxNjk0NDk4ODE1IDczLjI0My4xLjExIDUzNzAyIHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDM3NzU3XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNcclxuIn0=

ICE Connection State has changed: checking
eyJ0eXBlIjoiYW5zd2VyIiwic2RwIjoidj0wXHJcbm89LSA3MzM4NDEwODE4NDE3NTg5Njg2IDE2MTYwMjMwODAgSU4gSVA0IDAuMC4wLjBcclxucz0tXHJcbnQ9MCAwXHJcbmE9ZmluZ2VycHJpbnQ6c2hhLTI1NiAwOToyMzpFODpEQTpGRDpCRDpFNzoyNjpDNjpBMDpGQzo2QjozQTo0RDo2QTo0NTo5Njo4MTpCRDo4QTo4MTpFMDoyNTo4QzpFNToyRjo2NDozMjo1Qjo2NzpDNzo3MlxyXG5hPWdyb3VwOkJVTkRMRSAwXHJcbm09YXBwbGljYXRpb24gOSBVRFAvRFRMUy9TQ1RQIHdlYnJ0Yy1kYXRhY2hhbm5lbFxyXG5jPUlOIElQNCAwLjAuMC4wXHJcbmE9c2V0dXA6YWN0aXZlXHJcbmE9bWlkOjBcclxuYT1zZW5kcmVjdlxyXG5hPXNjdHAtcG9ydDo1MDAwXHJcbmE9aWNlLXVmcmFnOkNyQlhMa0pTU1N0UkNYeE1cclxuYT1pY2UtcHdkOm1HWGdDSHNiVlNiRFZMWmFxUWJZbmRra1ZQSk9OeEdYXHJcbmE9Y2FuZGlkYXRlOjMwMzE3NjQ2MTkgMSB1ZHAgMjEzMDcwNjQzMSAxOTIuMTY4LjI0LjEzMiA1NzcyMiB0eXAgaG9zdFxyXG5hPWNhbmRpZGF0ZTozMDMxNzY0NjE5IDIgdWRwIDIxMzA3MDY0MzEgMTkyLjE2OC4yNC4xMzIgNTc3MjIgdHlwIGhvc3RcclxuYT1jYW5kaWRhdGU6MjMzNzYyMTM5IDEgdWRwIDIxMzA3MDY0MzEgMTcyLjE3LjAuMSAzOTMxOSB0eXAgaG9zdFxyXG5hPWNhbmRpZGF0ZToyMzM3NjIxMzkgMiB1ZHAgMjEzMDcwNjQzMSAxNzIuMTcuMC4xIDM5MzE5IHR5cCBob3N0XHJcbmE9Y2FuZGlkYXRlOjk0MDc2MDk2NyAxIHVkcCAxNjk0NDk4ODE1IDczLjI0My4xLjExIDU1NDY3IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDQ4NTc2XHJcbmE9Y2FuZGlkYXRlOjk0MDc2MDk2NyAyIHVkcCAxNjk0NDk4ODE1IDczLjI0My4xLjExIDU1NDY3IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDQ4NTc2XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNcclxuIn0=
ICE Connection State has changed: connected
New DataChannel data 824637475034
Data channel 'data'-'824637475034' open. Random messages will now be sent to any connected DataChannels every 5 seconds
Sending 'oOPNoAVEGkOJytm'
Message from DataChannel 'data': 'AUBiqltUsGmfLdH'
Sending 'OVgMvURKIMJLKJY'
Message from DataChannel 'data': 'ONDrJkdvURrBMuc'
^C

Sean-Der added a commit that referenced this issue Mar 17, 2021
@Sean-Der
Close DTLS when fingerprint verification fails
465865e 
Before we would set the PeerConnection to failed, but we would leave the
DTLSTransport. This means that a user could still interact with the
other transports.

Relates to #1708
@Sean-Der Sean-Der mentioned this issue Mar 17, 2021
Merged
@Sean-Der
Copy link
Member

Yea you were 100% right @gaukas before! The test we had wasn't very useful.

I fixed the issue and added a test mind trying #1709 ?

Sean-Der added a commit that referenced this issue Mar 17, 2021
@Sean-Der
Close DTLS when fingerprint verification fails
9a1da17 
Before we would set the PeerConnection to failed, but we would leave the
DTLSTransport. This means that a user could still interact with the
other transports.

Relates to #1708
Sean-Der added a commit that referenced this issue Mar 17, 2021
@Sean-Der
Close DTLS when fingerprint verification fails
545613d 
Before we would set the PeerConnection to failed, but we would leave the
DTLSTransport. This means that a user could still interact with the
other transports.

Relates to #1708
@gaukas
Copy link
Member Author

gaukas commented Mar 18, 2021

Thanks for the fast response! Now I see the problem has been fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Sean-Der @gaukas