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

Initial version of the CA public key fingerprint check. #47

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

slaff
Copy link
Contributor

@slaff slaff commented Jul 18, 2017

Tested on real device with three different web-site certificates.
This PR is just initial work we can build on top of it to allow fingerprinting on CA'a public key.

Related to #44.

@slaff slaff force-pushed the feature/ca-key-fingerpint branch 3 times, most recently from 64132da to db727ff Compare July 19, 2017 13:38
@slaff
Copy link
Contributor Author

slaff commented Jul 19, 2017

@igrr Any comments on this? The code is tested on real device and was working as expected. I would be glad to get your feedback.

Copy link
Owner

@igrr igrr left a comment

Choose a reason for hiding this comment

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

Thanks a lot for sending this in. Sorry for not checking this earlier. The initial comment said "NOT tested on real device", so i assumed this was not ready yet.

The code for extracting authorityKeyIdentifier looks good. My only question is about ssl_match_auth_key_sha1, see below.

}

if(!x509_ctx->auth_key_sha1) {
x509_ctx->auth_key_sha1 = (uint8_t *)malloc(len);
Copy link
Owner

Choose a reason for hiding this comment

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

let's check if malloc fails here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

ssl/ssl.h Outdated
* @param fp [in] SHA1 hash to match against
* @return SSL_OK if the certificate is verified.
*/
EXP_FUNC int STDCALL ssl_match_auth_key_sha1(const SSL *ssl, const uint8_t* hash);
Copy link
Owner

Choose a reason for hiding this comment

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

I don't fully understand the usage of this function. Certificate received from the website contains an authorityKeyIdentifier, which you extract in asn1_auth_key_id function. The fact that a certificate contains valid authorityKeyIdentifier doesn't tell anything about authenticity of the certificate. So this function is not really useful for practical purposes of verifying the certificate.

The logic of certificate verification via authorityKeyIdentifier looks like this:

  1. get authorityKeyIdentifier from the certificate
  2. look up CAs RSA public key based on authorityKeyIdentifier (some map/list/etc of CA keyIdentifiers and public keys would be used in this step)
  3. if authorityKeyIdentifier was not found in the map, bail out
  4. if the authorityKeyIdentifier was found, use the associated public key to verify the certificate

Note that steps 1-4 happen on axTLS side; application has to provide axTLS with the map of {keyIdentifier => RSA public key} pairs. So i don't see much point in making this a public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... authorityKeyIdentifier doesn't tell anything about authenticity of the certificate.

That was the part that I was missing. I was, wrongly, thinking that the authorityKeyIdentifier is integral part of the certificate, set in stone during the signing and it will always contain the SHA1 from the CA public key and cannot be faked.

My idea with that function was to add a comparison based on the CA public key. Which should be valid for a long period of time (10 years for example). This way I don't need to rely on the SHA1 fingerprint of the certificate (which changes often (3 months ... 1 year), nor do I need to rely on the SHA256 of the public key of the certificate (which can change 1...2 years).

Ok, the asn1_auth_key_id extraction is working and can be used as a base for the improved verification process that you've suggested.

Copy link
Owner

@igrr igrr Jul 20, 2017

Choose a reason for hiding this comment

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

I was, wrongly, thinking that the authorityKeyIdentifier is integral part of the certificate, set in stone during the signing and it will always contain the SHA1 from the CA public key and cannot be faked.

No, it is indeed set in stone before the certificate is signed. But without checking the certificate's signature using CA's public key, you don't know whether it was indeed signed by CA (and as such, authorityKeyInfo is validated by the CA), or it is signed by an adversary trying to send you a fake certificate.

If you want to do comparison based on SHA256 of the public key of the certificate, you can use the existing ssl_match_spki_sha256 — SPKI will not change when the certificate is reissued. But that way is still tied to a single certificate (as in, single website).

Edit: read your comment again, now I understand that you have considered SPKI and its 1-2 years lifespan. Disregard the second part of my comment.

@slaff slaff force-pushed the feature/ca-key-fingerpint branch from db727ff to 8bf7ea7 Compare July 20, 2017 07:56
@igrr
Copy link
Owner

igrr commented Oct 7, 2017

Hi @slaff, can you make a version of the commit without the ssl_match_auth_key_sha1 function? As discussed above, asn1_auth_key_id is useful, and i would like to merge it and continue implementing the rest of the logic.

@slaff
Copy link
Contributor Author

slaff commented Oct 8, 2017

can you make a version of the commit without the ssl_match_auth_key_sha1 function

@igrr Yes, it will try to make the changes early next week so that you can build on top of them.

@slaff
Copy link
Contributor Author

slaff commented Oct 8, 2017

@igrr ssl_match_auth_key_sha1 is removed. If there are no other issues squash this PR and add the rest of the logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants