Permalink
Browse files

Fix h2 codec state after bad priority header.

Summary:
It's possible for the http2 codec to enter an invalid state after processing a http2 header with invalid priorities.
CVE-2018-6346

Reviewed By: maxgeorg

Differential Revision: D13510025

fbshipit-source-id: 7c4e42daf1cd2b912454d13a66ab8488d1863263
  • Loading branch information...
jalopezsilva authored and facebook-github-bot committed Dec 27, 2018
1 parent f81f900 commit 52cf331743ebd74194d6343a6c2ec52bb917c982
Showing with 34 additions and 8 deletions.
  1. +10 −8 proxygen/lib/http/codec/HTTP2Codec.cpp
  2. +24 −0 proxygen/lib/http/codec/test/HTTP2CodecTest.cpp
@@ -514,21 +514,23 @@ folly::Optional<ErrorCode> HTTP2Codec::parseHeadersDecodeFrames(
isReq = transportDirection_ == TransportDirection::DOWNSTREAM;
}

// Validate circular dependencies.
if (priority && (curHeader_.stream == priority->streamDependency)) {
streamError(
folly::to<string>("Circular dependency for txn=", curHeader_.stream),
ErrorCode::PROTOCOL_ERROR,
curHeader_.type == http2::FrameType::HEADERS);
return ErrorCode::NO_ERROR;
}

decodeInfo_.init(isReq, parsingDownstreamTrailers_);
if (priority) {
if (curHeader_.stream == priority->streamDependency) {
streamError(folly::to<string>("Circular dependency for txn=",
curHeader_.stream),
ErrorCode::PROTOCOL_ERROR,
curHeader_.type == http2::FrameType::HEADERS);
return ErrorCode::NO_ERROR;
}

decodeInfo_.msg->setHTTP2Priority(
std::make_tuple(priority->streamDependency,
priority->exclusive,
priority->weight));
}

headerCodec_.decodeStreaming(
headerCursor, curHeaderBlock_.chainLength(), this);
msg = std::move(decodeInfo_.msg);
@@ -1377,6 +1377,30 @@ TEST_F(HTTP2CodecTest, BadHeaderPriority) {
EXPECT_EQ(callbacks_.sessionErrors, 0);
}

TEST_F(HTTP2CodecTest, DuplicateBadHeaderPriority) {
// Sent an initial header with a circular dependency
HTTPMessage req = getGetRequest();
req.setHTTP2Priority(HTTPMessage::HTTPPriority(0, false, 7));
upstreamCodec_.generateHeader(output_, 1, req, true /* eom */);

// Hack ingress with circular dependency.
EXPECT_TRUE(parse([&](IOBuf* ingress) {
folly::io::RWPrivateCursor c(ingress);
c.skip(http2::kFrameHeaderSize + http2::kConnectionPreface.length());
c.writeBE<uint32_t>(1);
}));

EXPECT_EQ(callbacks_.streamErrors, 1);
EXPECT_EQ(callbacks_.sessionErrors, 0);

// On the same stream, send another request.
HTTPMessage nextRequest = getGetRequest();
upstreamCodec_.generateHeader(output_, 1, nextRequest, true /* eom */);
parse();
EXPECT_EQ(callbacks_.streamErrors, 2);
EXPECT_EQ(callbacks_.sessionErrors, 0);
}

TEST_F(HTTP2CodecTest, BadPriority) {
auto pri = HTTPMessage::HTTPPriority(0, true, 1);
upstreamCodec_.generatePriority(output_, 1, pri);

0 comments on commit 52cf331

Please sign in to comment.