Skip to content

Commit

Permalink
Fix TCP option size calculation
Browse files Browse the repository at this point in the history
From https://www.iana.org/assignments/tcp-parameters/tcp-parameters.xhtml :

    > Options 0 and 1 are exactly one octet which is their kind field.  All
    > other options have their one octet kind field, followed by a one octet
    > length field, followed by length-2 octets of option data.

Options affected by the libtins bug are those where length is listed as:

* 2, except for SACK permitted, for which tins has an explicit workaround, or
* N, for packets where the options payload happens to be empty (N==2)

Without this fix, any affected packet can still be parsed, but trying
to serialize the resulting PDU object again will fail in
Tins::PDU::serialize -> Tins::TCP::write_serialization
-> Tins::Memory::OutputMemoryStream::fill.
  • Loading branch information
fxkr committed Jul 8, 2020
1 parent ce409db commit e831725
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 2 deletions.
1 change: 1 addition & 0 deletions include/tins/tcp.h
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ class TINS_API TCP : public PDU {
SACK = 5,
TSOPT = 8,
ALTCHK = 14,
SCPS_CAPABILITIES = 20,
RFC_EXPERIMENT_1 = 253,
RFC_EXPERIMENT_2 = 254
};
Expand Down
6 changes: 4 additions & 2 deletions src/tcp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -385,8 +385,10 @@ uint32_t TCP::calculate_options_size() const {
for (options_type::const_iterator iter = options_.begin(); iter != options_.end(); ++iter) {
const option& opt = *iter;
options_size += sizeof(uint8_t);
// SACK_OK contains length but not data
if (opt.data_size() || opt.option() == SACK_OK) {
// Options 0 and 1 are exactly one octet which is their kind field.
// All other options have their one octet kind field,
// followed by a one octet length field (even if the option payload is empty)
if (opt.option() != 0 && opt.option() != 1) {
options_size += sizeof(uint8_t);
options_size += static_cast<uint16_t>(opt.data_size());
}
Expand Down
11 changes: 11 additions & 0 deletions tests/src/tcp_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,17 @@ TEST_F(TCPTest, SpoofedOptions) {
EXPECT_EQ(pdu.serialize().size(), pdu.size());
}

TEST_F(TCPTest, OptionWithEmptyPayload) {
TCP pdu;
uint8_t a[] = { };
// it has a payload, it's just empty (unlike EOL/NOP options which have no payload)
pdu.add_option(
TCP::option(TCP::SCPS_CAPABILITIES, 2, a, a + sizeof(a))
);
EXPECT_EQ(1U, pdu.options().size());
EXPECT_EQ(pdu.serialize().size(), pdu.size());
}

TEST_F(TCPTest, MalformedOptionAfterEOL) {
TCP tcp(malformed_option_after_eol_packet, sizeof(malformed_option_after_eol_packet));
EXPECT_EQ(0U, tcp.options().size());
Expand Down

0 comments on commit e831725

Please sign in to comment.