Skip to content

Commit f15ec82

Browse files
bencebekybencebeky
authored
[balsa] Disallow CR not followed by LF in header values. (envoyproxy#34829)
This makes the behavior of BalsaParser consistent with that of http-parser. Signed-off-by: Bence Béky <bnc@google.com>
1 parent 43d6b1e commit f15ec82

File tree

2 files changed

+25
-37
lines changed

2 files changed

+25
-37
lines changed

‎source/common/http/http1/balsa_parser.cc

+1
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,7 @@ BalsaParser::BalsaParser(MessageType type, ParserCallbacks* connection, size_t m
160160
http_validation_policy.validate_transfer_encoding = false;
161161
http_validation_policy.require_content_length_if_body_required = false;
162162
http_validation_policy.disallow_invalid_header_characters_in_response = true;
163+
http_validation_policy.disallow_lone_cr_in_request_headers = true;
163164
framer_.set_http_validation_policy(http_validation_policy);
164165

165166
framer_.set_balsa_headers(&headers_);

‎test/common/http/http1/codec_impl_test.cc

+24-37
Original file line numberDiff line numberDiff line change
@@ -4968,8 +4968,8 @@ TEST_P(Http1ServerConnectionImplTest, ValueStartsWithCR) {
49684968
const absl::string_view value = "\r value starts with carriage return";
49694969

49704970
if (parser_impl_ == Http1ParserImpl::BalsaParser) {
4971-
const absl::string_view expected_value = "value starts with carriage return";
4972-
testRequestWithValueExpectSuccess(value, expected_value);
4971+
testRequestWithValueExpectFailure(value, "http1.invalid_characters",
4972+
"header value contains invalid chars");
49734973
} else {
49744974
#ifdef ENVOY_ENABLE_UHV
49754975
testRequestWithValueExpectFailure(value, "http1.codec_error", "HPE_INVALID_HEADER_TOKEN");
@@ -4983,8 +4983,8 @@ TEST_P(Http1ServerConnectionImplTest, ValueWithCRInTheMiddle) {
49834983
const absl::string_view value = "value has \r carriage return in the middle";
49844984

49854985
if (parser_impl_ == Http1ParserImpl::BalsaParser) {
4986-
const absl::string_view expected_value = "value has carriage return in the middle";
4987-
testRequestWithValueExpectSuccess(value, expected_value);
4986+
testRequestWithValueExpectFailure(value, "http1.invalid_characters",
4987+
"header value contains invalid chars");
49884988
} else {
49894989
testRequestWithValueExpectFailure(value, "http1.codec_error", "HPE_LF_EXPECTED");
49904990
}
@@ -4994,8 +4994,8 @@ TEST_P(Http1ServerConnectionImplTest, ValueEndsWithCR) {
49944994
const absl::string_view value = "value ends in carriage return \r";
49954995

49964996
if (parser_impl_ == Http1ParserImpl::BalsaParser) {
4997-
const absl::string_view expected_value = "value ends in carriage return";
4998-
testRequestWithValueExpectSuccess(value, expected_value);
4997+
testRequestWithValueExpectFailure(value, "http1.invalid_characters",
4998+
"header value contains invalid chars");
49994999
} else {
50005000
testRequestWithValueExpectFailure(value, "http1.codec_error", "HPE_LF_EXPECTED");
50015001
}
@@ -5093,8 +5093,7 @@ TEST_P(Http1ClientConnectionImplTest, ValueStartsWithCR) {
50935093
const absl::string_view value = "\r value starts with carriage return";
50945094

50955095
if (parser_impl_ == Http1ParserImpl::BalsaParser) {
5096-
const absl::string_view expected_value = "value starts with carriage return";
5097-
testRequestWithValueExpectSuccess(value, expected_value);
5096+
testRequestWithValueExpectFailure(value, "header value contains invalid chars");
50985097
} else {
50995098
#ifdef ENVOY_ENABLE_UHV
51005099
testRequestWithValueExpectFailure(value, "HPE_INVALID_HEADER_TOKEN");
@@ -5108,8 +5107,7 @@ TEST_P(Http1ClientConnectionImplTest, ValueWithCRInTheMiddle) {
51085107
const absl::string_view value = "value has \r carriage return in the middle";
51095108

51105109
if (parser_impl_ == Http1ParserImpl::BalsaParser) {
5111-
const absl::string_view expected_value = "value has carriage return in the middle";
5112-
testRequestWithValueExpectSuccess(value, expected_value);
5110+
testRequestWithValueExpectFailure(value, "header value contains invalid chars");
51135111
} else {
51145112
testRequestWithValueExpectFailure(value, "HPE_LF_EXPECTED");
51155113
}
@@ -5119,8 +5117,7 @@ TEST_P(Http1ClientConnectionImplTest, ValueEndsWithCR) {
51195117
const absl::string_view value = "value ends in carriage return \r";
51205118

51215119
if (parser_impl_ == Http1ParserImpl::BalsaParser) {
5122-
const absl::string_view expected_value = "value ends in carriage return";
5123-
testRequestWithValueExpectSuccess(value, expected_value);
5120+
testRequestWithValueExpectFailure(value, "header value contains invalid chars");
51245121
} else {
51255122
testRequestWithValueExpectFailure(value, "HPE_LF_EXPECTED");
51265123
}
@@ -5154,24 +5151,20 @@ TEST_P(Http1ServerConnectionImplTest, FirstLineInvalidCR) {
51545151
MockRequestDecoder decoder;
51555152
EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder));
51565153

5157-
TestRequestHeaderMapImpl expected_headers{
5158-
{":path", "/"},
5159-
{":method", "GET"},
5160-
};
51615154
if (parser_impl_ == Http1ParserImpl::BalsaParser) {
5162-
EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true));
5155+
EXPECT_CALL(decoder, sendLocalReply(Http::Code::BadRequest, "Bad Request", _, _,
5156+
"http1.invalid_characters"));
51635157
} else {
51645158
EXPECT_CALL(decoder,
51655159
sendLocalReply(Http::Code::BadRequest, "Bad Request", _, _, "http1.codec_error"));
51665160
}
51675161

51685162
Buffer::OwnedImpl buffer("GET /\rHTTP/1.1\r\n\r\n");
51695163
auto status = codec_->dispatch(buffer);
5164+
EXPECT_TRUE(isCodecProtocolError(status));
51705165
if (parser_impl_ == Http1ParserImpl::BalsaParser) {
5171-
EXPECT_TRUE(status.ok());
5172-
EXPECT_EQ(0u, buffer.length());
5166+
EXPECT_EQ(status.message(), "http/1.1 protocol error: header value contains invalid chars");
51735167
} else {
5174-
EXPECT_TRUE(isCodecProtocolError(status));
51755168
EXPECT_EQ(status.message(), "http/1.1 protocol error: HPE_LF_EXPECTED");
51765169
}
51775170
}
@@ -5190,24 +5183,13 @@ TEST_P(Http1ClientConnectionImplTest, FirstLineInvalidCR) {
51905183
};
51915184
EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok());
51925185

5193-
TestResponseHeaderMapImpl expected_headers{
5194-
{":status", "200"},
5195-
{"content-length", "5"},
5196-
};
5197-
if (parser_impl_ == Http1ParserImpl::BalsaParser) {
5198-
EXPECT_CALL(response_decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false));
5199-
EXPECT_CALL(response_decoder, decodeData(BufferStringEqual("hello"), false));
5200-
EXPECT_CALL(response_decoder, decodeData(BufferStringEqual(""), true));
5201-
}
5202-
52035186
Buffer::OwnedImpl buffer("HTTP/1.1 200\rOK\r\ncontent-length: 5\r\n\r\n"
52045187
"hello");
52055188
auto status = codec_->dispatch(buffer);
5189+
EXPECT_TRUE(isCodecProtocolError(status));
52065190
if (parser_impl_ == Http1ParserImpl::BalsaParser) {
5207-
EXPECT_TRUE(status.ok());
5208-
EXPECT_EQ(0u, buffer.length());
5191+
EXPECT_EQ(status.message(), "http/1.1 protocol error: header value contains invalid chars");
52095192
} else {
5210-
EXPECT_TRUE(isCodecProtocolError(status));
52115193
#ifdef ENVOY_ENABLE_UHV
52125194
EXPECT_EQ(status.message(), "http/1.1 protocol error: HPE_INVALID_HEADER_TOKEN");
52135195
#else
@@ -5225,16 +5207,21 @@ TEST_P(Http1ServerConnectionImplTest, HeaderNameInvalidCR) {
52255207

52265208
MockRequestDecoder decoder;
52275209
EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder));
5228-
EXPECT_CALL(decoder,
5229-
sendLocalReply(Http::Code::BadRequest, "Bad Request", _, _, "http1.codec_error"));
5210+
if (parser_impl_ == Http1ParserImpl::BalsaParser) {
5211+
EXPECT_CALL(decoder, sendLocalReply(Http::Code::BadRequest, "Bad Request", _, _,
5212+
"http1.invalid_characters"));
5213+
} else {
5214+
EXPECT_CALL(decoder,
5215+
sendLocalReply(Http::Code::BadRequest, "Bad Request", _, _, "http1.codec_error"));
5216+
}
52305217

52315218
// SPELLCHECKER(off)
52325219
Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\nfo\ro: bar\r\n\r\n");
52335220
// SPELLCHECKER(on)
52345221
auto status = codec_->dispatch(buffer);
52355222
EXPECT_TRUE(isCodecProtocolError(status));
52365223
if (parser_impl_ == Http1ParserImpl::BalsaParser) {
5237-
EXPECT_EQ(status.message(), "http/1.1 protocol error: INVALID_HEADER_NAME_CHARACTER");
5224+
EXPECT_EQ(status.message(), "http/1.1 protocol error: header value contains invalid chars");
52385225
} else {
52395226
EXPECT_EQ(status.message(), "http/1.1 protocol error: HPE_INVALID_HEADER_TOKEN");
52405227
}
@@ -5260,7 +5247,7 @@ TEST_P(Http1ClientConnectionImplTest, HeaderNameInvalidCR) {
52605247
auto status = codec_->dispatch(buffer);
52615248
EXPECT_TRUE(isCodecProtocolError(status));
52625249
if (parser_impl_ == Http1ParserImpl::BalsaParser) {
5263-
EXPECT_EQ(status.message(), "http/1.1 protocol error: INVALID_HEADER_NAME_CHARACTER");
5250+
EXPECT_EQ(status.message(), "http/1.1 protocol error: header value contains invalid chars");
52645251
} else {
52655252
EXPECT_EQ(status.message(), "http/1.1 protocol error: HPE_INVALID_HEADER_TOKEN");
52665253
}

0 commit comments

Comments
 (0)