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

mbedtls: Fix pinned key return value on fail. draft1 #601

Closed
wants to merge 5 commits into from

Conversation

sithglan
Copy link
Contributor

  • Switch from verifying a pinned public key in a callback to right after
    the certificate verification.

- Switch from verifying a pinned public key in a callback to right after
the certificate verification.
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @sasq64, @jay and @bagder to be potential reviewers

@sithglan
Copy link
Contributor Author

This works for me. And it fixes the problem with checking the cert after resumption and makes it possible to disable verifypeer by also using certificate pinning. I tested it since it was created in my binary and had no issues with it.

@jay jay self-assigned this Jan 13, 2016
- A copy of the const peercert must be made in order to pass a non-const
public key to mbedtls_pk_write_pubkey_der.

FAIL: Currently this runs but all pinned key verification fails. I have
yet to debug it.
@jay
Copy link
Member

jay commented Jan 15, 2016

Before this goes in it must be fixed so that the input for mbedtls_pk_write_pubkey_der will be a non-const key instead of const. I just finished a second draft where I wrote what I thought would be the correct code to do that however I likely missed something because it does not work properly. All pinned verification fails and I've yet to debug. You can find the latest changes in the draft branch for this issue. I'll take another look soon.

- More correct cleanup pattern for the copy of peer cert *p.

- Add some descriptive error messages where appropriate.

- Change name ret => result to eliminate variable shadowing warning.

- Remove comment about no peer cert on session resume, since that's not
true of mbedTLS (the comment was carried over from PolarSSL code).

- Allocate cert info buffer on heap instead of stack.


Prior to these changes: In draft 2 the code worked fine, however I
marked it as 'FAIL' because all my tests failed due to what I later
realized was just a copy&paste error on my part: I hadn't enabled
curlssl_sha256sum even though I was running the sha256// tests.
jay added a commit that referenced this pull request Jan 18, 2016
- Switch from verifying a pinned public key in a callback during the
certificate verification to inline after the certificate verification.

The callback method had three problems:

1. If a pinned public key didn't match, CURLE_SSL_PINNEDPUBKEYNOTMATCH
was not returned.

2. If peer certificate verification was disabled the pinned key
verification did not take place as it should.

3. (related to #2) If there was no certificate of depth 0 the callback
would not have checked the pinned public key.

Though all those problems could have been fixed it would have made the
code more complex. Instead we now verify inline after the certificate
verification in mbedtls_connect_step2.

Ref: http://curl.haxx.se/mail/lib-2016-01/0047.html
Ref: #601
@jay
Copy link
Member

jay commented Jan 18, 2016

@sithglan This landed in d58ba66, thanks for your help. All the SSLpinning tests pass now.
runtests.pl 2034 2035 2037 2038 2041 2042 2048

test 2034...[simple HTTPS GET with DER public key pinning]
-pd---e--- OK (1   out of 7  , remaining: 00:30)
test 2035...[HTTPS wrong DER pinnedpubkey but right CN]
------e--- OK (2   out of 7  , remaining: 00:14)
test 2037...[simple HTTPS GET with PEM public key pinning]
-pd---e--- OK (3   out of 7  , remaining: 00:08)
test 2038...[HTTPS wrong PEM pinnedpubkey but right CN]
------e--- OK (4   out of 7  , remaining: 00:04)
test 2041...[simple HTTPS GET with base64-sha256 public key pinning]
-pd---e--- OK (5   out of 7  , remaining: 00:02)
test 2042...[HTTPS wrong base64-sha256 pinnedpubkey but right CN]
------e--- OK (6   out of 7  , remaining: 00:01)
Warning: test2048 not present in tests/data/Makefile.inc
test 2048...[pinnedpubkey no-match must fail even when insecure]
------e--- OK (7   out of 7  , remaining: 00:00)
TESTDONE: 7 tests out of 7 reported OK: 100%
TESTDONE: 7 tests were considered during 8 seconds.
curl 7.47.0-DEV (i686-w64-mingw32) libcurl/7.47.0-DEV mbedTLS/2.2.1 zlib/1.2.8 libidn/1.32 libssh2/1.6.0 nghttp2/1.6.0
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp scp sftp smtp smtps telnet tftp
Features: AsynchDNS Debug TrackMemory IDN IPv6 Largefile SSPI Kerberos SPNEGO NTLM SSL libz HTTP2

@jay jay closed this Jan 18, 2016
@sithglan
Copy link
Contributor Author

sithglan commented Jan 18, 2016 via email

@jay jay deleted the fix_mbedtls_pinned_key branch February 15, 2016 05:00
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants