diff --git a/src/wolfio.c b/src/wolfio.c index 4ebff95c69..8bee830907 100644 --- a/src/wolfio.c +++ b/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) diff --git a/tests/api.c b/tests/api.c index 9676c470a3..f2de9cfd14 100644 --- a/tests/api.c +++ b/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), diff --git a/tests/api/test_ocsp.c b/tests/api/test_ocsp.c index 9bc74de223..5ebed3b390 100644 --- a/tests/api/test_ocsp.c +++ b/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 */ diff --git a/tests/api/test_ocsp.h b/tests/api/test_ocsp.h index e8e2aa3cf2..cbe76c9ee3 100644 --- a/tests/api/test_ocsp.h +++ b/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 */