From 8366454cf9cffb63152ca5d750bb443b999800d2 Mon Sep 17 00:00:00 2001
From: Felix Kaiser <felix.kaiser@fxkr.net>
Date: Tue, 7 Jul 2020 19:45:06 -0700
Subject: [PATCH] Fix TCP option size calculation

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.
---
 include/tins/tcp.h     | 1 +
 src/tcp.cpp            | 6 ++++--
 tests/src/tcp_test.cpp | 8 ++++++++
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/tins/tcp.h b/include/tins/tcp.h
index 7e906c5e..65027ef9 100644
--- a/include/tins/tcp.h
+++ b/include/tins/tcp.h
@@ -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
     };
diff --git a/src/tcp.cpp b/src/tcp.cpp
index 81507f0e..e173601f 100644
--- a/src/tcp.cpp
+++ b/src/tcp.cpp
@@ -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());
         }
diff --git a/tests/src/tcp_test.cpp b/tests/src/tcp_test.cpp
index 4a6318df..48586b38 100644
--- a/tests/src/tcp_test.cpp
+++ b/tests/src/tcp_test.cpp
@@ -277,6 +277,14 @@ TEST_F(TCPTest, SpoofedOptions) {
     EXPECT_EQ(pdu.serialize().size(), pdu.size());
 }
 
+TEST_F(TCPTest, OptionWithEmptyPayload) {
+    TCP pdu;
+    // 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));
+    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());