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

Closed
wants to merge 5 commits into
from

Projects

None yet

3 participants

@sithglan
Contributor
  • Switch from verifying a pinned public key in a callback to right after
    the certificate verification.
@jay jay mbedtls: Fix pinned key return value on fail. draft1
- Switch from verifying a pinned public key in a callback to right after
the certificate verification.
fe28922
@mention-bot

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

@sithglan
Contributor

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
@jay jay mbedtls: Fix pinned key return value on fail. draft2
- 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.
94ff5e6
@jay
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.

jay added some commits Jan 18, 2016
@jay jay mbedtls: Fix pinned key return value on fail. draft3
- 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.
5972e52
@jay jay Merge branch 'master' into fix_mbedtls_pinned_key
5852389
@jay jay Merge branch 'master' into fix_mbedtls_pinned_key
4f58af3
@jay jay added a commit that referenced this pull request Jan 18, 2016
@jay jay mbedtls: Fix pinned key return value on fail
- 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
d58ba66
@jay
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
Contributor
@jay jay deleted the jay:fix_mbedtls_pinned_key branch Feb 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment