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

nss: fix build with disabled proxy support #5667

Closed
wants to merge 1 commit into from

Conversation

@baruchsiach
Copy link
Contributor

baruchsiach commented Jul 9, 2020

Avoid reference to fields that do not exist when CURL_DISABLE_PROXY is
defined.

Avoid reference to fields that do not exist when CURL_DISABLE_PROXY is
defined.
@kdudka
Copy link
Collaborator

kdudka commented Jul 9, 2020

I wonder if we could introduce additional macros here to avoid the excessive use of #ifndef all over the code: https://github.com/curl/curl/blob/963d17ad/lib/vtls/vtls.h#L136

@bagder
Copy link
Member

bagder commented Jul 9, 2020

That would be nice, but do you have any idea on how we could do it?

@kdudka
Copy link
Collaborator

kdudka commented Jul 10, 2020

Something like this? The patch applies on top of this PR but I can submit it as a separate PR on top of (or bellow) this PR if you like it:

--- a/lib/vtls/nss.c
+++ b/lib/vtls/nss.c
@@ -1027,12 +1027,7 @@ static SECStatus BadCertHandler(void *arg, PRFileDesc *sock)
   CERTCertificate *cert;
 
   /* remember the cert verification result */
-#ifndef CURL_DISABLE_PROXY
-  if(SSL_IS_PROXY())
-    data->set.proxy_ssl.certverifyresult = err;
-  else
-#endif
-    data->set.ssl.certverifyresult = err;
+  SSL_SET_OPTION_LVALUE(certverifyresult) = err;
 
   if(err == SSL_ERROR_BAD_CERT_DOMAIN && !SSL_CONN_CONFIG(verifyhost))
     /* we are asked not to verify the host name */
@@ -1838,12 +1833,6 @@ static CURLcode nss_setup_connect(struct connectdata *conn, int sockindex)
   CURLcode result;
   bool second_layer = FALSE;
   SSLVersionRange sslver_supported;
-#ifndef CURL_DISABLE_PROXY
-  const char *hostname = SSL_IS_PROXY() ? conn->http_proxy.host.name :
-    conn->host.name;
-#else
-  const char *hostname = conn->host.name;
-#endif
 
   SSLVersionRange sslver = {
     SSL_LIBRARY_VERSION_TLS_1_0,  /* min */
@@ -1948,12 +1937,7 @@ static CURLcode nss_setup_connect(struct connectdata *conn, int sockindex)
     goto error;
 
   /* not checked yet */
-#ifndef CURL_DISABLE_PROXY
-  if(SSL_IS_PROXY())
-    data->set.proxy_ssl.certverifyresult = 0;
-  else
-#endif
-    data->set.ssl.certverifyresult = 0;
+  SSL_SET_OPTION_LVALUE(certverifyresult) = 0;
 
   if(SSL_BadCertHook(model, BadCertHandler, conn) != SECSuccess)
     goto error;
@@ -2124,11 +2108,11 @@ static CURLcode nss_setup_connect(struct connectdata *conn, int sockindex)
     goto error;
 
   /* propagate hostname to the TLS layer */
-  if(SSL_SetURL(backend->handle, hostname) != SECSuccess)
+  if(SSL_SetURL(backend->handle, SSL_HOST_NAME()) != SECSuccess)
     goto error;
 
   /* prevent NSS from re-using the session for a different hostname */
-  if(SSL_SetSockPeerID(backend->handle, hostname) != SECSuccess)
+  if(SSL_SetSockPeerID(backend->handle, SSL_HOST_NAME()) != SECSuccess)
     goto error;
 
   return CURLE_OK;
@@ -2147,18 +2131,6 @@ static CURLcode nss_do_connect(struct connectdata *conn, int sockindex)
   struct Curl_easy *data = conn->data;
   CURLcode result = CURLE_SSL_CONNECT_ERROR;
   PRUint32 timeout;
-#ifndef CURL_DISABLE_PROXY
-  long * const certverifyresult = SSL_IS_PROXY() ?
-    &data->set.proxy_ssl.certverifyresult : &data->set.ssl.certverifyresult;
-  const char * const pinnedpubkey = SSL_IS_PROXY() ?
-              data->set.str[STRING_SSL_PINNEDPUBLICKEY_PROXY] :
-              data->set.str[STRING_SSL_PINNEDPUBLICKEY_ORIG];
-#else
-  long * const certverifyresult = &data->set.ssl.certverifyresult;
-  const char * const pinnedpubkey =
-              data->set.str[STRING_SSL_PINNEDPUBLICKEY_ORIG];
-#endif
-
 
   /* check timeout situation */
   const timediff_t time_left = Curl_timeleft(data, NULL, TRUE);
@@ -2174,9 +2146,9 @@ static CURLcode nss_do_connect(struct connectdata *conn, int sockindex)
     if(PR_GetError() == PR_WOULD_BLOCK_ERROR)
       /* blocking direction is updated by nss_update_connecting_state() */
       return CURLE_AGAIN;
-    else if(*certverifyresult == SSL_ERROR_BAD_CERT_DOMAIN)
+    else if(SSL_SET_OPTION(certverifyresult) == SSL_ERROR_BAD_CERT_DOMAIN)
       result = CURLE_PEER_FAILED_VERIFICATION;
-    else if(*certverifyresult != 0)
+    else if(SSL_SET_OPTION(certverifyresult) != 0)
       result = CURLE_PEER_FAILED_VERIFICATION;
     goto error;
   }
