Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.23k stars 2.56k forks source link

TLS-ECDH-RSA-* Ciphersuites Allow ECDSA Signed Certificates #1561

Open iluxonchik opened 6 years ago

iluxonchik commented 6 years ago

Description

mbed TLS build: Version: 2.7.0 (I assume it's also present in the newest build, as well as the previous ones)

When the negotiated ciphersuite is of the type TLS-ECDH-RSA-* (ECDH key exchange + RSA signed certificate), ECDSA signed certificates are accepted, which means that the ciphersuite technically becomes TLS-ECDH-ECDSA.

RFC 4492 states that in an ECDH_RSA key exchange, the certificate MUST be signed with RSA.

Proof Of Concept

Due to lack of time, I don't have time to submit a "pretty" POF, but here goes a sample client and server program. The client and the server accept a single argument: the id of the ciphersuite to use. You can use 49201 (TLS-ECDH-RSA-WITH-AES-128-GCM-SHA256), for example.

Here are the sources:

Simply compile both (if you place them in the programs/ssl directory and add the executables in CMake it should work fine.

  1. Compile
  2. Run ./server 49201
  3. Run ./client 49201
  4. Confirm that the connection is successful.

Note that the server is using the certificate (it's printed out to the console):

-----BEGIN CERTIFICATE-----
MIICHzCCAaWgAwIBAgIBCTAKBggqhkjOPQQDAjA+MQswCQYDVQQGEwJOTDERMA8G
A1UEChMIUG9sYXJTU0wxHDAaBgNVBAMTE1BvbGFyc3NsIFRlc3QgRUMgQ0EwHhcN
MTMwOTI0MTU1MjA0WhcNMjMwOTIyMTU1MjA0WjA0MQswCQYDVQQGEwJOTDERMA8G
A1UEChMIUG9sYXJTU0wxEjAQBgNVBAMTCWxvY2FsaG9zdDBZMBMGByqGSM49AgEG
CCqGSM49AwEHA0IABDfMVtl2CR5acj7HWS3/IG7ufPkGkXTQrRS192giWWKSTuUA
2CMR/+ov0jRdXRa9iojCa3cNVc2KKg76Aci07f+jgZ0wgZowCQYDVR0TBAIwADAd
BgNVHQ4EFgQUUGGlj9QH2deCAQzlZX+MY0anE74wbgYDVR0jBGcwZYAUnW0gJEkB
PyvLeLUZvH4kydv7NnyhQqRAMD4xCzAJBgNVBAYTAk5MMREwDwYDVQQKEwhQb2xh
clNTTDEcMBoGA1UEAxMTUG9sYXJzc2wgVGVzdCBFQyBDQYIJAMFD4n5iQ8zoMAoG
CCqGSM49BAMCA2gAMGUCMQCaLFzXptui5WQN8LlO3ddh1hMxx6tzgLvT03MTVK2S
C12r0Lz3ri/moSEpNZWqPjkCMCE2f53GXcYLqyfyJR078c/xNSUU5+Xxl7VZ414V
fGa5kHvHARBPc8YAIVIqDvHH1Q==
-----END CERTIFICATE-----

which is signed with ecdsa-with-SHA256

hanno-becker commented 6 years ago

Thanks @iluxonchik for raising this! On first sight it looks like you're right - we'll investigate in detail and come back to you afterwards.

iluxonchik commented 6 years ago

@hanno-arm no problem. Thank you.

ciarmcom commented 6 years ago

ARM Internal Ref: IOTSSL-2215

abergmann commented 6 years ago

CVE-2018-1000520 was assigned to this issue.

simonbutcher commented 6 years ago

There seems to be wider interest in this issue out on the internet because a CVE was raised against it, so I'd like to clarify how we see the nature of this issue.

The bug is that if the negotiated ciphersuite is of type TLS-ECDH-RSA the library still accepts ECDSA signed certificates when it should only accept RSA signed certificates.

The accepted certificate MUST be signed in the usual way by a given trusted CA, but we accept RSA signature when we should strictly accept only ECDSA.

From a security perspective, the advantage to an attacker is only very slight. An attacker could use an ECDSA signed certificate when it shouldn’t be permitted, but crucially, the certificate must still be signed properly. That means if, for instance, an attacker didn’t have an RSA certificate, and wanted to spoof a server, they could only replace it with an RSA certificate, but that certificate must still have been legitimately signed.

On its own – this issue isn’t exploitable.

It's definitely still a bug, but merely as a security issue it's a weak one.

We're working on a fix and this will be addressed soon.

hanno-becker commented 6 years ago

Some further remarks on the issue depending on the version of TLS that's used:

RFC 4492 states that in an ECDH_RSA key exchange, the certificate MUST be signed with RSA.

For TLS 1.2, RFC 4492 has been updated by RFC 5246, which states:

Note that this also implies that the DH_DSS, DH_RSA, ECDH_ECDSA, and ECDH_RSA key exchange algorithms do not restrict the algorithm used to sign the certificate. Fixed DH certificates MAY be signed with any hash/signature algorithm pair appearing in the [signature-algorithm] extension. The names DH_DSS, DH_RSA, ECDH_ECDSA, and ECDH_RSA are historical.

The only significance that ECDH_{ECDSA,RSA} have in TLS 1.2 is the following, quoting RFC 5246, 7.4.1.4.1:

If the client does not send the signature_algorithms extension, the server MUST do the following:

  • If the negotiated key exchange algorithm is one of (RSA, DHE_RSA, DH_RSA, RSA_PSK, ECDH_RSA, ECDHE_RSA), behave as if client had sent the value {sha1,rsa}.
  • If the negotiated key exchange algorithm is one of (DHE_DSS, DH_DSS), behave as if the client had sent the value {sha1,dsa}.
  • If the negotiated key exchange algorithm is one of (ECDH_ECDSA, ECDHE_ECDSA), behave as if the client had sent value {sha1,ecdsa}.

In particular: In the presence of a signature-algorithm extension, there is no difference between ECDH_ECDSA and ECDH_RSA in TLS 1.2. As Mbed TLS always sends the signature-algorithm extension when using TLS 1.2, it behaves compliant when ignoring the difference between ECDH_ECDSA and ECDH_RSA in TLS 1.2.

In TLS 1.1 or lower, though, the use of ECDH_ECDSA or ECDH_RSA mandates that all signatures in the certificate chain must be ECDSA resp. RSA, which Mbed TLS doesn't check, and which therefore is an issue of non-compliance. However, as @sbutcher-arm stated, certificates still need to be correctly signed to be accepted, and hence the issue isn't significant from the perspective of security.

simonbutcher commented 6 years ago

I've been looking at this further, and while, as @hanno-arm identified, this is non-conformant behaviour for TLS1.1 and earlier (but notably not TLS1.2), there is no immediate security issue. You can only substitute a signed RSA certificate for an equivalently signed ECDSA certificate. There is no loss of security - and to use this as part of an attack, you'd have to have another actual attack which can take advantage of ECDSA being exploitable and RSA not.

In that situation - the client must be capable of ECDSA and RSA to have the code present to do be able to do the ECDSA authentication. In other words, MBEDTLS_ECDSA_C must be defined. And equally, the client must have actively restricted the set of ciphersuites to exclude ECDSA. So my first observation is, if you don't want ECDSA in the library (for whatever reasons) you can compile it out by not defining MBEDTLS_ECDSA_Cin your config.h file. I think that's an acceptable workaround.

Next, on the issue of non-conformance - if a client indicates by ciphersuire that it can only accept from the server certificates that are signed by RSA, and despite this, a server still provides an ECDSA certificate (possibly because that's all it has), which is itself a matter of non-conformance, and quite an explicit one according to the RFC, do we really want the library really to reject the certificate and thereby deny a connection?

There is no immediate security advantage, because as I've already established those certificates are already valid, and have been authenticated, so ignoring cryptographic differences, are equivalent.

I can see users losing out in losing connections for no immediate or obvious advantage. We'd be adding in additional verification code which will expand our code footprint also. Therefore I'd like to propose we don't fix this issue.

@mpg, @andresag01, @hanno-arm, @yanesca, @gilles-peskine-arm - can I please have your views. If you agree and have nothing to add - please give this a thumbs up.

Also particularly interested in views from @iluxonchik, and @abergmann, who see this as a security issue. I'd like to know if you think we're missing something.

gilles-peskine-arm commented 6 years ago

@sbutcher-arm I have a slightly different analysis, but come to the same conclusion that we shouldn't change the behavior, at least for now.

Our code does not comply with the specification in an edge case (ECDSA supported but ECDSA ciphersuites disabled, signature_algorithms extension not used, the other party sends an ECDSA certificate in an RSA ciphersuite). Can this be a security issue? Yes, in the same way that pretty much any non-compliant behavior can break the security workflow. For example: suppose I have an application that has a configuration option for permitted ciphersuites, which currently allows both ECDSA and RSA certificates, and which only supports TLS 1.1. Suppose that a security issue is discovered in the ECDSA signature verification implementation in Mbed TLS and the advice is to upgrade, but it's inconvenient for me to upgrade. So I disable ECDSA ciphersuites in the belief that I will be protected, but I am not protected because Mbed TLS will still accept ECDSA-signed certificates. The present issue doesn't open a security hole in itself, but it can result in a lack of defense in depth against a bigger problem.

This is still a very minor security issue because some very specific constraints need to apply before this becomes an attack. For example, if my application allowed TLS 1.2 then it would automatically support the signature_algorithms extension and so it would be supposed to accept ECDSA certificates in RSA ciphersuites, so in the scenario above disabling ECDSA ciphersuites would not protect me.

Because this issue can only affect security in a corner case and only in combination with an issue with a much bigger security impact, we need to balance it against other considerations including interoperability and where our time is best spent. Where our time is best spent has already been blown out of the water with this discussion. But there's still interoperability to consider. Our implementation doesn't follow the RFC perfectly in a corner case — are we the only one? Do some of our users rely on the current behavior, perhaps unwittingly? I wouldn't want to break these users' workflow just for the sake of being compliant. If these users were at risk, then we should protect them by at the very least changing the default behavior and letting them make an informed choice between compliance and security. But here the security risk due to non-compliance only occurs in edge cases and involve specific application configurations. Therefore I think the best thing to do is to ensure that we're properly documenting the library's behavior, and not change this behavior.

After all, the workaround for this issue is “avoid older protocol versions, and then the erroneously accepted behavior will be legal”. If the workaround for a bug is to make it not-a-bug, then as bugs go, it isn't much of a bug.

This comment is my personal opinion and is not intended to reflect the opinion of the Mbed TLS team, of my employer or any other person or entity.

yanesca commented 6 years ago

To summarise your conclusion:

I agree to this assessment and with the proposal of not fixing it. (Unless of course someone can see a major security issue here that we all missed.)

simonbutcher commented 5 years ago

I really don't think this merits a CVE, which to me implies something immediately exploitable - and this is not. There is no compromise of authenticity or integrity with just this issue - it would need to be used with another bug which does that, as @gilles-peskine-arm says. Therefore I'm trying to get the CVE rejected.

There's definitely a conformance bug to handle here, so I won't close the bug, and we will look at this again soon, although as it stands, I'm not sure if we'll fix it or leave it as-is.

Views welcome from anyone who can see a security issue we can't.

yanesca commented 3 years ago

This issue was closed down by mistake, therefore I am reopening it.