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

SecureTransport/DarwinSSL: Implement public key pinning #1400

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@moparisthebest
Contributor

moparisthebest commented Apr 8, 2017

This has been a long time coming, refer to these links for history:

https://twitter.com/bagder/status/849732891995250689
https://curl.haxx.se/mail/lib-2016-05/0015.html
https://stackoverflow.com/questions/36843572/public-key-bytes-from-seckeyref

I have tested this with 2048 and 4096 bit RSA keys, and a ecDSA Secp256r1 key with these commands respectively:

./src/curl https://www.google.com --insecure --pinnedpubkey 'sha256//U9GGUC1+PGI7SLb8XVVuuBPLkQKbQ2LnfU5Wfe7WAAg=' -s -S -o /dev/null && echo success
./src/curl https://www.moparisthebest.com --insecure --pinnedpubkey 'sha256//t62CeU2tQiqkexU74Gxa2eg7fRbEgoChTociMee9wno=' -s -S -o /dev/null && echo success
./src/curl https://blog.joelj.org --insecure --pinnedpubkey 'sha256//1cCBZSdgsO2/rHuPlfHELChj+wCD2C7X5DnbefNoY0o=' -s -S -o /dev/null && echo success

Strangely Apple's API returns full DER encoding for the ecDSA key, but missing the header part for RSA keys. So in theory this might work for all ecDSA keys (anyone know another site to test this with other curves?), but no other RSA keys.

I have no idea if the way I hard-coded the headers is decent, curl-like, or even valid C, but it works, looking forward to comments here.

I also stumbled upon another public key pinning implementation in Objective-C and it seems to do a similar thing:
https://github.com/datatheorem/TrustKit/blob/master/TrustKit/Pinning/public_key_utils.m

Either way, working for a subset of common keys seems better than not working at all which is the current situation. This should simply fail with a pinning error when faced with other keys. (which is fail-closed, the correct way)

Lastly I have only tested this on an OSX El Capitan box I had access to, testing on other versions and/or iOS would be appreciated. Thanks to everyone that already helped!

@mention-bot

This comment has been minimized.

mention-bot commented Apr 8, 2017

@moparisthebest, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @yangtse and @nickzman to be potential reviewers.

@bagder bagder added the SSL/TLS label Apr 8, 2017

@Lukasa

This comment has been minimized.

Contributor

Lukasa commented Apr 9, 2017

I'm happy to try to test this sometime soon, though my schedule is nasty for the next two weeks. No promises, but it's in my task tracker.

@Lukasa

So, a quick test reveals that this also works on Sierra, which is a good start!

A quick dive into the docs suggests that this won't work on iOS, and won't work on macOS prior to 10.7 (SecItemExport is not documented as available on those platforms, though a quick test with MACOSX_DEPLOYMENT_TARGET set to 10.6 didn't actually cause the build to fail).

On a personal level, I don't like the do...break...while(0) style. I'd much rather just use early returns, especially as there is no cleanup required for the early break statements. That said, this isn't my project and @bagder should do whatever he likes. ;)

Otherwise, I've left a few comments inline in the diff.

SecTrustRef trust;
OSStatus ret = SSLCopyPeerTrust(ctx, &trust);
if(ret != noErr)
break;

This comment has been minimized.

@Lukasa

Lukasa Apr 10, 2017

Contributor

Per the CoreFoundation copy rule, if we get past this break statement you need to CFRelease the trust variable at the end of the function. Right now you don't, so we leak the SecTrustRef and all its associated certificate and key objects.

OSStatus success = SecItemExport(keyRef, kSecFormatOpenSSL, 0, NULL,
&publicKeyBits);
if(success != errSecSuccess)
break;

This comment has been minimized.

@Lukasa

Lukasa Apr 10, 2017

Contributor

Again, per the copy rule, you need to CFRelease publicKeyBits when you're done with it: right now you don't.

}
realpubkeylen = pubkeylen + rsaHeaderLength;
realpubkey = malloc(realpubkeylen);

This comment has been minimized.

@Lukasa

Lukasa Apr 10, 2017

Contributor

We can avoid a malloc here by stack-allocating a buffer large enough to hold a 4096-bit RSA key. Whether that's a good idea or not is up to @bagder, but if we want to avoid the need to free this pointer it might help.

This comment has been minimized.

@bagder

bagder Apr 10, 2017

Member

As that is still less than 1,000 bytes I'd say it is fine to just keep it on the stack.

@@ -2573,6 +2661,15 @@ void Curl_darwinssl_md5sum(unsigned char *tmp, /* input */
(void)CC_MD5(tmp, (CC_LONG)tmplen, md5sum);
}
void Curl_darwinssl_sha256sum(unsigned char *tmp, /* input */
size_t tmplen,
unsigned char *sha25sum, /* output */

This comment has been minimized.

@Lukasa

Lukasa Apr 10, 2017

Contributor

This should maybe be called sha256sum?

This comment has been minimized.

@moparisthebest

moparisthebest Apr 11, 2017

Contributor

This follows the pattern of other backends where it's called Curl_backendname_sha256sum, which I modelled off the Curl_backendname_md5sum convention. I don't really care either way. :)

This comment has been minimized.

@Lukasa

Lukasa Apr 11, 2017

Contributor

Sorry, I didn't mean the function name, I meant the variable name.

This comment has been minimized.

@moparisthebest

moparisthebest Apr 11, 2017

Contributor

HA totally missed that one (multiple times), good catch :)

unsigned char *sha25sum, /* output */
size_t sha25len)
{
(void)sha25len;

This comment has been minimized.

@Lukasa

Lukasa Apr 10, 2017

Contributor

This is neither here nor there, but should we validate that the size of the buffer is actually large enough to hold a SHA256 digest? Using assert would likely be sufficient.

This comment has been minimized.

@moparisthebest

moparisthebest Apr 11, 2017

Contributor

Curl_darwinssl_md5sum does the same here, should both change, or neither?

This comment has been minimized.

@Lukasa

Lukasa Apr 11, 2017

Contributor

No idea, up to @bagder.

This comment has been minimized.

@bagder

bagder Apr 11, 2017

Member

I think this is a mistake waiting to happen. The length passed in should either be checked or a comment to be added why it doesn't have to (I'd prefer the check though, as it seems safer for when someone reorganizes this code in the far future). (In this sha256 case, it is only called from a single spot (right now) so we know which fixed size is passed in.)

This comment has been minimized.

@moparisthebest

moparisthebest Apr 11, 2017

Contributor

is a simple assert(sha256len >= 32); ok ? Also should I fix the md5 function in this same commit or a different commit later?

OSStatus ret = SSLCopyPeerTrust(ctx, &trust);
if(ret != noErr)
break;
SecKeyRef keyRef = SecTrustCopyPublicKey(trust);

This comment has been minimized.

@Lukasa

Lukasa Apr 10, 2017

Contributor

Again, per the copy rule, you need to CFRelease keyRef when you're done with it: right now you don't.

pubkeylen = CFDataGetLength(publicKeyBits);
pubkey = CFDataGetBytePtr(publicKeyBits);
if(pubkeylen == 526) {

This comment has been minimized.

@Lukasa

Lukasa Apr 10, 2017

Contributor

So, this method of working out the key type strikes me as being something that will come back to bite us. However, Apple doesn't really provide any other option: there is basically no way to work out what the key type of a SecKeyRef is that I can see.

@bagder

This comment has been minimized.

Member

bagder commented Apr 10, 2017

I have a slight preference for early returns if possible, but I really don't mind breaking out of a do/while(0) as is done in this commit.

@moparisthebest

This comment has been minimized.

Contributor

moparisthebest commented Apr 11, 2017

I pushed up fixes for the comments, as a seperate commit so the comments would stay around, I'm guessing you'll want me to squish these before you merge them.

I also added support for more versions, unfortunately the new code won't link on my 10.11 box, it's only supported on 10.12+, so I can't test it at runtime. But per the docs macOS 10.7+ and iOS 10+ should be supported. 10.7 - 10.11 by one bit of code, and the rest by a different set.

Lastly, lukasa said earlier in IRC:

You can use SecCertificateCopyValues along with kSecOIDX509V1SubjectPublicKeyAlgorithm and kSecOIDX509V1SubjectPublicKeyAlgorithmParameters to work out what key and key size you've got, I think

Which I think would be a much better solution, but unfortunately I think SecCertificateCopyValues is only macOS and not supported on iOS at all if I read this right:
https://developer.apple.com/reference/security/1396051-seccertificatecopyvalues
Any thoughts?

@Lukasa

This comment has been minimized.

Contributor

Lukasa commented Apr 11, 2017

Whoever knew my superpower was going to be "has access to a 10.12 macOS box"? So the build fails on Sierra:

vtls/darwinssl.c:2056:21: error: 'SecKeyCopyExternalRepresentation' is only
      available on macOS 10.12 or newer [-Werror,-Wunguarded-availability]
    publicKeyBits = SecKeyCopyExternalRepresentation(keyRef, NULL);
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This is because curl, by default, sets -mmacosx-version-min=10.8 in the makefile. (Incidentally, this is probably why setting MACOSX_DEPLOYMENT_TARGET wasn't actually working, as the command-line option overrides the environment variable.

Now, the reason you're seeing this problem is because the #define is asking the wrong question. You need to have MIN_REQUIRED as at least 10.12 to go into that branch, rather than having the max be higher than 10.12. In fact, all of your MAX_ALLOWEDs need to become MIN_REQUIREDs.

You have further #define problems too: your check for whether we have either V1 or V2 defined doesn't work and so the DARWIN_SSL_PINNEDPUBKEY #define is always present and truthy. That means you compile in a bunch of code that won't work (and, in fact, that segfaults pretty badly). Given that we use #ifdef everywhere, you can't do this:

#define DARWIN_SSL_PINNEDPUBKEY ((DARWIN_SSL_PINNEDPUBKEY_V1) || \
                                 (DARWIN_SSL_PINNEDPUBKEY_V2))

You have to do:

#if defined(DARWIN_SSL_PINNEDPUBKEY_V1) || defined(DARWIN_SSL_PINNEDPUBKEY_V2)
#define DARWIN_SSL_PINNEDPUBKEY 1
#endif

Next up, the version check for v2 is wrong: the version code for v10.7 is 1070, not 100700. Here is the relevant section of Availability.h:

#define __MAC_10_0            1000
#define __MAC_10_1            1010
#define __MAC_10_2            1020
#define __MAC_10_3            1030
#define __MAC_10_4            1040
#define __MAC_10_5            1050
#define __MAC_10_6            1060
#define __MAC_10_7            1070
#define __MAC_10_8            1080
#define __MAC_10_9            1090
#define __MAC_10_10         101000
#define __MAC_10_10_2       101002
#define __MAC_10_10_3       101003
#define __MAC_10_11         101100
#define __MAC_10_11_2       101102
#define __MAC_10_11_3       101103
#define __MAC_10_11_4       101104
#define __MAC_10_12         101200
#define __MAC_10_12_1       101201
#define __MAC_10_12_2       101202
#define __MAC_10_12_4       101204

(You're right not to use the symbolic names of these constants btw, I'm just showing them for reference.)

Once that stuff gets fixed up and curl is told to compile against the 10.12 SDK, the compile appears to pass and a quick test demonstrates that both branches of code function correctly.

@moparisthebest

This comment has been minimized.

Contributor

moparisthebest commented Apr 12, 2017

Thanks much for reviewing that again, hopefully one final patch.

So I managed to get access to a macOS Sierra box, and my fears were true. For RSA keys the new macOS/iOS code returns the same as the macOS 10.7+ code for RSA keys, but for ecDSA keys it returns them missing the header, while the 10.7+ code returns the full DER encoding. SecureTransport is such a mess of a library...

ANYWAY I tested all 4 supported key types with both sets of code, on a el capitan box, and a sierra box (with CFLAGS=-mmacosx-version-min=10.12), and these commands:

# rsa2048
./src/curl https://www.google.com --insecure --pinnedpubkey 'sha256//U9GGUC1+PGI7SLb8XVVuuBPLkQKbQ2LnfU5Wfe7WAAg=' -s -S -o /dev/null && echo success
# rsa4096
./src/curl https://www.moparisthebest.com --insecure --pinnedpubkey 'sha256//t62CeU2tQiqkexU74Gxa2eg7fRbEgoChTociMee9wno=' -s -S -o /dev/null && echo success
# ecDSA secp256r1
./src/curl https://blog.joelj.org --insecure --pinnedpubkey 'sha256//1cCBZSdgsO2/rHuPlfHELChj+wCD2C7X5DnbefNoY0o=' -s -S -o /dev/null && echo success
# ecDSA secp384r1
./src/curl https://suche.org --pinnedpubkey 'sha256//I3dI5BuM4PHlzYhqmtMf4OUPahIOIqIzP4rxjEFd3X0=' -s -S -o /dev/null && echo success

The last command can't use --insecure because that site requires SNI, and SecureTransport can't send SNI when certificate validation is disabled, did I mention how great SecureTransport is? :)

@moparisthebest

This comment has been minimized.

Contributor

moparisthebest commented Apr 12, 2017

I feel a bit more comfortable with only supporting those 4 key types now, turns out letsencrypt only supports those 2 curves for ECC keys:

https://github.com/letsencrypt/boulder/blob/master/goodkey/good_key.go#L172-L183

And only supports RSA keys from 2048 to 4096 (we wouldn't support anything in the middle, but whatever..).

https://github.com/letsencrypt/boulder/blob/master/goodkey/good_key.go#L194-L200

@Lukasa

Cool, looking pretty good. Some notes.

0x01, 0x06, 0x05, 0x2b, 0x81, 0x04,
0x00, 0x22, 0x03, 0x62, 0x00};
#endif /* DARWIN_SSL_PINNEDPUBKEY_V1 */
#endif /* DARWIN_SSL_PINNEDPUBKEY */

This comment has been minimized.

@Lukasa

Lukasa Apr 14, 2017

Contributor

So this is a little confusing: I don't think these are technically DER headers (DER doesn't have headers), they're SPKI headers for the appropriate key types. It may be worth changing the names, or at least the comments, to clarify.

This comment has been minimized.

@moparisthebest

moparisthebest Apr 17, 2017

Contributor

I'll change it to spkiHeaders then. :)

SecTrustRef tempTrust;
SecTrustCreateWithCertificates(cert, policy, &tempTrust);
SecTrustEvaluate(tempTrust, NULL);
keyRef = SecTrustCopyPublicKey(tempTrust);

This comment has been minimized.

@Lukasa

Lukasa Apr 14, 2017

Contributor

Wow, this is a lot of work to do the equivalent of calling SecTrustCopyPublicKey on the original trust object. Why, exactly, are we doing this?

This comment has been minimized.

@moparisthebest

moparisthebest Apr 17, 2017

Contributor

Seems to work without it, stripping that out.

* we assume rest of algorithms do same, so do nothing
*/
result = Curl_pin_peer_pubkey(data, pinnedpubkey, pubkey,
pubkeylen);

This comment has been minimized.

@Lukasa

Lukasa Apr 14, 2017

Contributor

This is supposed to break out of the do...while right?

This comment has been minimized.

@moparisthebest

moparisthebest Apr 17, 2017

Contributor

Yes, but I did it below with the if(derHeader == NULL) break; statement, I guess breaking on labels isn't supported in C89 ?

edit: Turns out continue works perfectly well there since it's a do{}while(0);

This comment has been minimized.

@Lukasa

Lukasa Apr 17, 2017

Contributor

Hah, yes, so while continue works it's definitely a bit confusing in this context. Still, that's @bagder's call.

This comment has been minimized.

@moparisthebest

moparisthebest Apr 17, 2017

Contributor

Yea hopefully the comment makes up for it, since it explains exactly what the intention is.

@moparisthebest

This comment has been minimized.

Contributor

moparisthebest commented Apr 17, 2017

I believe @Lukasa 's comments have been addressed with the latest commit, thanks again for reviewing it!

@Lukasa

Lukasa approved these changes Apr 17, 2017

Cool, this LGTM. Nice work!

@bagder

bagder approved these changes Apr 17, 2017

I just found some very minor nits. Apart from those I'm fine with merging. However I think we should wait and merge this after 7.54.0 (due to ship in ~36 hours) to take the safer route.

SecTrustRef trust;
OSStatus ret = SSLCopyPeerTrust(ctx, &trust);
if(ret != noErr || trust == NULL)
break;

This comment has been minimized.

@bagder

bagder Apr 17, 2017

Member

This, and the three following breaks all use 4-space indents instead of 2...

@nickzman

Thanks for your work. I didn't test this, but I see no obvious problems or regressions in the code.

@moparisthebest

This comment has been minimized.

Contributor

moparisthebest commented Apr 18, 2017

I fully agree this should wait until 7.55.0 and have updated the patch to fix bagder's nits and change the supported version number in the documentation.

@bagder bagder closed this in eb16305 May 15, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.