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.

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

Dtls srtp support #1813

Closed
wants to merge 31 commits into from
Closed

Conversation

RonEld
Copy link
Contributor

@RonEld RonEld commented Jun 28, 2018

Description

This PR supersedes PR #361 by @jeannotlapin, with additional mki support, some rework, and rebased on current development branch
Supersedes #1540

Open question:
How will the server decide whether to accept the MKI value from the client, in the use_srtp extension? Should there be a callback function? Should the server user set the MKI in advance and the server send it if it matches? Should the server just accept the MKI from the client and use it as its MKI ( basically making mbedtls_ssl_dtls_srtp_set_mki_value() a client only function)?

Status

READY

Requires Backporting

NO

Additional comments

Missing still:
tests
interop tests

Todos

  • Tests
  • Documentation
  • Changelog updated
  • Backported

Steps to test or reproduce

run ssl-opt.sh -f SRTP

@jeannotlapin
Copy link
Contributor

@RonEld,
here are patches to fix the self-signed certificate usage and the need of server certificateVerify messages.
It shall allow interop with webRTC.
First patch just merge the release 2.11.0 into this pr branch (I did it on the PR1540, it shall still be ok)
The second one applies the fix.

I didn't performed intensive testing but it shall work. Still we need tests (ideally interop ones but it's difficult to automatize) for the PR.
Johan

0001-Change-the-library-version-to-2.11.0.patch.txt
0002-DTLS-SRTP-forces-ssl-verify-to-optional-when-not-set.patch.txt

@RonEld
Copy link
Contributor Author

RonEld commented Jul 1, 2018

@jeannotlapin Thank you for supplying your patches.
I am puzzled with the set authmode to optional part.
The RFC states that the server must send a certificate in dtls-srtp. Why do you think it's ok to keep the authmode optional in this case?

@RonEld
Copy link
Contributor Author

RonEld commented Jul 1, 2018

@jeannotlapin I believe that the reason you get failure when the server sends their self signed certificate, is because you haven't set the server's trusted root CA certificate through mbedtls_ssl_conf_ca_chain(). When you set the authmode to optional, the certificate verification could still fail, but you ignore it.
This is not the desired behavior.
If it's a different issue, please clarify.

@jeannotlapin
Copy link
Contributor

@RonEld
The regular DTLS SRTP handshake when interfacing webRTC can't provide the root CA as trusted chain.
webRTC is generating on the fly a self signed certificate (actually it's a temporary one used few days)
Setting the verify to optional will force the optional certificateRequest packet to be sent by the server to follow the RFC as both sides will provide certificate. A certificate fingerprint is sent on the signaling channel during call establishment allowing to confirm the trust on the peer certificate (being client or server) without using the regular TLS certificate chain mechanism.

So when doing a DTLS handshake to produce SRTP keys, it is requested to use client and server certificate but you do not need to verify them during the handshake as they are supposed to be verified through the certificate fingerprint mechanism.

That's why my patch will only upgrade to optional the verify mode when set to none as you may use the DTLS handshake for SRTP in other context and thus use the regular verify mode.

@RonEld
Copy link
Contributor Author

RonEld commented Jul 2, 2018

@jeannotlapin Thank you for your explanation.
To ease my concerns, could you please refer me to were this certificate fingerprint is mentioned?
Are you referring to RFC 5763?

I am concerned that we allow a handshake with a peer which certificate is not verified successful.
It might work with the specific dtls-srtp infrastructure you are working with, but can you guarantee it will work for all?

@roxlu
Copy link

roxlu commented Jul 2, 2018

I tried to use this PR and after applying the patches it indeed succeeds the "verification" but I do receive this error "ECP - The signature is not valid". It might be that I'm not setting up my self-signed certificate correctly (?). I'm not sure if I need to configure ECP.

@jeannotlapin
Copy link
Contributor

@RonEld yes I was referring to RFC5763.
Thing is that the patch will just turn on the certificate exchange from both parts if the user set the verify to none but it won't downgrade to no verification if the user set the ssl_verify_required.

So if a user set the flag to verify_none it will force the use of client and server certificate anyway as requested by the RFC5764 but setting the verify to optional. In case the verifyy flag was set to required the patch will have no effect so it won't make any damage to a scheme explicitely requesting the verification to take place.

The general usage of DTLS-SRTP uses self-signed certificate (or it means that each end user could provide a valid certificate that could be verified by its correspondant but it is nearly never the case when using VOIP).

@roxlu do you think your error is linked to the patch? Was it ok before the patch and without the commit forcing the use of certificate ?

@RonEld
Copy link
Contributor Author

RonEld commented Jul 2, 2018

@jeannotlapin Thanks for your explanation

@roxlu When the authmode is optional, thenthe handshake will continue, ignoring the verification error, if there was. Could you clarify what API returns this "ECP - The signature is not valid` error? IF you are ignoring the error with optional authmode, you shouldn't get this error.

@roxlu
Copy link

roxlu commented Jul 2, 2018

@RonEld I've been looking into this a bit more and currently I think this could be caused because I'm passing wrong buffers into mbedtls. This could be the case but I've been testing/using the dtls_srtp_support PR for the last ~6 weeks (I was using 9ebb178 but I can't find that commit anymore).

So let's assume it's not an issue related to my code. If that's the case then this paste is interesting: https://gist.github.com/roxlu/7e870f739876d51060aa673816cdd0de#file-mbedtls-log-L67. There you can see that it detects a bad chipher change message. When you look at the code which triggers this error: https://gist.github.com/roxlu/7e870f739876d51060aa673816cdd0de#file-ssl_tls-c-L15 you see that it's triggered because of a wrong message type. The type that was parsed by mbedtls is 15, which is a "Certificate Verify". I'm using mbedtls_ssl_conf_authmode(&ssl_conf, MBEDTLS_SSL_VERIFY_OPTIONAL); which should have disabled this as per your previous comment.

@RonEld
Copy link
Contributor Author

RonEld commented Jul 2, 2018

@roxlu The commit you referenced has changed for some reason to 15179bf
Probably do to rebase I have done or something else.
The only code changing commit since, is my commit for enforcing the certificate messages, which the patch should resolve.
Are you using mki? I have just noticed that the mki value is a bit buggy, I will work on it.

@RonEld
Copy link
Contributor Author

RonEld commented Jul 2, 2018

In addition, the certificate_verify message shouldn't have been disabled. The verification result itself should have been ignored. The certificate_verify message is still mandatory in RFC 5764.

@roxlu
Copy link

roxlu commented Jul 2, 2018

@RonEld Ok, when using the commit (which was gone) I did not see a "Certificate Verify" message. When I'm currently setting auth mode to ..._OPTIONAL but also testing with ..._NONE. And I would expect this code to be executed then https://github.com/RonEld/mbedtls/blob/dtls_srtp_support/library/ssl_tls.c#L4448 which is not the case.

I'm not using MKI as far as I'm aware of.

@RonEld
Copy link
Contributor Author

RonEld commented Jul 3, 2018

@roxlu the code you are refering will not be executec in this case, because authomde is not MBEDTLS_SSL_VERIFY_NONE .
You should verify that both peers work with same version of the SW. For example, have youupdated only one peer of the negotiation? This would explain why you don't expect the verify_certificate message.

@RonEld
Copy link
Contributor Author

RonEld commented Jul 3, 2018

Added an open questoin in the description, adding to comment as well, for discussion:

How will the server decide whether to accept the MKI value from the client, in the use_srtp extension? Should there be a callback function? Should the server user set the MKI in advance and the server send it if it matches? Should the server just accept the MKI from the client and use it as its MKI ( basically making mbedtls_ssl_dtls_srtp_set_mki_value() a client only function)?

@RonEld
Copy link
Contributor Author

RonEld commented Jul 4, 2018

I fixed some MKI related issues. Among them, answering my open question, deciding that the server will accept and use the MKI value received from the client.
Comments are welcome

@RonEld
Copy link
Contributor Author

RonEld commented Jul 10, 2018

