GH-33: Added /SECURITY weak|strong option and tests

negrutiu · Aug 31, 2024 · 2386538 · 2386538
1 parent 8ae4bb8
commit 2386538
Show file tree

Hide file tree

Showing 5 changed files with 244 additions and 57 deletions.
diff --git a/src/nscurl/NScurl.readme.md b/src/nscurl/NScurl.readme.md
@@ -433,6 +433,22 @@ NScurl::http GET "https://self-signed.badssl.com" "${file}" /CERT '${BADSSL_SELF
 Pop $0
 ```
 
+### /SECURITY
+```
+/SECURITY weak|strong
+```
+Configure the security level for the current transfer.  
+The default security is `weak` to favor compatibility with legacy servers.
+
+Security level `weak`:
+- call [SSL_CTX_set_options(..., SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION);](https://docs.openssl.org/3.1/man3/SSL_CTX_set_options/#notes) to enable unsafe legacy renegociation
+- call [SSL_CTX_set_security_level( 0 )](https://docs.openssl.org/1.1.1/man3/SSL_CTX_set_security_level/#default-callback-behaviour) to enable weak cryptographic algorithms
+- call [curl_easy_setopt(..., CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1);](https://curl.se/libcurl/c/CURLOPT_SSLVERSION.html) to enable `SSL3`, `TLS 1.0` and `TLS 1.1` protocols
+
+Security level `strong`:
+- use the default `openssl` crypto algorithms and standards that are considered secure
+
+
 ### /DEPEND
 ```
 /DEPEND id

diff --git a/src/nscurl/curl.c b/src/nscurl/curl.c
@@ -456,6 +456,16 @@ ULONG CurlParseRequestParam( _In_ ULONG iParamIndex, _In_ LPTSTR pszParam, _In_
 			if (popstring( pszParam ) == NOERROR)
 				pReq->pszTlsPass = MyStrDup( eT2A, pszParam );
 		}
