Permalink
Browse files

Don't lower lastStreamID_

Summary:
This could throw off the logic about what is headers/trailers
CVE-2018-6347

Reviewed By: avasylev, bolekk

Differential Revision: D13521704

fbshipit-source-id: ed436ff13f191bf04764494973fcb73b35ed1256
  • Loading branch information...
afrind authored and facebook-github-bot committed Dec 28, 2018
1 parent 52cf331 commit 223e0aa6bc7590e86af1e917185a2e0efe160711
@@ -987,7 +987,7 @@ ErrorCode HTTP2Codec::checkNewStream(uint32_t streamId, bool trailersAllowed) {
VLOG(4) << "Parsing downstream trailers streamId=" << streamId;
}

if (sessionClosing_ != ClosingState::CLOSED) {
if (sessionClosing_ != ClosingState::CLOSED && streamId > lastStreamID_) {
lastStreamID_ = streamId;
}

@@ -1313,6 +1313,7 @@ size_t HTTP2Codec::generateChunkTerminator(folly::IOBufQueue& /*writeBuf*/,
size_t HTTP2Codec::generateTrailers(folly::IOBufQueue& writeBuf,
StreamID stream,
const HTTPHeaders& trailers) {
VLOG(4) << "generating TRAILERS for stream=" << stream;
std::vector<compress::Header> allHeaders;
CodecUtil::appendHeaders(trailers, allHeaders, HTTP_HEADER_NONE);

@@ -2145,3 +2145,28 @@ TEST_F(HTTP2CodecTest, TrailersReplyMissingContinuation) {
EXPECT_EQ(upstreamCodec_.getReceivedFrameCount(), 4);
#endif
}

TEST_F(HTTP2CodecTest, TrailersNotLatest) {
HTTPMessage req = getGetRequest("/guacamole");
req.getHeaders().add(HTTP_HEADER_USER_AGENT, "coolio");
upstreamCodec_.generateHeader(output_, 1, req);
upstreamCodec_.generateHeader(output_, 3, req);

HTTPHeaders trailers;
trailers.add("x-trailer-1", "pico-de-gallo");
upstreamCodec_.generateTrailers(output_, 1, trailers);
upstreamCodec_.generateHeader(output_, 3, req);

parse();

EXPECT_EQ(callbacks_.messageBegin, 2);
EXPECT_EQ(callbacks_.headersComplete, 2);
EXPECT_EQ(callbacks_.bodyCalls, 0);
EXPECT_EQ(callbacks_.trailers, 1);
EXPECT_NE(nullptr, callbacks_.msg->getTrailers());
EXPECT_EQ("pico-de-gallo",
callbacks_.msg->getTrailers()->getSingleOrEmpty("x-trailer-1"));
EXPECT_EQ(callbacks_.messageComplete, 1);
EXPECT_EQ(callbacks_.streamErrors, 1);
EXPECT_EQ(callbacks_.sessionErrors, 0);
}
@@ -3568,3 +3568,35 @@ TEST_F(HTTP2DownstreamSessionTest, TestSetEgressSettings) {
flushRequestsAndLoop();
gracefulShutdown();
}

TEST_F(HTTP2DownstreamSessionTest, TestDuplicateRequestStream) {
// Send the following:
// HEADERS id=1
// HEADERS id=2
// HEADERS id=1 (trailers)
// HEADERS id=2 -> contains pseudo-headers after EOM so ignored
auto handler2 = addSimpleStrictHandler();
auto handler1 = addSimpleStrictHandler();
auto streamID1 = sendRequest("/withtrailers", 0, false);
auto streamID2 = sendRequest();
HTTPHeaders trailers;
trailers.add("Foo", "Bar");
clientCodec_->generateTrailers(requests_, streamID1, trailers);
clientCodec_->generateEOM(requests_, streamID1);

clientCodec_->generateHeader(requests_, streamID2, getGetRequest(), false);
handler1->expectHeaders();
handler2->expectHeaders();
handler2->expectEOM();
handler1->expectTrailers();
handler1->expectEOM([&] {
handler1->sendReplyWithBody(200, 100);
// 2 got an error after EOM, which gets ignored - need a response to
// cleanly terminate it
handler2->sendReplyWithBody(200, 100);
});
handler1->expectDetachTransaction();
handler2->expectDetachTransaction();
flushRequestsAndLoop();
gracefulShutdown();
}
@@ -221,6 +221,17 @@ class MockHTTPHandler
.RetiresOnSaturation();
}

void expectTrailers(
std::function<void()> callback = std::function<void()>()) {
if (callback) {
EXPECT_CALL(*this, onTrailers(testing::_))
.WillOnce(testing::InvokeWithoutArgs(callback))
.RetiresOnSaturation();
} else {
EXPECT_CALL(*this, onTrailers(testing::_));
}
}

void expectTrailers(
std::function<void(std::shared_ptr<HTTPHeaders> trailers)> cb) {
EXPECT_CALL(*this, onTrailers(testing::_))

0 comments on commit 223e0aa

Please sign in to comment.