wwbmmm commented on code in PR #3387:
URL: https://github.com/apache/brpc/pull/3387#discussion_r3585566891
##########
src/brpc/details/http_message.cpp:
##########
@@ -644,12 +654,17 @@ void MakeRawHttpRequest(butil::IOBuf* request,
}
os << BRPC_CRLF;
}
- if (!h->content_type().empty()) {
+ if (!h->content_type().empty() && !HeaderHasCRLF(h->content_type())) {
os << "Content-Type: " << h->content_type()
<< BRPC_CRLF;
}
for (HttpHeader::HeaderIterator it = h->HeaderBegin();
it != h->HeaderEnd(); ++it) {
+ if (HeaderHasCRLF(it->first) || HeaderHasCRLF(it->second)) {
+ LOG(ERROR) << "Skip header `" << it->first
Review Comment:
This should be warning, not error
##########
src/brpc/details/http_message.cpp:
##########
@@ -578,6 +578,16 @@ std::ostream& operator<<(std::ostream& os, const
http_parser& parser) {
#define BRPC_CRLF "\r\n"
+// A header field-name or field-value carrying a raw CR or LF lets whoever
+// controls it close the current line and inject extra header fields (or a
+// body) into the serialized message, i.e. HTTP request/response splitting.
+// The inbound parser already refuses these bytes; the outbound path drops
+// such fields so a value forwarded from an untrusted source can't smuggle
+// headers.
+static bool HeaderHasCRLF(const std::string& s) {
+ return s.find_first_of("\r\n") != std::string::npos;
Review Comment:
This may affect performance, since MakeRawHttpRequest/Response are hot
paths. You'd better add a flag to switch this on/off.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
