Always revoke certificate on CRL

[raoulstrackx]

Description

Always revoke certificate on CRL

RFC5280 (https://tools.ietf.org/html/rfc5280) does not state that the revocationDate should be checked.

In addition, when no time source is available (i.e., when MBEDTLS_HAVE_TIME_DATE is not defined), mbedtls_x509_time_is_past always returns 0. This results in the CRL not being checked at all.

Status

READY

Requires Backporting

Yes

• 2.16 - Backport 2.16: Always revoke certificate on CRL #3562
• 2.7 - Backport 2.7: Always revoke certificate on CRL #3561

Todos

• Tests
• Changelog updated

Fix #3340

[mpg]

Thanks for your contribution! I agree with your assessment: I looked at all instances of "revocationDate" in RFC 5280 and it never says that it should be checked. Moreover, point (j) on page 95 says to marked as revoked as soon as issuer name and serial number match. It looks like this field is more informational (a bit like the optional "reason" field.) Also, from a security perspective, if the certificate ended up on a CRL, it shouldn't be trusted regardless of the revocation date, especially considering the local clock can't always be trusted.

The new behaviour should come with a test. Please generate a CRL with a revocation date in the far future (to make sure it's still in the future in a few years) for one of our existing certificate and add a test for verifying this certificate with that CRL using the function x509_verify in tests/suites/test_suite_x509parse.data. The generated CRL file should live in tests/data_files and its generation method documented in tests/data_files/Makefile. In order to obtain a revocation date in the future, you might need to use the faketime utility (see examples in that Makefile). Feel free to ask for help on testing if needed, we're aware that out test system is not always intuitive first time you touch it.

Since this changes the observable behaviour, it deserves an entry in our ChangeLog, I'd say under Bugfix as the current behaviour isn't in accordance to the standard. Can you write one? See [https://github.com/ARMmbed/mbedtls/blob/development/ChangeLog.d/00README.md](the ChangeLog README).

Finally, can you add a Signed-Off-By line to your commit message, ton indicate that you agree with our licensing terms?

[mpg]

The change itself looks good to me, but there are a few additional things to do:

• add a test
• add a ChangeLog entry
• add a Signed-Off-By line in each commit message

[mpg]

Thanks for adding a ChangeLog entry and unit tests as requested! This is looking quite good, except for a typo in the ChangeLog entry; I also suggested a possible improvement to the test descriptions.

[mpg]

That's an interesting point. As you've noted when adding tests (thanks for that!), currently we have a config.h option MBEDTLS_HAVE_TIME_DATE whose documentation reads:

 * System has time.h, time(), and an implementation for
 * mbedtls_platform_gmtime_r() (see below).
 * The time needs to be correct (not necessarily very accurate, but at least
 * the date should be correct). This is used to verify the validity period of
 * X.509 certificates.
 *
 * Comment if your system does not have a correct clock.

Perhaps this documentation should be made more explicit:

 * \warning You should comment this out if the system's clock could be controlled by an attacker.

I'm mentioning this because my initial reaction was "but on systems where the local clock can't be trusted, people should just undefine MBEDTLS_HAVE_TIME_DATE" - then I checked if the documentation was actually making that clear, and perhaps not enough.

[gilles-peskine-arm]

“people should just undefine MBEDTLS_HAVE_TIME_DATE” That depends on your threat model. Undefining MBEDTLS_HAVE_TIME_DATE means that no certificate will ever be considered to have expired.

[raoulstrackx]

I agree description could be more clear. The same applies for MBEDTLS_HAVE_TIME: when the system's clock can't be trusted, it also cannot be trusted for measuring time differences.

[raoulstrackx]

Good point @gilles-peskine-arm but if you leave MBEDTLS_HAVE_TIME_DATE defined, things like SSL caches may never expire.

[mpg]

Ok, after thinking a bit more about this, I think I was wrong to suggest placing the ChangeLog entry under "Bugfix". As the PR description says, when MBEDTLS_HAVE_TIME_DATE is unset, we would simply never consider any certificate as revoked. I'm not sure how I overlooked this before, but that's enough to qualify as a security issue.

I'm now thinking the ChangeLog entry should look more like:

Security
   * When checking X.509 CRLs, a certificate was only considered as revoked if its revocationDate was in the past according to the local clock if available. In particular, on builds without MBEDTLS_HAVE_TIME_DATE, certificates were never considered as revoked. On builds with MBEDTLS_HAVE_TIME_DATE, an attacker able to control the local clock (for example, an untrusted OS attacking a secure enclave) could prevent revocation of certificates via CRLs. Fixed by no longer checking the revocationDate field, in accordance with RFC 5280.  Reported and fixed by Raoul Strackx & Jethro Beekman.

[mpg]

Thanks for updating the PR according to our feedback.

As commented above, I think I was wrong to recommend a "bugfix" changelog entry.

Also, I'm afraid one of the tests you added is currently not exercised in our CI, which should be fixed.

Finally, regarding changes to the documentation and to the ChangeLog, you may want to wait for Gilles to weigh in, in case we're not immediately in agreement, to avoid making changes in different directions each time one of us comments.

[mpg]

Thanks for updating the PR. I'm satisfied with the result, but the CI complains about whitespace issues, which need to be fixed.

[raoulstrackx]

@mpg could you send me the output of CI? Running the tests locally doesn't give me much insight. There are failed tests, but those fail on the development branch too.

[raoulstrackx]

One of the tests that fail is:

client:

../programs/ssl/ssl_client2 server_port=14928 server_addr=127.0.0.1 force_version=tls1_2 ca_file=none auth_mode=none crt_file=none key_file=none force_ciphersuite=TLS-ECDHE-ECDSA-WITH-AES-128-CBC-SHA256
                                                                                                                                                                                                                                             
  . Seeding the random number generator... ok                                                                                                                                                                                                
  . Loading the CA root certificate ... ok (0 skipped)                                                                                                                                                                                       
  . Loading the client cert. and key... ok (key type: invalid PK)                                                                                                                                                                            
  . Connecting to tcp/127.0.0.1/14928... ok                                                                                                                                                                                                  
  . Setting up the SSL/TLS structure... ok                                                                                                                                                                                                   
  . Performing the SSL/TLS handshake... failed                                                                                                                                                                                               
  ! mbedtls_ssl_handshake returned -0x7780                                                                                                                                                                                                   
                                                                                                                                                                                                                                             
Last error was: -0x7780 - SSL - A fatal alert message was received from our peer                                                                                                                                                             
                                                                                                                                                                                                                                             
EXIT: 1                                                                                                                                                                                                                                      
EXIT: 1

server:

gnutls-serv -p 14928 --http  --disable-client-cert --x509certfile data_files/server5.crt --x509keyfile data_files/server5.key --priority NORMAL:+AES-256-CCM-8:+AES-128-CCM-8:+ARCFOUR-128:+NULL:+MD5:+PSK:+DHE-PSK:+ECDHE-PSK:+RSA-PSK:-VERS-TLS-ALL:+VERS-TLS1.2                                                                                                                                                                                                                        
HTTP Server listening on IPv4 0.0.0.0 port 14928...done
HTTP Server listening on IPv6 :: port 14928...done                                                                                                                                                                                           
No certificates found!                                                                                                                                                                                                                       
Set static Diffie-Hellman parameters, consider --dhparams.                                                                                                                                                                                   
                                                                                                                                                                                                                                             
* Accepted connection from IPv4 127.0.0.1 port 51090 on Wed Jul  1 13:45:09 2020                                                                                                                                                             
- Description: (TLS1.2)-(ECDHE-ECDSA-SECP521R1)-(AES-128-CBC)-(SHA1)                                                                                                                                                                         
- Session ID: 3A:1B:0B:91:85:1B:B3:D2:D3:CA:5B:83:6B:AD:8C:B0:35:4A:21:01:72:E6:F5:B3:65:73:0D:F0:E0:C6:D3:21                                                                                                                                
- Given server name[1]: localhost                                                                                                                                                                                                            
- Ephemeral EC Diffie-Hellman parameters                                                                                                                                                                                                     
 - Using curve: SECP521R1                                                                                                                                                                                                                    
 - Curve size: 528 bits                                                                                                                                                                                                                      
- Version: TLS1.2
- Key Exchange: ECDHE-ECDSA
- Server Signature: ECDSA-SHA512
- Cipher: AES-128-CBC
- MAC: SHA1
- Compression: NULL
- Options: extended master secret, safe renegotiation, EtM,
- Channel binding 'tls-unique': 0c25dc0a33cd9291f2a8d0e0
No certificates found!

* Accepted connection from IPv4 127.0.0.1 port 51092 on Wed Jul  1 13:45:09 2020
- Description: (TLS1.2)-(ECDHE-ECDSA-SECP521R1)-(AES-256-CBC)-(SHA1)
- Session ID: 6F:38:69:0F:4D:1E:24:32:B2:1A:F6:E3:51:86:B9:1E:23:87:83:27:D3:A5:2A:26:29:6B:61:3B:FB:D6:60:39
- Given server name[1]: localhost
- Ephemeral EC Diffie-Hellman parameters
 - Using curve: SECP521R1
 - Curve size: 528 bits
- Version: TLS1.2
- Key Exchange: ECDHE-ECDSA
- Server Signature: ECDSA-SHA512
- Cipher: AES-256-CBC
- MAC: SHA1
- Compression: NULL
- Options: extended master secret, safe renegotiation, EtM,
- Channel binding 'tls-unique': f2fe159ed656e501fe4a5b85
Error in handshake

[gilles-peskine-arm]

You don't need to do anything else to pass CI.

• Travis passed.
• pr-head had a single test case failure which is in a DTLS test where an unexpected resend happened. This can happen if the OS drops a UDP packet, which is rare but possible. So that's as good as a pass.
• pr-merge passed with an older version of development, and didn't run with the latest version due to the "needs work" label. I've kicked a new run, should be over in an hour or so.
• Mbed OS jobs are failing due to an unrelated change on the Mbed OS side. We're ignoring Mbed OS CI results until that's fixed.

If you're getting failures with interoperability tests locally, it's probably because you're using different versions of OpenSSL and GnuTLS. Different tests need different versions because older versions lack some features or have bugs, and newer versions have removed some obsolete features. The docker image in tests/docker should work.

[raoulstrackx]

Thanks for your help!

Who would be backporting the patch?

[mpg]

Well, if you're willing to do it, that would be much appreciated. It would involve raising two new PRs, one targeting mbedtls-2.16, the other targeting mbedtls-2.7, with the same content as this one (usually git cherry-pick is helpful for that), and then linking to those in this PR's description.

Otherwise, we can handle it ourselves, but that may be a bit slower as that means involving more people on our side (1 for creating the PRs, 2 for reviewing them, while if you can create the PRs, the 2 reviewers who approved the original can usually review the backports pretty quickly).

Just let us know.

[raoulstrackx]

Sure I can do that.

[raoulstrackx]

Backported as #3561 and #3562

[gilles-peskine-arm]

Unfortunately, there's a conflict. It's a simple add/add conflict, but it prevents merging. Please either merge with the latest state of the development branch or rebase on top of development.

[mpg]

Thanks for resolving the conflict. I've done the rebase myself and got an equivalent result.

[gilles-peskine-arm]

Thanks. Please resolve the similar conflict in the backports as well.

[raoulstrackx]

Sorry for the late response. Merge conflicts have been resolved