Added some interop tests in ssl-opts.sh with gnutls and openssl

Copy link
Contributor Author

@RonEld RonEld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general note: check that lines don't exceed 80 characters

*
* \param conf SSL configuration
* \param support_mki_value Enable or disable (MBEDTLS_SSL_DTLS_SRTP_MKI_UNSUPPORTED or
* MBEDTLS_SSL_DTLS_SRTP_MKI_SUPPORTED)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix alignment

*
* \return 0 on success, MBEDTLS_ERR_SSL_BAD_INPUT_DATA or MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE
*/
int mbedtls_ssl_dtls_srtp_set_mki_value( mbedtls_ssl_context *ssl, unsigned char* mki_value, size_t mki_len );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: change to unsigned char *mki_value

*
*/
if( ssl->conf->dtls_srtp_mki_support == MBEDTLS_SSL_DTLS_SRTP_MKI_SUPPORTED &&
ssl->dtls_srtp_info.mki_len != 0 )
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix alignment

* } UseSRTPData;

* SRTPProtectionProfile SRTPProtectionProfiles<2..2^16-1>;
*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra line

{
mki_len = ssl->dtls_srtp_info.mki_len;
}
/* Extension length = 2bytes for profiles length, ssl->conf->dtls_srtp_profile_list_len*2 (each profile is 2 bytes length ) + 1 byte for srtp_mki vector length and the mki_len value */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check that doesn't exceed 80 chars

@@ -344,6 +362,9 @@ struct options
int fallback; /* is this a fallback connection? */
int extended_ms; /* negotiate extended master secret? */
int etm; /* negotiate encrypt then mac? */
int use_srtp; /* Support SRTP */
int force_srtp_profile; /* SRTP protection profile to use or all */
const char* mki; /* The dtls mki value to use */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to char *mki

