Reject CR/LF in OCSP/CRL URLs to block HTTP injection

src/wolfio.c

@@ -1680,7 +1680,7 @@ int wolfIO_TcpAccept(SOCKET_T sockfd, SOCKADDR* peer_addr, XSOCKLENT* peer_len)
 int wolfIO_DecodeUrl(const char* url, int urlSz, char* outName, char* outPath,
     word16* outPort)
 {
-    int result = -1;
+    int result = WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR);
 
     if (url == NULL || urlSz == 0) {
         if (outName)
@@ -1706,6 +1706,8 @@ int wolfIO_DecodeUrl(const char* url, int urlSz, char* outName, char* outPath,
             /* copy until ']' */
             while (i < MAX_URL_ITEM_SIZE-1 && cur < urlSz && url[cur] != 0 &&
                     url[cur] != ']') {
+                if (url[cur] == '\r' || url[cur] == '\n')
+                    return WOLFSSL_FATAL_ERROR;
                 if (outName)
                     outName[i] = url[cur];
                 i++; cur++;
@@ -1715,6 +1717,8 @@ int wolfIO_DecodeUrl(const char* url, int urlSz, char* outName, char* outPath,
         else {
             while (i < MAX_URL_ITEM_SIZE-1 && cur < urlSz && url[cur] != 0 &&
                     url[cur] != ':' && url[cur] != '/') {
+                if (url[cur] == '\r' || url[cur] == '\n')
+                    return WOLFSSL_FATAL_ERROR;
                 if (outName)
                     outName[i] = url[cur];
                 i++; cur++;
@@ -1752,6 +1756,8 @@ int wolfIO_DecodeUrl(const char* url, int urlSz, char* outName, char* outPath,
         if (cur < urlSz && url[cur] == '/') {
             i = 0;
             while (i < MAX_URL_ITEM_SIZE-1 && cur < urlSz && url[cur] != 0) {
+                if (url[cur] == '\r' || url[cur] == '\n')
+                    return WOLFSSL_FATAL_ERROR;
                 if (outPath)
                     outPath[i] = url[cur];
                 i++; cur++;
@@ -2065,6 +2071,21 @@ int wolfIO_HttpBuildRequest(const char *reqType, const char *domainName,
     return wolfIO_HttpBuildRequest_ex(reqType, domainName, path, pathLen, reqSz, contentType, "", buf, bufSize);
 }
 
+/* Returns 1 if the buffer contains a CR or LF byte, 0 otherwise. Used to
+ * reject attacker-controlled URL components that would allow HTTP header
+ * injection or request splitting. */
+static int wolfIO_UrlHasCrlf(const char* in, int inSz)
+{
+    int i = 0;
+    if (in == NULL)
+        return 0;
+    for (i = 0; i < inSz; i++) {
+        if (in[i] == '\r' || in[i] == '\n')
+            return 1;
+    }
+    return 0;
+}
+
 int wolfIO_HttpBuildRequest_ex(const char *reqType, const char *domainName,
                                 const char *path, int pathLen, int reqSz, const char *contentType,
                                 const char *exHdrs, byte *buf, int bufSize)
@@ -2082,11 +2103,24 @@ int wolfIO_HttpBuildRequest_ex(const char *reqType, const char *domainName,
     word32 blankStrLen, http11StrLen, hostStrLen, contentLenStrLen,
         contentTypeStrLen, singleCrLfStrLen, doubleCrLfStrLen;
 
+    /* reject NULL pointers and invalid lengths before any deref or length
+     * math: the XSTRLEN/copy calls below assume non-NULL inputs */
+    if (reqType == NULL || domainName == NULL || path == NULL ||
+            contentType == NULL || pathLen < 0 || bufSize <= 0)
+        return 0;
+
     reqTypeLen = (word32)XSTRLEN(reqType);
     domainNameLen = (word32)XSTRLEN(domainName);
     reqSzStrLen = wolfIO_Word16ToString(reqSzStr, (word16)reqSz);
     contentTypeLen = (word32)XSTRLEN(contentType);
 
+    /* reject CR/LF in the path or host to prevent HTTP header injection or
+     * request splitting from attacker-controlled URL components */
+    if (wolfIO_UrlHasCrlf(path, pathLen) ||
+            wolfIO_UrlHasCrlf(domainName, (int)domainNameLen)) {
+        return 0;
+    }
+
     blankStrLen = (word32)XSTRLEN(blankStr);
     http11StrLen = (word32)XSTRLEN(http11Str);
     hostStrLen = (word32)XSTRLEN(hostStr);
@@ -2126,7 +2160,10 @@ int wolfIO_HttpBuildRequest_ex(const char *reqType, const char *domainName,
     buf += reqTypeLen; bufSize -= (int)reqTypeLen;
     XSTRNCPY((char*)buf, blankStr, (size_t)bufSize);
     buf += blankStrLen; bufSize -= (int)blankStrLen;
-    XSTRNCPY((char*)buf, path, (size_t)bufSize);
+    /* path may not be NUL terminated (CRL raw-URL case passes a pointer into
+     * the certificate with only pathLen valid bytes), so bound the copy to
+     * pathLen rather than scanning for a NUL */
+    XMEMCPY((char*)buf, path, (size_t)pathLen);
     buf += pathLen; bufSize -= (int)pathLen;
     XSTRNCPY((char*)buf, http11Str, (size_t)bufSize);
     buf += http11StrLen; bufSize -= (int)http11StrLen;
@@ -2251,8 +2288,11 @@ int EmbedOcspLookup(void* ctx, const char* url, int urlSz,
             httpBufSz = wolfIO_HttpBuildRequestOcsp(domainName, path, ocspReqSz,
                                                             httpBuf, httpBufSz);
 
-            ret = wolfIO_TcpConnect(&sfd, domainName, port, io_timeout_sec);
-            if (ret != 0) {
+            if (httpBufSz <= 0) {
+                WOLFSSL_MSG("Unable to build OCSP request");
+            }
+            else if ((ret = wolfIO_TcpConnect(&sfd, domainName, port,
+                                              io_timeout_sec)) != 0) {
                 WOLFSSL_MSG("OCSP Responder connection failed");
             }
             else if (wolfIO_Send(sfd, (char*)httpBuf, httpBufSz, 0) !=
@@ -2351,8 +2391,11 @@ int EmbedCrlLookup(WOLFSSL_CRL* crl, const char* url, int urlSz)
             httpBufSz = wolfIO_HttpBuildRequestCrl(url, urlSz, domainName,
                 httpBuf, httpBufSz);
 
-            ret = wolfIO_TcpConnect(&sfd, domainName, port, io_timeout_sec);
-            if (ret != 0) {
+            if (httpBufSz <= 0) {
+                WOLFSSL_MSG("Unable to build CRL request");
+            }
+            else if ((ret = wolfIO_TcpConnect(&sfd, domainName, port,
+                                              io_timeout_sec)) != 0) {
                 WOLFSSL_MSG("CRL connection failed");
             }
             else if (wolfIO_Send(sfd, (char*)httpBuf, httpBufSz, 0)

tests/api.c

@@ -35293,6 +35293,7 @@ TEST_CASE testCases[] = {
     TEST_DECL(test_ocsp_cert_unknown_crl_fallback_nonleaf),
     TEST_DECL(test_tls13_nonblock_ocsp_low_mfl),
     TEST_DECL(test_ocsp_responder),
+    TEST_DECL(test_wolfIO_DecodeUrl_crlf_reject),
     TEST_TLS_DECLS,
     TEST_SESSION_DECLS,
     TEST_DECL(test_wc_DhSetNamedKey),

tests/api/test_ocsp.c

@@ -1809,3 +1809,101 @@ int test_ocsp_responder(void)
     return TEST_SKIPPED;
 }
 #endif /* HAVE_OCSP_RESPONDER && !NO_SHA && !NO_RSA */
+
+#if defined(HAVE_HTTP_CLIENT)
+/* A peer-supplied AIA/CRL URL must not be able to smuggle CR/LF into the
+ * outbound OCSP/CRL HTTP request (header injection / request splitting). */
+int test_wolfIO_DecodeUrl_crlf_reject(void)
+{
+    EXPECT_DECLS;
+    char   domainName[80];
+    char   path[80];
+    word16 port;
+    byte   reqBuf[512];
+    char*  tightPath = NULL;
+    int    tightLen  = 16;
+    const char* crlfInPath = "http://ocsp.example.com/ocsp\r\nX-Injected: 1";
+    const char* crlfInHost = "http://evil\r\nHost: x/ocsp";
+    const char* crlfInV6Host = "http://[::1\r\n]/ocsp";
+    const char* cleanUrl   = "http://ocsp.example.com/ocsp";
+    const char* taintedPath = "/ocsp\r\nX-Injected: 1";
+    const char* crlfAfterCap =
+        "http://ocsp.example.com/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\r\nX: 1";
+
+    /* parser rejects CR/LF in the path */
+    ExpectIntLT(wolfIO_DecodeUrl(crlfInPath, (int)XSTRLEN(crlfInPath),
+        domainName, path, &port), 0);
+
+    /* parser rejects CR/LF in the host */
+    ExpectIntLT(wolfIO_DecodeUrl(crlfInHost, (int)XSTRLEN(crlfInHost),
+        domainName, path, &port), 0);
+
+    /* parser rejects CR/LF in an IPv6 bracket-literal host */
+    ExpectIntLT(wolfIO_DecodeUrl(crlfInV6Host, (int)XSTRLEN(crlfInV6Host),
+        domainName, path, &port), 0);
+
+    /* sink rejects a tainted path even when the parser is bypassed, which is
+     * the CRL raw-URL case */
+    ExpectIntEQ(wolfIO_HttpBuildRequest("POST", "ocsp.example.com",
+        taintedPath, (int)XSTRLEN(taintedPath), 0, "application/ocsp-request",
+        reqBuf, (int)sizeof(reqBuf)), 0);
+
+    /* sink rejects CR/LF even when it appears beyond the decoder cap */
+    ExpectIntEQ(wolfIO_HttpBuildRequest("GET", "ocsp.example.com",
+        crlfAfterCap, (int)XSTRLEN(crlfAfterCap), 0, "", reqBuf,
+        (int)sizeof(reqBuf)), 0);
+
+    /* sink rejects CR/LF in the host (domainName) with a clean path */
+    ExpectIntEQ(wolfIO_HttpBuildRequest("POST", "evil.example.com\r\nX: 1",
+        "/ocsp", (int)XSTRLEN("/ocsp"), 0, "application/ocsp-request", reqBuf,
+        (int)sizeof(reqBuf)), 0);
+
+    /* the CRL raw-URL path is not NUL terminated, so the request builder must
+     * copy only pathLen bytes. Allocate exactly pathLen bytes with no
+     * terminator so an over-read past the buffer is caught under ASAN. */
+    tightPath = (char*)XMALLOC((size_t)tightLen, NULL, DYNAMIC_TYPE_TMP_BUFFER);
+    ExpectNotNull(tightPath);
+    if (tightPath != NULL) {
+        XMEMSET(tightPath, 'a', (size_t)tightLen);
+        tightPath[0] = '/';
+        ExpectIntGT(wolfIO_HttpBuildRequest("GET", "ocsp.example.com",
+            tightPath, tightLen, 0, "", reqBuf, (int)sizeof(reqBuf)), 0);
+    }
+
+    /* sink rejects a negative pathLen before any length math or copy: with the
+     * old XMEMCPY this wrapped (word32)pathLen and overran the output buffer */
+    ExpectIntEQ(wolfIO_HttpBuildRequest("GET", "ocsp.example.com", "/ocsp",
+        -1, 0, "", reqBuf, (int)sizeof(reqBuf)), 0);
+
+    /* sink rejects a non-positive bufSize before any copy */
+    ExpectIntEQ(wolfIO_HttpBuildRequest("GET", "ocsp.example.com", "/ocsp",
+        (int)XSTRLEN("/ocsp"), 0, "", reqBuf, 0), 0);
+
+    /* sink rejects NULL pointer parameters at the boundary, before any
+     * XSTRLEN deref */
+    ExpectIntEQ(wolfIO_HttpBuildRequest(NULL, "ocsp.example.com", "/ocsp",
+        (int)XSTRLEN("/ocsp"), 0, "", reqBuf, (int)sizeof(reqBuf)), 0);
+    ExpectIntEQ(wolfIO_HttpBuildRequest("GET", NULL, "/ocsp",
+        (int)XSTRLEN("/ocsp"), 0, "", reqBuf, (int)sizeof(reqBuf)), 0);
+    ExpectIntEQ(wolfIO_HttpBuildRequest("GET", "ocsp.example.com", NULL,
+        0, 0, "", reqBuf, (int)sizeof(reqBuf)), 0);
+    ExpectIntEQ(wolfIO_HttpBuildRequest("GET", "ocsp.example.com", "/ocsp",
+        (int)XSTRLEN("/ocsp"), 0, NULL, reqBuf, (int)sizeof(reqBuf)), 0);
+
+    /* positive control: a clean URL decodes and still builds a request */
+    ExpectIntEQ(wolfIO_DecodeUrl(cleanUrl, (int)XSTRLEN(cleanUrl), domainName,
+        path, &port), 0);
+    ExpectIntGT(wolfIO_HttpBuildRequest("POST", domainName, path,
+        (int)XSTRLEN(path), 0, "application/ocsp-request", reqBuf,
+        (int)sizeof(reqBuf)), 0);
+
+    XFREE(tightPath, NULL, DYNAMIC_TYPE_TMP_BUFFER);
+
+    return EXPECT_RESULT();
+}
+#else
+int test_wolfIO_DecodeUrl_crlf_reject(void)
+{
+    return TEST_SKIPPED;
+}
+#endif /* HAVE_HTTP_CLIENT */

tests/api/test_ocsp.h

@@ -35,5 +35,6 @@ int test_ocsp_cert_unknown_crl_fallback_nonleaf(void);
 int test_tls13_nonblock_ocsp_low_mfl(void);
 int test_ocsp_responder(void);
 int test_ocsp_ancestor_responder_rejected(void);
+int test_wolfIO_DecodeUrl_crlf_reject(void);
 #endif /* WOLFSSL_TEST_OCSP_H */