+	} else if (lstrcmpi( pszParam, _T( "/SECURITY" ) ) == 0) {
+		if (popstring( pszParam ) == NOERROR) {
+			if (lstrcmpi(pszParam, _T("weak")) == 0) {
+				pReq->bStrongSecurity = FALSE;
+			} else if (lstrcmpi(pszParam, _T("strong")) == 0) {
+				pReq->bStrongSecurity = TRUE;
+			} else {
+				err = ERROR_INVALID_PARAMETER;
+			}
+		}
 	} else if (lstrcmpi( pszParam, _T( "/CACERT" ) ) == 0) {
 		if (popstring( pszParam ) == NOERROR) {						/// pszParam may be empty ("")
 			if (lstrcmpi(pszParam, _T("builtin")) == 0) {
@@ -639,7 +649,13 @@ CURLcode CurlSSLCallback( CURL *curl, void *ssl_ctx, void * userdata)
 		SSL_CTX_set_options(sslctx, flags);
 	}
 
-    // Add `/CERT pem` certificates to the SSL_CTX store
+	// https://docs.openssl.org/1.1.1/man3/SSL_CTX_set_security_level/#default-callback-behaviour
+	if (!req->bStrongSecurity)
+	{
+		SSL_CTX_set_security_level(sslctx, 0);
+	}
+
+	// Add `/CERT pem` certificates to the SSL_CTX store
 	const struct curl_slist* pem;
 	for (pem = req->pPemList; pem; pem = pem->next)
 	{
@@ -1140,9 +1156,16 @@ void CurlTransfer( _In_ PCURL_REQUEST pReq )
 				curl_easy_setopt( curl, CURLOPT_SSL_VERIFYHOST, FALSE );
 			}
 
-			// GH-31: allow "unsafe legacy renegotiation"
-			// Symptomatic URL: https://publicinfobanjir.water.gov.my/hujan/data-hujan/?state=PLS&lang=en
-			pReq->opensslSetFlags |= SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION;
+			// Security level
+			if (!pReq->bStrongSecurity)
+			{
+				// GH-31: allow "unsafe legacy renegotiation"
+				// Symptomatic URL: https://publicinfobanjir.water.gov.my/hujan/data-hujan/?state=PLS&lang=en
+				pReq->opensslSetFlags |= SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION;
+
+				// Allow TLS 1.0, TLS 1.1
+				curl_easy_setopt(curl, CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_0);
+			}
 
 			// SSL callback
 			curl_easy_setopt(curl, CURLOPT_SSL_CTX_FUNCTION, CurlSSLCallback);
@@ -1355,7 +1378,7 @@ void CurlTransfer( _In_ PCURL_REQUEST pReq )
 						break;		/// Canceled
 					if (pReq->Error.iHttp > 0 && (pReq->Error.iHttp < 200 || pReq->Error.iHttp >= 300))
 						break;		/// HTTP error
-					if (pReq->Error.iCurl == CURLE_PEER_FAILED_VERIFICATION || pReq->Error.iX509 != X509_V_OK)
+					if (pReq->Error.iCurl == CURLE_SSL_CONNECT_ERROR || pReq->Error.iCurl == CURLE_PEER_FAILED_VERIFICATION || pReq->Error.iX509 != X509_V_OK)
 						break;		/// SSL error
 
 					// Cancel?

diff --git a/src/nscurl/curl.h b/src/nscurl/curl.h
@@ -40,6 +40,7 @@ typedef struct _CURL_REQUEST {
 	BOOLEAN		bMarkOfTheWeb : 1;
 	BOOLEAN     bHttp11       : 1;
 	BOOLEAN     bEncoding     : 1;
+	BOOLEAN     bStrongSecurity : 1;	/// Use default openssl security level. By default, we use SSL_CTX_set_security_level(0) and SSL3 as minimum protocol
 	BOOLEAN     bCastore      : 1;      /// Use native CA store (CURLSSLOPT_NATIVE_CA)
 	LPCSTR		pszCacert;				/// can be CACERT_BUILTIN(NULL), CACERT_NONE, or a file path
 	struct curl_slist *pCertList;		/// List of sha1 certificate thumprints. can be NULL

diff --git a/tests/NScurl-Debug.nsi b/tests/NScurl-Debug.nsi
@@ -699,14 +699,16 @@ Var /global testCastoreName
 Var /global testCastoreValue
 Var /global testCertName
 Var /global testCertValue
+Var /global testSecurityName
+Var /global testSecurityValue
 
-!macro CERT_TEST url file cacert castore cert errortype errorcode
+!macro TRANSFER_TEST url file cacert castore cert security errortype errorcode
     StrCpy $R0 '${file}'
 
     ${If} `${cacert}` == ""
         StrCpy $testCacertName ""
         StrCpy $testCacertValue ""
-        StrCpy $R0 '$R0_default'
+        StrCpy $R0 '$R0_defcacert'
     ${ElseIf} `${cacert}` == "none"
     ${OrIf} `${cacert}` == "builtin"
         StrCpy $testCacertName "/CACERT"
@@ -721,7 +723,7 @@ Var /global testCertValue
     ${If} `${castore}` == ""
         StrCpy $testCastoreName ""
         StrCpy $testCastoreValue ""
-        StrCpy $R0 '$R0_default'
+        StrCpy $R0 '$R0_defcastore'
     ${Else}
         StrCpy $testCastoreName "/CASTORE"
         StrCpy $testCastoreValue `${castore}`
@@ -731,14 +733,24 @@ Var /global testCertValue
     ${If} `${cert}` == ""
         StrCpy $testCertName ""
         StrCpy $testCertValue ""
-        StrCpy $R0 '$R0_nocert'
+        StrCpy $R0 '$R0_defcert'
     ${Else}
         StrCpy $testCertName "/CERT"
         StrCpy $testCertValue `${cert}`
         StrCpy $0 `${cert}` 8
         StrCpy $R0 '$R0_$0'
     ${EndIf}
 
+    ${If} `${security}` == ""
+        StrCpy $testSecurityName ""
+        StrCpy $testSecurityValue ""
+        StrCpy $R0 '$R0_defsecurity'
+    ${Else}
+        StrCpy $testSecurityName "/SECURITY"
+        StrCpy $testSecurityValue `${security}`
+        StrCpy $R0 '$R0_${security}'
+    ${EndIf}
+
     ${GetFileName} $R0 $0
 	DetailPrint 'NScurl::http "${url}" "$0"'
 
@@ -761,6 +773,8 @@ Var /global testCertValue
     Push $testCastoreName
     Push $testCacertValue
     Push $testCacertName
+    Push $testSecurityValue
+    Push $testSecurityName
 	Push memory
 	Push "${url}"
 	Push "GET"
@@ -808,9 +822,9 @@ Section "Expired certificate"
 
     !define /ifndef X509_V_ERR_CERT_HAS_EXPIRED 10
 
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' '' '' '' x509 ${X509_V_ERR_CERT_HAS_EXPIRED}
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'none' 'true' '' x509 ${X509_V_ERR_CERT_HAS_EXPIRED}
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'none' 'false' '' http 200       ; SSL validation disabled
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' ''     ''      '' '' x509 ${X509_V_ERR_CERT_HAS_EXPIRED}
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'none' 'true'  '' '' x509 ${X509_V_ERR_CERT_HAS_EXPIRED}
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'none' 'false' '' '' http 200       ; SSL validation disabled
 
     Push /REMOVE
     Push "test"
@@ -827,9 +841,9 @@ Section "Wrong host"
 
     !define /ifndef CURLE_PEER_FAILED_VERIFICATION 60
 
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' '' '' '' curl ${CURLE_PEER_FAILED_VERIFICATION}
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'none' 'true' '' curl ${CURLE_PEER_FAILED_VERIFICATION}
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'none' 'false' '' http 200       ; SSL validation disabled
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' ''     ''      '' '' curl ${CURLE_PEER_FAILED_VERIFICATION}
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'none' 'true'  '' '' curl ${CURLE_PEER_FAILED_VERIFICATION}
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'none' 'false' '' '' http 200       ; SSL validation disabled
 
     Push /REMOVE
     Push "test"
@@ -847,11 +861,11 @@ Section "Untrusted root"
     !define /ifndef UNTRUSTED_CERT '7890C8934D5869B25D2F8D0D646F9A5D7385BA85'
     !define /ifndef X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN 19
 
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' '' '' '' x509 ${X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN}
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' '' '' ${UNTRUSTED_CERT} http 200
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' ''                '' x509 ${X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN}
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' ${UNTRUSTED_CERT} '' http 200
 
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'none' 'true' '' x509 ${X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN}
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'none' 'false' '' http 200       ; SSL validation disabled
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'none' 'true'  '' '' x509 ${X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN}
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'none' 'false' '' '' http 200       ; SSL validation disabled
 
     Push /REMOVE
     Push "test"
@@ -868,29 +882,92 @@ Section "Self-signed certificate"
 
     !define /ifndef X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT 18
 
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' ''        '' '' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'builtin' '' '' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'none'    '' '' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' ''        '' '' '' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'builtin' '' '' '' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'none'    '' '' '' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
+
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'none' 'false' '${BADSSL_SELFSIGNED_CRT}' '' http 200
+
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'builtin' 'true'  '' '' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'builtin' 'false' '' '' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'none'    'true'  '' '' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
+
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'none' 'false' '' '' http 200       ; SSL validation disabled
+
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'none' 'true'  '1111111111111111111111111111111111111111' '' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'none' 'false' '1111111111111111111111111111111111111111' '' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
+
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'none' 'true'  ${BADSSL_SELFSIGNED_THUMBPRINT} '' http 200
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' 'none' 'false' ${BADSSL_SELFSIGNED_THUMBPRINT} '' http 200
+
+    Push /REMOVE
+    Push "test"
+    Push /TAG
+    CallInstDLL $DLL cancel     ; no return
 
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'none' 'false' '${BADSSL_SELFSIGNED_CRT}' http 200
+SectionEnd
 
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'builtin' 'true'  '' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'builtin' 'false' '' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'none'    'true'  '' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
+Section "Unsafe legacy renegociation"
+	SectionIn ${INSTTYPE_MOST}
+	DetailPrint '=====[ ${__SECTION__} ]==============================='
 
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'none' 'false' '' http 200       ; SSL validation disabled
+	!define /redef LINK 'https://publicinfobanjir.water.gov.my'
+	!define /redef FILE '$EXEDIR\_test_legacynego'
 
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'none' 'true'  '1111111111111111111111111111111111111111' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'none' 'false' '1111111111111111111111111111111111111111' x509 ${X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT}
+    !define /redef CURLE_SSL_CONNECT_ERROR 35
 
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'none' 'true'  ${BADSSL_SELFSIGNED_THUMBPRINT} http 200
-    !insertmacro CERT_TEST '${LINK}' '${FILE}' 'none' 'false' ${BADSSL_SELFSIGNED_THUMBPRINT} http 200
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' '' ''       http 200
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' '' 'weak'   http 200
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' '' 'strong' curl ${CURLE_SSL_CONNECT_ERROR} ; OpenSSL/3.3.1: error:0A000152:SSL routines::unsafe legacy renegotiation disabled
 
     Push /REMOVE
     Push "test"
     Push /TAG
     CallInstDLL $DLL cancel     ; no return
+SectionEnd
 
+Section "Weak protocols"
+	SectionIn ${INSTTYPE_MOST}
+	DetailPrint '=====[ ${__SECTION__} ]==============================='
+
+    !define /redef CURLE_SSL_CONNECT_ERROR 35
+
+	!define /redef LINK 'https://tls-v1-0.badssl.com:1010/'
+	!define /redef FILE '$EXEDIR\_test_weaktls10'
+
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' '' ''       http 200
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' '' 'weak'   http 200
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' '' 'strong' curl ${CURLE_SSL_CONNECT_ERROR}    ; OpenSSL/3.3.1: error:0A000102:SSL routines::unsupported protocol
+
+
+	!define /redef LINK 'https://tls-v1-1.badssl.com:1011/'
+	!define /redef FILE '$EXEDIR\_test_weaktls11'
+
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' '' ''       http 200
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' '' 'weak'   http 200
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' '' 'strong' curl ${CURLE_SSL_CONNECT_ERROR}    ; OpenSSL/3.3.1: error:0A000102:SSL routines::unsupported protocol
+
+
+    !define /redef LINK 'https://tls-v1-2.badssl.com:1012/'
+	!define /redef FILE '$EXEDIR\_test_weaktls12'
+
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' '' ''       http 200
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' '' 'weak'   http 200
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' '' 'strong' http 200   ; TLS 1.2 should always work
+
+    ; ----------------------------------------------
+
+    !define /redef LINK 'https://dh2048.badssl.com'
+	!define /redef FILE '$EXEDIR\_test_weakdh2k'
+
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' '' ''       http 200
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' '' 'weak'   http 200
+    !insertmacro TRANSFER_TEST '${LINK}' '${FILE}' '' '' '' 'strong' http 200
+
+    Push /REMOVE
+    Push "test"
+    Push /TAG
+    CallInstDLL $DLL cancel     ; no return
 SectionEnd