if( opt.force_srtp_profile != DFL_SRTP_FORCE_PROFILE )
{
const mbedtls_ssl_srtp_profile forced_profile[] = { opt.force_srtp_profile };
ret = mbedtls_ssl_conf_dtls_srtp_protection_profiles( &conf, forced_profile, sizeof( forced_profile ) / sizeof( mbedtls_ssl_srtp_profile ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix alignment

MBEDTLS_SRTP_AES128_CM_HMAC_SHA1_32,
MBEDTLS_SRTP_NULL_HMAC_SHA1_80,
MBEDTLS_SRTP_NULL_HMAC_SHA1_32 };
ret = mbedtls_ssl_conf_dtls_srtp_protection_profiles( &conf, default_profiles, sizeof( default_profiles ) / sizeof( mbedtls_ssl_srtp_profile ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix alignment

if( opt.force_srtp_profile != DFL_SRTP_FORCE_PROFILE )
{
const mbedtls_ssl_srtp_profile forced_profile[] = { opt.force_srtp_profile };
ret = mbedtls_ssl_conf_dtls_srtp_protection_profiles( &conf, forced_profile, sizeof( forced_profile ) / sizeof( mbedtls_ssl_srtp_profile ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix alignment

MBEDTLS_SRTP_AES128_CM_HMAC_SHA1_32,
MBEDTLS_SRTP_NULL_HMAC_SHA1_80,
MBEDTLS_SRTP_NULL_HMAC_SHA1_32 };
ret = mbedtls_ssl_conf_dtls_srtp_protection_profiles( &conf, default_profiles, sizeof( default_profiles ) / sizeof( mbedtls_ssl_srtp_profile ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix alignment

@RonEld RonEld added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jul 12, 2018
@jeannotlapin
Copy link
Contributor

Hi Ron,
any news on merging this PR?
cheers,
Johan

Copy link

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work, @jeannotlapin and @RonEld! In regards to the code in this PR, I have the following comments:

  1. Two blockers: A missing allocation validation and a missing bounds check during extension parsing on the server.
  2. I pointed at some places where code size and robustness could be improved.
  3. There are numerous minor style and documentation issues that need to be addressed.

Apart from that, the intended use of the new SRTP functionality should be clarified. Specifically, the RFC says that after the handshake the RTP traffic still passes through the DTLS stack:

  1. On the sending side, the DTLS stack calls the SRTP stack to protect the packet and then forwards it to the underlying transport layer:

When a user of DTLS wishes to send an RTP packet in SRTP mode, it
delivers it to the DTLS implementation as an ordinary application
data write (e.g., SSL_write()). The DTLS implementation then invokes
the processing described in RFC 3711, Sections 3 and 4. The
resulting SRTP packet is then sent directly on the wire as a single
datagram with no DTLS framing.

  1. On the receiving side, the DTLS stack demultiplexes DTLS vs. SRTP by considering the first byte, calls the SRTP stack to deprotect the package if it's an SRTP one, and then forwards to the RTP stack. The passage through the DTLS stack is mandatory if rekeying is desired.

With the current code, to my understanding, users would instead (a) Perform a DTLS handshake, (b) Extract the SRTP keying material and forward it to the SRTP stack, (c) remove DTLS context and subsequently send/receive everything through the SRTP stack. I'm not sure if that's what we want, or whether we should stick to the RFC more closely.

ChangeLog Outdated
@@ -6,6 +6,8 @@ Features
* Add new crypto primitives from RFC 7539: stream cipher Chacha20, one-time
authenticator Poly1305 and AEAD construct Chacha20-Poly1305. Contributed by
Daniel King (#485).
* Add support for DTLS-SRTP as defined in RFC 5764. Based on contribution done

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Based on a contribution. Also, 'contribution done by' sounds slightly awkward. Suggestion: Based on #361 contributed by Johan Pascal.

*
* Comment this to disable support for DTLS-SRTP.
*/
#define MBEDTLS_SSL_DTLS_SRTP

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be disabled by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, for testing it was turned on, and accidentally pushed that way

@@ -354,6 +359,14 @@

#define MBEDTLS_TLS_EXT_RENEGOTIATION_INFO 0xFF01

/*
* use_srtp extension protection profiles values as defined in http://www.iana.org/assignments/srtp-protection/srtp-protection.xhtml

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Please avoid the overly long line.


typedef struct mbedtls_dtls_srtp_info_t
{
mbedtls_ssl_srtp_profile chosen_dtls_srtp_profile; /*!< negotiated SRTP profile */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Please avoid overly long lines and use full sentences in documenting this public structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but I am not sure about how doxygen treats comments not in the same line

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you begin with /*! ..., it applies to the next line; if you begin with /*!< ..., it applies to the current line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for nitpicking, but I think it may be useful to remark, that, to be precise, /*! ... says that the documented code is after the comment, and /*!< ... says it's before the comment. Therefore, the latter type doesn't really need to be on the same line; it may be on the next one. This may come in handy, in case of long comments documenting long lines of code.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k-stachowiak Useful nitpicking, thanks!

@@ -923,6 +972,11 @@ struct mbedtls_ssl_config
const char **alpn_list; /*!< ordered list of protocols */
#endif

#if defined(MBEDTLS_SSL_DTLS_SRTP)
mbedtls_ssl_srtp_profile *dtls_srtp_profile_list; /*!< ordered list of supported srtp profile */
size_t dtls_srtp_profile_list_len; /*!< number of supported profiles */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Pick matching alignment for the comments.

ssl->dtls_srtp_info.dtls_srtp_keys_len );
*key_len = ssl->dtls_srtp_info.dtls_srtp_keys_len;

return 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Please add brackets.

unsigned char c;
size_t j;
if( unhexify( psk, opt.psk, &psk_len ) != 0 )
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Indentation

const mbedtls_ssl_srtp_profile forced_profile[] = { opt.force_srtp_profile };
ret = mbedtls_ssl_conf_dtls_srtp_protection_profiles( &conf,
forced_profile,
sizeof( forced_profile ) / sizeof( mbedtls_ssl_srtp_profile ) );

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Please avoid overly long lines.

MBEDTLS_SRTP_NULL_HMAC_SHA1_32 };
ret = mbedtls_ssl_conf_dtls_srtp_protection_profiles( &conf,
default_profiles,
sizeof( default_profiles ) / sizeof( mbedtls_ssl_srtp_profile ) );

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Please avoid overly long lines.

size_t j;

*olen = strlen( input );
if( *olen % 2 != 0 || *olen / 2 > MBEDTLS_PSK_MAX_LEN )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now used for MKI parsing, too, so we shouldn't apply a PSK specific bound anymore.

Ron Eldor added 5 commits September 26, 2019 14:28
Add a comment that describes that the feature only supportes the
`use_srtp` extension, and not hte full DTLS-SRTP RFC.
Change setting the mki value byte after byte with `memcpy()`.
1. Check allocation success.
2. Check parameter correctness in the use_srtp extension
in server and client.
Disable `MBEDTLS_SSL_DTLS_SRTP` by default in the configuration file.
Force using IPv4 in the GNU_CLI SRTP tests, as introduced for
other tests in Mbed-TLS#1918.
Ron Eldor added 2 commits September 26, 2019 17:20
Use the export keys functionality, to call the public API
`mbedtls_ssl_tls_prf()`, and remove the function
`mbedtls_ssl_get_dtls_srtp_key_material()`.
Add the DTLS_SRTP configuration to `query_config`.
@fire
Copy link

fire commented Jan 15, 2020

Is this pr still active?

@simonbutcher
Copy link
Contributor

Yes, I think it is. We've been trying to get SRTP into the project for years, and this work has been worked on and off by many people. I don't know if @RonEld is still actively working on it, but if he isn't I don't mind picking it up and championing it in a week or two.

@RonEld - Are you still actively working on this? And if not, would you like me to take it off your hands?

@RonEld
Copy link
Contributor Author

RonEld commented Jan 16, 2020

@sbutcher-arm I haven't been working on this lately.
Please pick it up.
I would replace the key derivation part to the TLS PRF key derivation feature

@simonbutcher
Copy link
Contributor

Thanks for all your work on this @RonEld! I'm happy to take it off you and do the small push to get this over the line.

@simonbutcher simonbutcher assigned simonbutcher and unassigned RonEld Jan 16, 2020
@jeannotlapin
Copy link
Contributor

Hi Simon,
any update on this merge? I was planning to add support for SRTP_AEAD_AES_128_GCM and SRTP_AEAD_AES_256_GCM profiles as defined in RFC 7714 section 14.2. I'll do that once the patch is stable and ready to merge (or just after the merge and I'll do a new PR in order not to delay anymore this one).
Thanks.

@simonbutcher
Copy link
Contributor

Hi @jeannotlapin. If you want to add features, I recommend that as separate PRs and given the age of this PR, I personally think it would be better to get this one in first.

As an update - I haven't started work on this one yet, and plan to soon.

If you wanted to flip this around, and do the rebase yourself and any rework, and I'll review it, you're more than welcome.

@jeannotlapin
Copy link
Contributor

Thanks @sbutcher-arm

I'll wait for your rebase and merge before working on the upgrade on a new PR. It's not something I have time to do right now anyway.

@jeannotlapin
Copy link
Contributor

Hi @sbutcher-arm ,
I just rebased this PR on the current development branch. You can find it on:

https://github.com/jeannotlapin/mbedtls/tree/pr1813

Any chance it would help merging it? Shall I create a new PR from this branch?

cheers

@gilles-peskine-arm
Copy link
Contributor

Hi @jeannotlapin,

Thanks for staying with us and for following up! Because this is a substantial contribution (and thank you for that), I can't promise that we'll have the bandwidth to review it in the near future. However, if there isn't a pull request with no merge conflicts and passing CI, we definitely won't review it. So since you've done this rebase, please to make a new pull request, and we'll add it to our backlog we're starting to make public at https://github.com/ARMmbed/mbedtls/projects/3.

@danh-arm
Copy link
Contributor

Superseded by #3235

@danh-arm danh-arm closed this May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-tls enhancement needs-review Every commit must be reviewed by at least two team members,
Projects
None yet
Development

Successfully merging this pull request may close these issues.