search

pion / dtls

DTLS 1.2 Server/Client implementation for Go
https://pion.ly/
MIT License
604 stars 158 forks source link

Fix RSA signature verification issue #679

Closed hoihochan closed 2 weeks ago

hoihochan commented 2 weeks ago

Both verifyKeySignature() and verifyCertificateVerify() has a bug when handling RSA signature as it looks at the signature algorithm of the certificate to determine whether to verify with RSA PKCSv1.5. This will cause issues if the certificate's issuing CA uses something other than RSA (e.g. ECDSA) to sign the certificate.

Since DTLS v1.2 does not support RSA-PSS [1], we can just use RSA PKCSv1.5 verification directly if the public key of the certificate is RSA.

[1] https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-16

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.23%. Comparing base (98a05d6) to head (d2d2fee). Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
crypto.go 50.00% 2 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #679 +/- ## ========================================== - Coverage 80.18% 78.23% -1.95% ========================================== Files 101 101 Lines 5374 6567 +1193 ========================================== + Hits 4309 5138 +829 - Misses 695 1058 +363 - Partials 370 371 +1 ``` | [Flag](https://app.codecov.io/gh/pion/dtls/pull/679/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | | |---|---|---| | [go](https://app.codecov.io/gh/pion/dtls/pull/679/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `78.26% <50.00%> (-1.95%)` | :arrow_down: | | [wasm](https://app.codecov.io/gh/pion/dtls/pull/679/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `62.92% <25.00%> (-1.56%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Sean-Der commented 2 weeks ago

Thank you so much @hoihochan !

How was the library otherwise? Any things I can improve/change?

hoihochan commented 2 weeks ago

Thank you so much @hoihochan !

How was the library otherwise? Any things I can improve/change?

It's been good. We had an old fork of the library with this fix and I wanted to upstream the change so we can use the upstream library.

When I get time I would like to help on https://github.com/pion/dtls/issues/524

Sean-Der commented 2 weeks ago

I would love your help!

Reach out if you need anything. Really appreciate any help :)