-
Notifications
You must be signed in to change notification settings - Fork 33
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
base: master
Are you sure you want to change the base?
Conversation
64132da
to
db727ff
Compare
@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. |
There was a problem hiding this 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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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:
- get authorityKeyIdentifier from the certificate
- 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)
- if authorityKeyIdentifier was not found in the map, bail out
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Tested on real device.
db727ff
to
8bf7ea7
Compare
Hi @slaff, can you make a version of the commit without the |
@igrr Yes, it will try to make the changes early next week so that you can build on top of them. |
@igrr |
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.