HTTPSock: AddHeader should reject CRLF in header name/value #1094

New Issue

2026-10-05T05:02:57+02:00

adam commented

2026-10-05 05:02:57 +02:00

AddHeader writes its arguments verbatim into the response stream, so any caller that passes attacker-controlled bytes can split headers / inject a response.

// src/HTTPSock.cpp
765: void CHTTPSock::AddHeader(const CString& sName, const CString& sValue) {
766:     m_msHeaders[sName] = sValue;
767: }
...
749:     for (const auto& it : m_msHeaders) {
750:         Write(it.first + ": " + it.second + "\r\n");
751:     }

I traced every call site in src/ and modules/. None currently passes attacker-controlled bytes:

AddHeader("Content-Encoding", "gzip"): literal
AddHeader("Last-Modified", GetDate(iMTime)): RFC1123 date, no CRLF
AddHeader("ETag", "\"" + sETag + "\"") with sETag = "-" + iMTime
AddHeader("Cache-Control", "public"): literal
AddHeader("WWW-Authenticate", "Basic realm=\"ZNC\""): literal
AddHeader("Location", m_sURIPrefix + sURL) where sURL.StartsWith("/") is required and every caller passes a controlled path.

So this is not currently exploitable. But the function is exported via CHTTPSock and any module author can call it with user-supplied data. A two-line guard saves the next contributor from a footgun.

Fix

void CHTTPSock::AddHeader(const CString& sName, const CString& sValue) {
    if (sName.find_first_of("\r\n") != CString::npos) return;
    if (sValue.find_first_of("\r\n") != CString::npos) return;
    m_msHeaders[sName] = sValue;
}

(Could also assert rather than silently ignore; either works. Filtering seems safer than asserting given that production builds don't run with asserts on.)

Filed as a hardening note, no PoC: there is no caller today that hits this.

`AddHeader` writes its arguments verbatim into the response stream, so any caller that passes attacker-controlled bytes can split headers / inject a response. ```cpp // src/HTTPSock.cpp 765: void CHTTPSock::AddHeader(const CString& sName, const CString& sValue) { 766: m_msHeaders[sName] = sValue; 767: } ... 749: for (const auto& it : m_msHeaders) { 750: Write(it.first + ": " + it.second + "\r\n"); 751: } ``` I traced every call site in `src/` and `modules/`. None currently passes attacker-controlled bytes: - `AddHeader("Content-Encoding", "gzip")`: literal - `AddHeader("Last-Modified", GetDate(iMTime))`: RFC1123 date, no CRLF - `AddHeader("ETag", "\"" + sETag + "\"")` with `sETag = "-" + iMTime` - `AddHeader("Cache-Control", "public")`: literal - `AddHeader("WWW-Authenticate", "Basic realm=\"ZNC\"")`: literal - `AddHeader("Location", m_sURIPrefix + sURL)` where `sURL.StartsWith("/")` is required and every caller passes a controlled path. So this is not currently exploitable. But the function is exported via `CHTTPSock` and any module author can call it with user-supplied data. A two-line guard saves the next contributor from a footgun. ## Fix ```cpp void CHTTPSock::AddHeader(const CString& sName, const CString& sValue) { if (sName.find_first_of("\r\n") != CString::npos) return; if (sValue.find_first_of("\r\n") != CString::npos) return; m_msHeaders[sName] = sValue; } ``` (Could also `assert` rather than silently ignore; either works. Filtering seems safer than asserting given that production builds don't run with asserts on.) Filed as a hardening note, no PoC: there is no caller today that hits this.

adam closed this issue

2026-10-05 05:02:57 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: adam/znc#1094