@@ -2204,7 +2176,7 @@ static CURLcode nss_do_connect(struct connectdata *conn, int sockindex)
     }
   }
 
-  result = cmp_peer_pubkey(connssl, pinnedpubkey);
+  result = cmp_peer_pubkey(connssl, SSL_PINNED_PUB_KEY());
   if(result)
     /* status already printed */
     goto error;
diff --git a/lib/vtls/vtls.h b/lib/vtls/vtls.h
index bcc844416..f4a67f39e 100644
--- a/lib/vtls/vtls.h
+++ b/lib/vtls/vtls.h
@@ -131,12 +131,23 @@ CURLcode Curl_none_md5sum(unsigned char *input, size_t inputlen,
                    CURL_SOCKET_BAD ? FIRSTSOCKET : SECONDARYSOCKET].state)
 #define SSL_SET_OPTION(var)                                             \
   (SSL_IS_PROXY() ? data->set.proxy_ssl.var : data->set.ssl.var)
+#define SSL_SET_OPTION_LVALUE(var)                                      \
+  (*(SSL_IS_PROXY() ? &data->set.proxy_ssl.var : &data->set.ssl.var))
 #define SSL_CONN_CONFIG(var)                                            \
   (SSL_IS_PROXY() ? conn->proxy_ssl_config.var : conn->ssl_config.var)
+#define SSL_HOST_NAME()                                                 \
+  (SSL_IS_PROXY() ? conn->http_proxy.host.name : conn->host.name)
+#define SSL_PINNED_PUB_KEY() (SSL_IS_PROXY()                            \
+  ? data->set.str[STRING_SSL_PINNEDPUBLICKEY_PROXY]                     \
+  : data->set.str[STRING_SSL_PINNEDPUBLICKEY_ORIG])
 #else
 #define SSL_IS_PROXY() FALSE
 #define SSL_SET_OPTION(var) data->set.ssl.var
+#define SSL_SET_OPTION_LVALUE(var) data->set.ssl.var
 #define SSL_CONN_CONFIG(var) conn->ssl_config.var
+#define SSL_HOST_NAME() conn->host.name
+#define SSL_PINNED_PUB_KEY()                                            \
+  data->set.str[STRING_SSL_PINNEDPUBLICKEY_ORIG]
 #endif
 
 bool Curl_ssl_config_matches(struct ssl_primary_config *data,
@bagder
Copy link
Member

bagder commented Jul 12, 2020

@kdudka I think it'll be better if you do it as a separate PR. Also, take a look and see if the same macros/style can be used for the other backends, as it would certainly be cool if we could apply the same logic on them all.

@kdudka
Copy link
Collaborator

kdudka commented Jul 13, 2020

So should I wait till this and the related pull requests are merged, or open a separate pull request against master now and let others deal with all the conflicts?

As for the other backends, I am pretty sure that it will simplify their code as well. But I will only patch the backends which I am able to locally compile curl against.

@baruchsiach
Copy link
Contributor Author

baruchsiach commented Jul 13, 2020

As for the other backends, I am pretty sure that it will simplify their code as well. But I will only patch the backends which I am able to locally compile curl against.

You can use Buildroot to build curl against all it supported backends, one backend at a time. These currently include openssl, bearssl, gnutls, nss, mbedtls, and wolfssl.
To apply a patch to curl before build just put your .patch file under package/libcurl/.

@kdudka
Copy link
Collaborator

kdudka commented Jul 13, 2020

@baruchsiach How long is the edit-compile-test loop with Buildroot? Could you please point me to any tutorial that describes how to quickly check with Buildroot that my curl patch works (or at least compiles)?

@baruchsiach
Copy link
Contributor Author

baruchsiach commented Jul 13, 2020

@baruchsiach How long is the edit-compile-test loop with Buildroot? Could you please point me to any tutorial that describes how to quickly check with Buildroot that my curl patch works (or at least compiles)?

Here is a quick howto build curl for the ARM64 target. I'm using a pre-built toolchain here to save built time.

git clone git://git.buildroot.net/buildroot
cd buildroot
cp support/config-fragments/autobuild/arm-aarch64.config .config
make menuconfig
Go to "Target packages" -> "Libraries" -> "Crypto"
Enable all relevant crypto libraries
Go to "Target packages" -> "Libraries" -> "Networking"
Enable "libcurl"
Select the crypto backend under the "SSL/TLS library to use" menu
Exit and save
Copy your curl patches to package/libcurl/
make libcurl

To build against a different backend do make menuconfig again, and select your target backend under the libcurl "SSL/TLS library to use" menu.

Buildroot can also build a bootable image with qemu to boot it in an emulated environment. A terse readme is at board/qemu/aarch64-virt/readme.txt based on the qemu_aarch64_virt_defconfig. I have not tested that, though.

@bagder
Copy link
Member

bagder commented Jul 14, 2020

should I wait till this and the related pull requests are merged

That is probably the easiest. I'll merge this now and you can create a separate PR for your work. I can then chip in on a few TLS backends I can build.

@bagder bagder closed this in 2f5d0e4 Jul 14, 2020
@kdudka
Copy link
Collaborator

kdudka commented Jul 27, 2020

@baruchsiach Thanks for the howto. It looks like an interesting project but it would be overkill for my needs. I do not really need to cross-compile anything. After typing make libcurl it started to build GNU tar from sources, which I believe is not needed for my task.

@kdudka
Copy link
Collaborator

kdudka commented Jul 27, 2020

@bagder I have submitted pull request #5735 for this.

@baruchsiach
Copy link
Contributor Author

baruchsiach commented Jul 27, 2020

@baruchsiach Thanks for the howto. It looks like an interesting project but it would be overkill for my needs. I do not really need to cross-compile anything. After typing make libcurl it started to build GNU tar from sources, which I believe is not needed for my task.

Buildroot builds host tar because of unfortunate effect of a (not so) recent tar bugfix on archives reproducibility:

https://git.buildroot.net/buildroot/commit/?id=b8fa273d500b44153e9939f0a100e97db2ff63ed
http://lists.busybox.net/pipermail/buildroot/2018-January/211222.html

Buildroot depends on tar generating reproducible archive to verify hash of non-tar source trees. I have a locally built tar version 1.29 in my PATH to save Buildroot build time.

The advantage of Buildroot in this case is the ease of build against various crypto backends.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.