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

SChannel/WinSSL: Implement public key pinning #1429

Closed

Conversation

Projects
None yet
5 participants
@moparisthebest
Copy link
Contributor

commented Apr 19, 2017

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.

@mention-bot

This comment has been minimized.

Copy link

commented Apr 19, 2017

@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.

@bagder bagder requested a review from mback2k Apr 19, 2017

@bagder

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

Hey @mback2k, we could use your keen eyes and area expertise here...

@mback2k

This comment has been minimized.

Copy link
Member

commented Apr 21, 2017

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. :-)

@mback2k
Copy link
Member

left a comment

Thanks for your hard work on this feature and pull-request, I really appreciate it. I hope that you will find the time to address my review comments, 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,

This comment has been minimized.

Copy link
@mback2k

mback2k Apr 23, 2017

Member

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.



if(!CryptAcquireContext(&hCryptProv, NULL, NULL,
/*PROV_RSA_FULL,*/

This comment has been minimized.

Copy link
@mback2k

mback2k Apr 23, 2017

Member

Please remove such comments if the code is no longer needed for the final version, or at least document why it is currently deactivated.

This comment has been minimized.

Copy link
@moparisthebest

moparisthebest Apr 24, 2017

Author Contributor

Sure I was just trying to show what I had tried with regard to ecdsa certificates failing at the next step, they fail for both of those values.

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

This comment has been minimized.

Copy link
@mback2k

mback2k Apr 23, 2017

Member

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

This comment has been minimized.

Copy link
@moparisthebest

moparisthebest Apr 24, 2017

Author Contributor

Will look thank you

}

FILE *fp;
fp=fopen("windows.key", "wb");

This comment has been minimized.

Copy link
@mback2k

mback2k Apr 23, 2017

Member

I guess this is some debug code which should be removed for the final version.

This comment has been minimized.

Copy link
@moparisthebest

moparisthebest Apr 24, 2017

Author Contributor

Yep again just debug code so it's easy to compare what we got out of windows with what we expect with tools like xxd

memcpy(realpubkey, spkiHeader, spkiHeaderLength);
memcpy(realpubkey + spkiHeaderLength, pubkey, pubkeylen);

fp=fopen("windows.real.key", "wb");

This comment has been minimized.

Copy link
@mback2k

mback2k Apr 23, 2017

Member

I guess this is some debug code which should be removed for the final version.

This comment has been minimized.

Copy link
@moparisthebest

moparisthebest Apr 24, 2017

Author Contributor

yep

@@ -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,

This comment has been minimized.

Copy link
@mback2k

mback2k Apr 23, 2017

Member

I would advise to have a return value that tells the user of this function if the checksum failed or not.

This comment has been minimized.

Copy link
@moparisthebest

moparisthebest Apr 24, 2017

Author Contributor

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...

@dscho

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2017

@moparisthebest I see that this PR's changes conflict with my PR to configure TLS backends at runtime (which was just merged to master).

Would you like me to assist you in resolving the merge conflicts?

@moparisthebest

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2017

@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. :)

@dscho

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2017

I can probably handle the merge conflicts myself

Cool!

I'm still unable to support anything but RSA keys with bit length of 2048 or 4096

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 moparisthebest force-pushed the moparisthebest:schannel-pinnedpubkey branch from 1d49e25 to 8a87887 Jan 16, 2018

@moparisthebest moparisthebest force-pushed the moparisthebest:schannel-pinnedpubkey branch from 8a87887 to f0f3b8c Jan 16, 2018

@moparisthebest moparisthebest changed the title WIP: SChannel/WinSSL: Implement public key pinning SChannel/WinSSL: Implement public key pinning Jan 16, 2018

@moparisthebest

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2018

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:

curl.exe https://www.moparisthebest.com --insecure --pinnedpubkey 'sha256//t62CeU2tQiqkexU74Gxa2eg7fRbEgoChTociMee9wno=' -s -S -o test.out.txt
curl.exe https://blog.joelj.org --insecure --pinnedpubkey 'sha256//1cCBZSdgsO2/rHuPlfHELChj+wCD2C7X5DnbefNoY0o=' -s -S -o test.out.txt

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.

@bagder

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

thanks!

@bagder bagder closed this in e178fbd Jan 25, 2018

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.