ZitchCode / SecureHttpClient

Cross-platform HttpClientHandler with TLS1.2 and Certificate Pinning
MIT License
33 stars 6 forks source link

Removing Bouncy Castle #1

Closed hlogmans closed 7 years ago

hlogmans commented 7 years ago

Hi,

I was wondering why Bouncy Castle is a dependency, as this code also extracts the public key from a certificate? (SpkiFingerprint.Compute)

        var cert = new X509Certificate2(certificate);
        var spkiDer = cert.GetPublicKey();

It will return the public key structure as binary data (DER), or is something missing?

This code works with Xamarin.

Best regards,

Hugo

tranb3r commented 7 years ago

My code extracts the SPKI (which requires the ASN1 structure, hence BouncyCastle), while your code extracts the PublicKey, and it's not exactly the same thing. More details here and here

hlogmans commented 7 years ago

Thank you for these details. Did you choose this specific variant because you need this structure in the Android platform? On IOS it seems the project code is validating the pin instead of the platform. Taking away the Bouncy Castle dependency (which causes some version conflicts) by also validating the public key structure on Android could make the code more independent. Taking the GetPublicKey data as base for the hash is not less secure, or am I wrong? But introducing a new hash could be quite a hassle for future updates.

tranb3r commented 7 years ago

I think that pinning the PublicKey is theoretically less secure than pinning the SPKI, because one could theoretically forge a SPKI with the same PublicKey hash but a different algorithm. Pinning the SPKI prevents this from happenning. I think I'm doing exactly the same thing on Android and iOS, so I do not understand your comment. One more thing : the SPKI fingerprint can be easily obtained by executing this command (assuming cert.pem is your certificate): "openssl x509 -noout -in cert.pem -pubkey | openssl rsa -pubin -outform der | openssl dgst -sha256 -binary | base64"

Hope this helps tranb3r

hlogmans commented 7 years ago

In Android, you provide the pins by setting the CertificatePinner for OkHttp3. In iOS and Portable, the "check" method of the certificate pinner is called. So for the pins only the Android versions has an external structure dependency (as defined in the OKHttp3 documentation). The ModernHttpClient code still also calls the generic .net callback used for the Portable code. That's why it could be easily converted.

About the GetPublicKey method: it is true that if GetPublicKey only gets the public key, and not the full PublicKeyStructure including the algorithm, it is less safe. But I did some examination of the result some time ago, and afaik it delivers the full public key structure including algorithm and parameters. If this is not true, only DSA is also allowed for X509Certificate2 objects, it is not possible to use some other faked algorithm that accepts a generated public key, that will generate an exception.

https://msdn.microsoft.com/en-us/library/system.security.cryptography.x509certificates.x509certificate2.publickey(v=vs.110).aspx

(note that the docs on GetPublicKey do not state what exactly is returned)

Thanks for your time, it is very important to be absolutely clear about the inner workings, that will help others to trust this too.

hlogmans commented 7 years ago

Update: the GetPublicKey method only returns the public key and exponent (270 bytes) and leaves out the oid. So, to complete the check and check for the correct algrithm, something like "cert.PublicKey.Oid.FriendlyName == "RSA"; has to be done, or the Oid/name has to be included in the name of the hash...

tranb3r commented 7 years ago

Maybe I should clarify once again: