SChannel/WinSSL: Implement public key pinning #1429
Conversation
|
@moparisthebest, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @yangtse and @mback2k to be potential reviewers. |
|
Hey @mback2k, we could use your keen eyes and area expertise here... |
|
Hey @bagder, I will try to take a look this weekend. I don't have much time at the moment and I am drowning in roughly 3000 GitHub notifications and 1000 mailinglist mails. If something like this comes up, could you please send me a direct message? Thanks. :-) |
| @@ -1576,6 +1610,159 @@ CURLcode Curl_schannel_random(unsigned char *entropy, size_t length) | |||
| return CURLE_OK; | |||
| } | |||
|
|
|||
| static CURLcode pkp_pin_peer_pubkey(struct connectdata *conn, int sockindex, | |||
There was a problem hiding this comment.
I would recommend the same as in #1325 (comment) and would like to see such a function moved to a separate source file, e.g. schannel_cert.c.
lib/vtls/schannel.c
Outdated
| not sure what second arg should be, | ||
| X509_PUBLIC_KEY_INFO -- fails | ||
| RSA_CSP_PUBLICKEYBLOB -- not quite der, missing spki header bytes | ||
| CNG_RSA_PUBLIC_KEY_BLOB -- fails |
There was a problem hiding this comment.
I will take a look into this. You may also find some answers in my WinCNG-based implementation for libssh2. CNG is the successor of crypt32 and requires the use of the bcrypt library. See: https://github.com/libssh2/libssh2/blob/master/src/wincng.c
There was a problem hiding this comment.
Will look thank you
| @@ -1716,4 +1903,69 @@ static CURLcode verify_certificate(struct connectdata *conn, int sockindex) | |||
| } | |||
| #endif /* _WIN32_WCE */ | |||
|
|
|||
| static void Curl_schannel_checksum(const unsigned char *input, | |||
There was a problem hiding this comment.
I would advise to have a return value that tells the user of this function if the checksum failed or not.
There was a problem hiding this comment.
I guess it can't? It's used to define curlssl_sha256sum (and later curlssl_md5sum), and that function doesn't have a return value. schannel appears to be the only backend where it even has a possibility of failure, which might be why it wasn't implemented before...
|
@moparisthebest I see that this PR's changes conflict with my PR to configure TLS backends at runtime (which was just merged to Would you like me to assist you in resolving the merge conflicts? |
|
@dscho Thanks for the offer and that amazing runtime TLS switching work. I can probably handle the merge conflicts myself whenever I get around to spinning windows back up and cleaning this up to be merged. I'm still unable to support anything but RSA keys with bit length of 2048 or 4096, but unless someone who knows what they are doing with schannel comes along, I'll probably just submit it as-is. It's better than nothing anyway. :) |
Cool!
As you say, it is better than nothing! And if anybody comes along with the need for different sizes, then that will give them a perfect opportunity to "pay back" to the project ;-) |
moparisthebest
force-pushed
the
schannel-pinnedpubkey
branch
from
1d49e25
to
8a87887
Compare
moparisthebest
force-pushed
the
schannel-pinnedpubkey
branch
from
8a87887
to
f0f3b8c
Compare
moparisthebest
changed the title
|
In October @boris9000 (thanks!) put in a pull request to fix this patch to support all types of certificates and keys. I finally got another windows dev box spun up and was able to rebase my and his commit on top of master, including the new runtime TLS backend support. I have tested it with rsa4096 and ecDSA Secp256r1 keys like so: Additionally I added another not-really-needed commit to replace Curl_none_md5sum with Curl_schannel_md5sum, if this isn't desired I can pull it. If @dscho wouldn't mind giving it a quick once-over to make sure I didn't do anything to harm the runtime TLS stuff, and @mback2k wouldn't mind making sure the windows specific code looks OK then I believe it is ready to merge. Note: I have only tested this on Windows 7 SP1 x86, and don't really have the ability to run it on anything else. |
|
thanks! |
This implements public key pinning for the schannel backend. Currently it works with pem/der/hashes, but only if the remote server has a RSA key with a length of 2048 or 4096 bits. I'm looking for help with this error that occurs with ecDSA keys (regardless of PROV_RSA_FULL or PROV_RSA_AES):
moparisthebest@1d49e25#diff-2ba9d5898aad590d89cd83e17ad244fbR1658
And also hoping that I don't really have to resort to the horrible hard-coded header hacks that the apple backend had to do:
moparisthebest@1d49e25#diff-2ba9d5898aad590d89cd83e17ad244fbR1719
Is there a proper way to change a PUBLICKEYBLOB to a DER (or a PEM that I can convert)?
Also thoughts about this working on XP, 8, or 10 would be appreciated too, I've only tested on 7 SP1.