Menu ▾ ▴

#1471 CURLOPT_PINNEDPUBLICKEY documentation incomplete

closed-fixed
Daniel Stenberg
None
5
2015-02-17
2015-01-16
Kyle
No

The documentation for CURLOPT_PINNEDPUBLICKEY available at:

http://curl.haxx.se/libcurl/c/CURLOPT_PINNEDPUBLICKEY.html

Does not state what happens when
BOTH CURLOPT_SSL_VERIFYHOST == 0 AND CURLOPT_SSL_VERIFYPEER == 0.

This is bad because (at least vtls/openssl.c) ignores the pinned public
key (other than to emit a verbose mode message) when VERIFYHOST and VERIFYPEER
are off.

For example (using curl 7.40 built with OpenSSL):

cd /tmp
openssl genrsa | openssl rsa -pubout > dummykey.pem
curl -vI --pinnedpubkey dummykey.pem https://github.com/

This appears in the output:

* SSL: public key does not match pinned public key!
curl: (90) SSL: public key does not match pinned public key!

and curl's exit status is 90. However, if we repeat like so:

curl -vI -k --pinnedpubkey dummykey.pem https://github.com/

Then only this appears in the output:

* SSL: public key does not match pinned public key!

And curl's exit status is 0.

That is completely unexpected and not mentioned anywhere in the docs for CURLOPT_PINNEDPUBLICKEY, so either it's a bug or the docs are wrong. And while you might also want to VERIFYHOST when using a pinned public key, that shouldn't be required to use one.

Please take appropriate action. I'm inclined to believe it's a bug because the whole point of using pinned public keys is so that you can completely ignore trusted root certificates etc. in favor of a pinned public key.

Discussion

  • Daniel Stenberg

    Daniel Stenberg - 2015-01-16
    • status: open --> open-confirmed
     

  • Daniel Stenberg

    Daniel Stenberg - 2015-01-16

    Thanks for your report!

    In general we try to make options as independent from each other as possible and I don't see any reason for this option to depend on the other. I think you're right in that it can be made to work even with -k, so we should make it so. At least where possible - I'm stating it carefully because some TLS backend may make it hard.

     

  • Kyle

    Kyle - 2015-01-16

    In that case, here's a suggested fix for vtls/openssl.c:

    diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index a68d88ea..efbdc17f 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -2671,8 +2671,11 @@ static CURLcode ossl_connect_step3(struct connectdata *conn, int sockindex)
* operations.
*/

-  if(!data->set.ssl.verifypeer && !data->set.ssl.verifyhost)
-    (void)servercert(conn, connssl, FALSE);
+  if(!data->set.ssl.verifypeer && !data->set.ssl.verifyhost) {
+    CURLcode check = servercert(conn, connssl, FALSE);
+    if(check == CURLE_SSL_PINNEDPUBKEYNOTMATCH)
+      result = check;
+  }
   else
     result = servercert(conn, connssl, TRUE);

    Or something along those lines.

     

  • Daniel Stenberg

    Daniel Stenberg - 2015-01-19

    Thanks but it doesn't work exactly like that. servercert() would still skip the pin check when 'result' is non-zero.

    What do you think about the attached patch?

     
    0001-openssl-do-public-key-pinning-check-independently.patch

  • Kyle

    Kyle - 2015-01-19

    Yes, just tried that on top of 7.40.0. It seems to work very nicely. :)

     

  • Daniel Stenberg

    Daniel Stenberg - 2015-01-19
    • status: open-confirmed --> closed-fixed
     

  • Daniel Stenberg

    Daniel Stenberg - 2015-01-19

    Thanks for that, merged in commit be57f689b. Cased closed!