Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ message ThriftProxy {

// The Thrift proxy will assume the client is using the Thrift unframed transport.
UNFRAMED = 2;

// The Thrift proxy will assume the client is using the Thrift header transport.
HEADER = 3;
}

// Supplies the type of transport that the Thrift proxy should use. Defaults to `AUTO_TRANSPORT`.
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/filters/network/thrift_proxy/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -187,11 +187,13 @@ envoy_cc_library(
name = "transport_lib",
srcs = [
"framed_transport_impl.cc",
"header_transport_impl.cc",
"transport_impl.cc",
"unframed_transport_impl.cc",
],
hdrs = [
"framed_transport_impl.h",
"header_transport_impl.h",
"transport_impl.h",
"unframed_transport_impl.h",
],
Expand Down
20 changes: 18 additions & 2 deletions source/extensions/filters/network/thrift_proxy/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ static const TransportTypeMap& transportTypeMap() {
{envoy::config::filter::network::thrift_proxy::v2alpha1::
ThriftProxy_TransportType_UNFRAMED,
TransportType::Unframed},
{envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy_TransportType_HEADER,
TransportType::Header},
});
}

Expand Down Expand Up @@ -92,6 +94,20 @@ ConfigImpl::ConfigImpl(
transport_(config.transport()), proto_(config.protocol()),
route_matcher_(new Router::RouteMatcher(config.route_config())) {

if (transportTypeMap().find(transport_) == transportTypeMap().end()) {
throw EnvoyException(fmt::format(
"unknown transport {}",
envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy_TransportType_Name(
transport_)));
}

if (protocolTypeMap().find(proto_) == protocolTypeMap().end()) {
throw EnvoyException(fmt::format(
"unknown protocol {}",
envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy_ProtocolType_Name(
proto_)));
}

// Construct the only Thrift DecoderFilter: the Router
auto& factory =
Envoy::Config::Utility::getAndCheckFactory<ThriftFilters::NamedThriftFilterConfigFactory>(
Expand All @@ -115,14 +131,14 @@ DecoderPtr ConfigImpl::createDecoder(DecoderCallbacks& callbacks) {

TransportPtr ConfigImpl::createTransport() {
TransportTypeMap::const_iterator i = transportTypeMap().find(transport_);
RELEASE_ASSERT(i != transportTypeMap().end(), "invalid transport type");
ASSERT(i != transportTypeMap().end());

return NamedTransportConfigFactory::getFactory(i->second).createTransport();
}

ProtocolPtr ConfigImpl::createProtocol() {
ProtocolTypeMap::const_iterator i = protocolTypeMap().find(proto_);
RELEASE_ASSERT(i != protocolTypeMap().end(), "invalid protocol type");
ASSERT(i != protocolTypeMap().end());
return NamedProtocolConfigFactory::getFactory(i->second).createProtocol();
}

Expand Down
19 changes: 19 additions & 0 deletions source/extensions/filters/network/thrift_proxy/decoder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,25 @@ ThriftFilters::FilterStatus Decoder::onData(Buffer::Instance& data, bool& buffer
}
ENVOY_LOG(debug, "thrift: {} transport started", transport_->name());

if (metadata_->hasProtocol()) {
if (protocol_->type() == ProtocolType::Auto) {
protocol_->setType(metadata_->protocol());
ENVOY_LOG(debug, "thrift: {} transport forced {} protocol", transport_->name(),
protocol_->name());
} else if (metadata_->protocol() != protocol_->type()) {
throw EnvoyException(fmt::format("transport reports protocol {}, but configured for {}",
ProtocolNames::get().fromType(metadata_->protocol()),
ProtocolNames::get().fromType(protocol_->type())));
}
}
if (metadata_->hasAppException()) {
AppExceptionType ex_type = metadata_->appExceptionType();
std::string ex_msg = metadata_->appExceptionMessage();
// Force new metadata if we get called again.
metadata_.reset();
throw AppException(ex_type, ex_msg);
}

request_ = std::make_unique<ActiveRequest>(callbacks_.newDecoderFilter());
frame_started_ = true;
state_machine_ =
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,299 @@
#include "extensions/filters/network/thrift_proxy/header_transport_impl.h"

#include <limits>

#include "envoy/common/exception.h"

#include "common/buffer/buffer_impl.h"

#include "extensions/filters/network/thrift_proxy/buffer_helper.h"
#include "extensions/filters/network/thrift_proxy/transport_impl.h"

namespace Envoy {
namespace Extensions {
namespace NetworkFilters {
namespace ThriftProxy {
namespace {

// c.f.
// https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/protocol/TProtocolTypes.h#L27
enum class HeaderProtocolType {
Binary = 0,
JSON = 1,
Compact = 2,

FirstHeaderProtocolType = Binary,
LastHeaderProtocolType = Compact,
};

} // namespace

bool HeaderTransportImpl::decodeFrameStart(Buffer::Instance& buffer, MessageMetadata& metadata) {
if (buffer.length() < 14) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should this be a constant (e.g.: HEADER_LEN)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some constants for the various sizes this transport uses.

return false;
}

// Size of frame, not including the length bytes.
const int32_t frame_size = BufferHelper::peekI32(buffer);

// Minimum header frame size is 18 bytes (4 bytes of frame size + 10 bytes of fixed header +
// minimum 4 bytes of variable header data), so frame_size must be at least 14.
if (frame_size < 14 || frame_size > MaxFrameSize) {
throw EnvoyException(fmt::format("invalid thrift header transport frame size {}", frame_size));
}

int16_t magic = BufferHelper::peekU16(buffer, 4);
if (!isMagic(magic)) {
throw EnvoyException(fmt::format("invalid thrift header transport magic {:04x}", magic));
}

// offset 6: 16 bit flags field, unused
// offset 8: 32 bit sequence number field
int32_t seq_id = BufferHelper::peekI32(buffer, 8);

// offset 12: 16 bit (remaining) header size / 4 (spec erroneously claims / 32).
int16_t raw_header_size = BufferHelper::peekI16(buffer, 12);
int32_t header_size = static_cast<int32_t>(raw_header_size) * 4;
if (header_size < 0 || header_size > MaxHeadersSize) {
throw EnvoyException(fmt::format("invalid thrift header transport header size {} ({:04x})",
header_size, static_cast<uint16_t>(raw_header_size)));
}

if (header_size == 0) {
throw EnvoyException("no header data");
}

if (buffer.length() < static_cast<uint64_t>(header_size) + 14) {
// Need more header data.
return false;
}

// Header data starts at offset 14 (4 bytes of frame size followed by 10 bytes of fixed header).
buffer.drain(14);

// Remaining frame size is the original frame size (which does not count itself), less the 10
// fixed bytes of the header (magic, flags, etc), less the size of the variable header data
// (header_size).
metadata.setFrameSize(static_cast<uint32_t>(frame_size - header_size - 10));
metadata.setSequenceId(seq_id);

ProtocolType proto = ProtocolType::Auto;
HeaderProtocolType header_proto =
static_cast<HeaderProtocolType>(drainVarIntI16(buffer, header_size, "protocol id"));
switch (header_proto) {
case HeaderProtocolType::Binary:
proto = ProtocolType::Binary;
break;
case HeaderProtocolType::Compact:
proto = ProtocolType::Compact;
break;
default:
throw EnvoyException(fmt::format("Unknown protocol {}", static_cast<int>(header_proto)));
}
metadata.setProtocol(proto);

int16_t num_xforms = drainVarIntI16(buffer, header_size, "transform count");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line vulnerable to a buffer read overflow? Here's the scenario I'm worried about:

  • A malicious client sends a 15-byte message where header_size = 1
  • This message gets through all the checks on lines 57-66 above.
  • Then line 72 removes the first 14 bytes, leaving 1 byte in buffer.
  • Depending on the value in that 1 remaining byte, drainVarInt16 may try to read the byte after it, which is beyond the end of buffer.

@zuercher zuercher Aug 10, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe so. We require that the full header (as given by header size) by available. As we read bytes from the header we decrement header_size (which is passed as a reference to drainVarIntI16). drainVarIntI16 is just drainVarIntI32 with an upper bound check. The latter requires remaining header_size to be at least 1 and then invokes BufferHelper::peekVarIntI32 which validates the buffer has enough data before reading each byte that composes the value. If the peek reports a buffer underflow (buffer doesn't have enough data to read the whole number) or requires more bytes than are left in the header (supposing the buffer has more data beyond the end of the header) we throw.

That said, I'll add a test that exercises this directly. Mostly as a remdinder to myself: It's basically this one except that the number of transforms should be truncated.

if (num_xforms < 0) {
throw EnvoyException(fmt::format("invalid header transport transform count {}", num_xforms));
}

while (num_xforms-- > 0) {
int32_t xform_id = drainVarIntI32(buffer, header_size, "transform id");

// To date, no transforms have a data field. In the future, some transform IDs may require
// consuming another varint 32 at this point. The known transform IDs are:
// 1: zlib compression
// 2: hmac (appended to end of packet)
// 3: snappy compression
buffer.drain(header_size);
metadata.setAppException(AppExceptionType::MissingResult,
fmt::format("Unknown transform {}", xform_id));
return true;
}

while (header_size > 0) {
// Attempt to read info blocks
int32_t info_id = drainVarIntI32(buffer, header_size, "info id");
if (info_id != 1) {
// 0 indicates a padding byte, and the end of the info block.
// 1 indicates an info id header/value pair.
// Any other value is an unknown info id block, which we ignore.
break;
}

int32_t num_headers = drainVarIntI32(buffer, header_size, "header count");
if (num_headers < 0) {
throw EnvoyException(fmt::format("invalid header transport header count {}", num_headers));
}

while (num_headers-- > 0) {
std::string key = drainVarString(buffer, header_size, "header key");
std::string value = drainVarString(buffer, header_size, "header value");
metadata.addHeader(Header(key, value));
}
}

// Remaining bytes are padding or ignored info blocks.
if (header_size > 0) {
buffer.drain(header_size);
}

return true;
}

bool HeaderTransportImpl::decodeFrameEnd(Buffer::Instance&) {
exception_.reset();
exception_reason_.clear();

return true;
}

void HeaderTransportImpl::encodeFrame(Buffer::Instance& buffer, const MessageMetadata& metadata,
Buffer::Instance& message) {
uint64_t msg_size = message.length();
if (msg_size == 0) {
throw EnvoyException(fmt::format("invalid thrift header transport message size {}", msg_size));
}

const HeaderMap& headers = metadata.headers();
if (headers.size() > MaxHeadersSize / 2) {
// Each header takes a minimum of 2 bytes, yielding this limit.
throw EnvoyException(
fmt::format("invalid thrift header transport too many headers {}", headers.size()));
}

Buffer::OwnedImpl header_buffer;

if (!metadata.hasProtocol()) {
throw EnvoyException("missing header transport protocol");
}

switch (metadata.protocol()) {
case ProtocolType::Binary:
BufferHelper::writeVarIntI32(header_buffer, static_cast<int32_t>(HeaderProtocolType::Binary));
break;
case ProtocolType::Compact:
BufferHelper::writeVarIntI32(header_buffer, static_cast<int32_t>(HeaderProtocolType::Compact));
break;
default:
throw EnvoyException(fmt::format("invalid header transport protocol {}",
ProtocolNames::get().fromType(metadata.protocol())));
}

BufferHelper::writeVarIntI32(header_buffer, 0); // num transforms
if (headers.size() > 0) {
// Info ID 1
BufferHelper::writeI8(header_buffer, 1);

// Num headers
BufferHelper::writeVarIntI32(header_buffer, static_cast<int32_t>(headers.size()));

for (const Header& header : headers) {
writeVarString(header_buffer, header.key());
writeVarString(header_buffer, header.value());
}
}

uint64_t header_size = header_buffer.length();

// Always pad (as the Apache implementation does).
int padding = 4 - (header_size % 4);
header_buffer.add("\0\0\0\0", padding);
header_size += padding;

if (header_size > MaxHeadersSize) {
throw EnvoyException(
fmt::format("invalid thrift header transport header size {}", header_size));
}

// Frame size does not include the frame length itself.
uint64_t size = header_size + msg_size + 10;
if (size > MaxFrameSize) {
throw EnvoyException(fmt::format("invalid thrift header transport frame size {}", size));
}

int32_t seq_id = 0;
if (metadata.hasSequenceId()) {
seq_id = metadata.sequenceId();
}

BufferHelper::writeU32(buffer, static_cast<uint32_t>(size));
BufferHelper::writeU16(buffer, Magic);
BufferHelper::writeU16(buffer, 0); // flags
BufferHelper::writeI32(buffer, seq_id);
BufferHelper::writeU16(buffer, static_cast<uint16_t>(header_size / 4));
buffer.move(header_buffer);
buffer.move(message);
}

int16_t HeaderTransportImpl::drainVarIntI16(Buffer::Instance& buffer, int32_t& header_size,
const char* desc) {
int32_t value = drainVarIntI32(buffer, header_size, desc);
if (value > static_cast<int32_t>(std::numeric_limits<int16_t>::max())) {
throw EnvoyException(fmt::format("header transport {}: value {} exceeds max i16 ({})", desc,
value, std::numeric_limits<int16_t>::max()));
}
return static_cast<int16_t>(value);
}

int32_t HeaderTransportImpl::drainVarIntI32(Buffer::Instance& buffer, int32_t& header_size,
const char* desc) {
if (header_size <= 0) {
throw EnvoyException(fmt::format("unable to read header transport {}: header too small", desc));
}

int size;
int32_t value = BufferHelper::peekVarIntI32(buffer, 0, size);
if (size < 0 || (header_size - size) < 0) {
throw EnvoyException(fmt::format("unable to read header transport {}: header too small", desc));
}
buffer.drain(size);
header_size -= size;
return value;
}

std::string HeaderTransportImpl::drainVarString(Buffer::Instance& buffer, int32_t& header_size,
const char* desc) {
int16_t str_len = drainVarIntI16(buffer, header_size, desc);
if (str_len == 0) {
return "";
}

if (header_size < static_cast<int32_t>(str_len)) {
throw EnvoyException(fmt::format("unable to read header transport {}: header too small", desc));
}

std::string value(static_cast<char*>(buffer.linearize(str_len)), str_len);
buffer.drain(str_len);
header_size -= str_len;
return value;
}

void HeaderTransportImpl::writeVarString(Buffer::Instance& buffer, const std::string& str) {
std::string::size_type len = str.length();
if (len > static_cast<uint32_t>(std::numeric_limits<int16_t>::max())) {
throw EnvoyException(fmt::format("header string too long: {}", len));
}

BufferHelper::writeVarIntI32(buffer, static_cast<int32_t>(len));
if (len == 0) {
return;
}
buffer.add(str);
}

class HeaderTransportConfigFactory : public TransportFactoryBase<HeaderTransportImpl> {
public:
HeaderTransportConfigFactory() : TransportFactoryBase(TransportNames::get().HEADER) {}
};

/**
* Static registration for the header transport. @see RegisterFactory.
*/
static Registry::RegisterFactory<HeaderTransportConfigFactory, NamedTransportConfigFactory>
register_;

} // namespace ThriftProxy
} // namespace NetworkFilters
} // namespace Extensions
} // namespace Envoy
Loading