From 02d3a140839f3098c8db115e54896046b5a40994 Mon Sep 17 00:00:00 2001 From: Santiago Alessandri Date: Fri, 12 Jul 2013 20:06:55 -0300 Subject: [PATCH 1/2] Fixed issue related with EthernetII protocol. The Ethernet II protocol forces a minimum length of 60 bytes. It is necessary to add a trailer for padding of null-bytes when the inner_pdu's size does not meet the requirement. Also EthernetII packets received were being incorrectly parsed due to the existance of this padding. The issue has been solved and several tests were added. All tests successfully pass. --- include/ethernetII.h | 8 ++++++++ src/arp.cpp | 1 + src/eapol.cpp | 1 + src/ethernetII.cpp | 21 ++++++++++++++++++++- src/ip.cpp | 3 +++ src/pppoe.cpp | 1 + tests/src/ethernetII.cpp | 30 ++++++++++++++++++++++++++++-- tests/src/pppoe.cpp | 14 ++++++++++++++ 8 files changed, 76 insertions(+), 3 deletions(-) diff --git a/include/ethernetII.h b/include/ethernetII.h index ba7cf22..e8e55bc 100644 --- a/include/ethernetII.h +++ b/include/ethernetII.h @@ -136,6 +136,14 @@ namespace Tins { */ uint32_t header_size() const; + /** + * \brief Returns the ethernet II frame's padding. + * + * \return An uint32_t with the padding size. + * \sa PDU::trailer_size() + */ + uint32_t trailer_size() const; + // Windows does not support sending L2 PDUs. #ifndef WIN32 /** diff --git a/src/arp.cpp b/src/arp.cpp index 02bf1bc..1aebb88 100644 --- a/src/arp.cpp +++ b/src/arp.cpp @@ -63,6 +63,7 @@ ARP::ARP(const uint8_t *buffer, uint32_t total_sz) throw malformed_packet(); memcpy(&_arp, buffer, sizeof(arphdr)); total_sz -= sizeof(arphdr); + //TODO: Check whether this should be removed or not. if(total_sz) inner_pdu(new RawPDU(buffer + sizeof(arphdr), total_sz)); } diff --git a/src/eapol.cpp b/src/eapol.cpp index ffd0bf6..2601732 100644 --- a/src/eapol.cpp +++ b/src/eapol.cpp @@ -56,6 +56,7 @@ EAPOL *EAPOL::from_bytes(const uint8_t *buffer, uint32_t total_sz) { if(total_sz < sizeof(eapolhdr)) throw malformed_packet(); const eapolhdr *ptr = (const eapolhdr*)buffer; + total_sz = std::min(total_sz, (uint32_t)ptr->length); switch(ptr->type) { case RC4: return new Tins::RC4EAPOL(buffer, total_sz); diff --git a/src/ethernetII.cpp b/src/ethernetII.cpp index e155be8..0f46ed9 100644 --- a/src/ethernetII.cpp +++ b/src/ethernetII.cpp @@ -82,6 +82,7 @@ EthernetII::EthernetII(const uint8_t *buffer, uint32_t total_sz) ) ); } + } void EthernetII::dst_addr(const address_type &new_dst_addr) { @@ -97,9 +98,19 @@ void EthernetII::payload_type(uint16_t new_payload_type) { } uint32_t EthernetII::header_size() const { + return sizeof(ethhdr); } +uint32_t EthernetII::trailer_size() const { + int32_t padding = 60 - sizeof(ethhdr); // EthernetII min size is 60, padding is sometimes needed + if (inner_pdu()) { + padding -= inner_pdu()->size(); + padding = std::max(0, padding); + } + return padding; +} + #ifndef WIN32 void EthernetII::send(PacketSender &sender, const NetworkInterface &iface) { if(!iface) @@ -140,7 +151,7 @@ bool EthernetII::matches_response(const uint8_t *ptr, uint32_t total_sz) const { void EthernetII::write_serialization(uint8_t *buffer, uint32_t total_sz, const PDU *parent) { #ifdef TINS_DEBUG - assert(total_sz >= header_size()); + assert(total_sz >= header_size() + trailer_size()); #endif /* Inner type defaults to IP */ @@ -151,6 +162,14 @@ void EthernetII::write_serialization(uint8_t *buffer, uint32_t total_sz, const P payload_type(static_cast(flag)); } memcpy(buffer, &_eth, sizeof(ethhdr)); + uint32_t trailer = trailer_size(); + if (trailer) { + uint32_t trailer_offset = header_size(); + if (inner_pdu()) + trailer_offset += inner_pdu()->size(); + memset(buffer + trailer_offset, 0, trailer); + } + } #ifndef WIN32 diff --git a/src/ip.cpp b/src/ip.cpp index 503b849..10dc611 100644 --- a/src/ip.cpp +++ b/src/ip.cpp @@ -115,6 +115,9 @@ IP::IP(const uint8_t *buffer, uint32_t total_sz) uint8_t padding = _options_size % 4; _padded_options_size = padding ? (_options_size - padding + 4) : _options_size; // check this line PLX + total_sz = std::min(total_sz, (uint32_t)tot_len()); + if (total_sz < head_len() * sizeof(uint32_t)) + throw malformed_packet(); total_sz -= head_len() * sizeof(uint32_t); if (total_sz) { switch(_ip.protocol) { diff --git a/src/pppoe.cpp b/src/pppoe.cpp index 3cb23a8..d2bfe0f 100644 --- a/src/pppoe.cpp +++ b/src/pppoe.cpp @@ -51,6 +51,7 @@ PPPoE::PPPoE(const uint8_t *buffer, uint32_t total_sz) std::memcpy(&_header, buffer, sizeof(_header)); buffer += sizeof(_header); total_sz -= sizeof(_header); + total_sz = std::min(total_sz, (uint32_t)payload_length()); const uint8_t *end = buffer + total_sz; while(buffer < end) { if(buffer + sizeof(uint32_t) * 2 > end) diff --git a/tests/src/ethernetII.cpp b/tests/src/ethernetII.cpp index 0d6fabc..3cfb5c4 100644 --- a/tests/src/ethernetII.cpp +++ b/tests/src/ethernetII.cpp @@ -5,6 +5,8 @@ #include "macros.h" #include "ipv6.h" #include "ip.h" +#include "tcp.h" +#include "rawpdu.h" using namespace Tins; @@ -12,7 +14,7 @@ typedef EthernetII::address_type address_type; class EthernetIITest : public ::testing::Test { public: - static const uint8_t expected_packet[], ip_packet[], ipv6_packet[]; + static const uint8_t expected_packet[], ip_packet[], ipv6_packet[], smallip_packet[]; static address_type src_addr; static address_type dst_addr; static address_type empty_addr; @@ -22,7 +24,10 @@ public: }; const uint8_t EthernetIITest::expected_packet[] = { - 170, 187, 204, 221, 238, 255, 138, 139, 140, 141, 142, 143, 208, 171 + 170, 187, 204, 221, 238, 255, 138, 139, 140, 141, 142, 143, 208, 171, + 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, + 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, + 00, 00, 00, 00, 00, 00, 00, 00, 00, 00 }, EthernetIITest::ip_packet[] = { 255, 255, 255, 255, 255, 255, 0, 0, 0, 0, 0, 0, 8, 0, 69, 0, 0, 20, @@ -32,6 +37,12 @@ EthernetIITest::ipv6_packet[] = { 255, 255, 255, 255, 255, 255, 0, 0, 0, 0, 0, 0, 134, 221, 96, 0, 0, 0, 0, 0, 59, 64, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1 +}, +EthernetIITest::smallip_packet[] = { + 64, 97, 134, 43, 174, 3, 0, 36, 1, 254, 210, 68, 8, 0, 69, 0, 0, 40, + 53, 163, 64, 0, 127, 6, 44, 53, 192, 168, 1, 120, 173, 194, 42, 21, + 163, 42, 1, 187, 162, 113, 212, 162, 132, 15, 66, 219, 80, 16, 16, + 194, 34, 54, 0, 0, 0, 0, 0, 0, 0, 0 }; address_type EthernetIITest::src_addr("8a:8b:8c:8d:8e:8f"); @@ -113,6 +124,14 @@ TEST_F(EthernetIITest, Serialize) { EXPECT_TRUE(std::equal(serialized.begin(), serialized.end(), expected_packet)); } +TEST_F(EthernetIITest, SerializeSmallEthernetWithPadding) { + EthernetII eth(smallip_packet, sizeof(smallip_packet)); + ASSERT_TRUE(eth.inner_pdu()); + PDU::serialization_type serialized = eth.serialize(); + EXPECT_EQ(serialized.size(), sizeof(smallip_packet)); + EXPECT_TRUE(std::equal(serialized.begin(), serialized.end(), smallip_packet)); +} + TEST_F(EthernetIITest, ConstructorFromBuffer) { EthernetII eth(expected_packet, sizeof(expected_packet)); EXPECT_EQ(eth.src_addr(), src_addr); @@ -132,3 +151,10 @@ TEST_F(EthernetIITest, ConstructorFromIPv6Buffer) { EXPECT_EQ(eth.find_pdu(), eth.inner_pdu()); } +TEST_F(EthernetIITest, EliminateEthernetPadding) { + EthernetII eth(smallip_packet, sizeof(smallip_packet)); + ASSERT_TRUE(eth.inner_pdu()); + ASSERT_TRUE(eth.find_pdu()); + ASSERT_TRUE(eth.find_pdu()); + ASSERT_FALSE(eth.find_pdu()); +} diff --git a/tests/src/pppoe.cpp b/tests/src/pppoe.cpp index 33ad1d9..4aaa429 100644 --- a/tests/src/pppoe.cpp +++ b/tests/src/pppoe.cpp @@ -46,6 +46,20 @@ TEST_F(PPPoETest, StackedOnEthernet) { ASSERT_TRUE(eth2.find_pdu()); } +TEST_F(PPPoETest, StackedOnEthernetSerializationWithTags) { + PPPoE pdu(expected_packet, sizeof(expected_packet)); + EthernetII eth = EthernetII() / pdu; + PDU::serialization_type buffer = eth.serialize(); + EthernetII eth2(&buffer[0], buffer.size()); + PPPoE* unserialized = eth2.find_pdu(); + ASSERT_TRUE(unserialized); + EXPECT_EQ( + PPPoE::serialization_type(expected_packet, expected_packet + sizeof(expected_packet)), + unserialized->serialize() + ); + +} + TEST_F(PPPoETest, Serialize) { PPPoE pdu(expected_packet, sizeof(expected_packet)); PPPoE::serialization_type buffer = pdu.serialize(); From 2a5b64526f3ad6566151a1fe306d349c63476c92 Mon Sep 17 00:00:00 2001 From: Santiago Alessandri Date: Sun, 14 Jul 2013 13:13:38 -0300 Subject: [PATCH 2/2] Replaced tabs for spaces in the previous commit --- src/ethernetII.cpp | 20 ++++++++++---------- src/ip.cpp | 2 +- tests/src/ethernetII.cpp | 28 ++++++++++++++-------------- tests/src/pppoe.cpp | 6 +++--- 4 files changed, 28 insertions(+), 28 deletions(-) diff --git a/src/ethernetII.cpp b/src/ethernetII.cpp index 0f46ed9..5d28f79 100644 --- a/src/ethernetII.cpp +++ b/src/ethernetII.cpp @@ -103,12 +103,12 @@ uint32_t EthernetII::header_size() const { } uint32_t EthernetII::trailer_size() const { - int32_t padding = 60 - sizeof(ethhdr); // EthernetII min size is 60, padding is sometimes needed - if (inner_pdu()) { - padding -= inner_pdu()->size(); - padding = std::max(0, padding); - } - return padding; + int32_t padding = 60 - sizeof(ethhdr); // EthernetII min size is 60, padding is sometimes needed + if (inner_pdu()) { + padding -= inner_pdu()->size(); + padding = std::max(0, padding); + } + return padding; } #ifndef WIN32 @@ -164,10 +164,10 @@ void EthernetII::write_serialization(uint8_t *buffer, uint32_t total_sz, const P memcpy(buffer, &_eth, sizeof(ethhdr)); uint32_t trailer = trailer_size(); if (trailer) { - uint32_t trailer_offset = header_size(); - if (inner_pdu()) - trailer_offset += inner_pdu()->size(); - memset(buffer + trailer_offset, 0, trailer); + uint32_t trailer_offset = header_size(); + if (inner_pdu()) + trailer_offset += inner_pdu()->size(); + memset(buffer + trailer_offset, 0, trailer); } } diff --git a/src/ip.cpp b/src/ip.cpp index 10dc611..e0acf09 100644 --- a/src/ip.cpp +++ b/src/ip.cpp @@ -117,7 +117,7 @@ IP::IP(const uint8_t *buffer, uint32_t total_sz) // check this line PLX total_sz = std::min(total_sz, (uint32_t)tot_len()); if (total_sz < head_len() * sizeof(uint32_t)) - throw malformed_packet(); + throw malformed_packet(); total_sz -= head_len() * sizeof(uint32_t); if (total_sz) { switch(_ip.protocol) { diff --git a/tests/src/ethernetII.cpp b/tests/src/ethernetII.cpp index 3cfb5c4..64b736b 100644 --- a/tests/src/ethernetII.cpp +++ b/tests/src/ethernetII.cpp @@ -39,10 +39,10 @@ EthernetIITest::ipv6_packet[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1 }, EthernetIITest::smallip_packet[] = { - 64, 97, 134, 43, 174, 3, 0, 36, 1, 254, 210, 68, 8, 0, 69, 0, 0, 40, - 53, 163, 64, 0, 127, 6, 44, 53, 192, 168, 1, 120, 173, 194, 42, 21, - 163, 42, 1, 187, 162, 113, 212, 162, 132, 15, 66, 219, 80, 16, 16, - 194, 34, 54, 0, 0, 0, 0, 0, 0, 0, 0 + 64, 97, 134, 43, 174, 3, 0, 36, 1, 254, 210, 68, 8, 0, 69, 0, 0, 40, + 53, 163, 64, 0, 127, 6, 44, 53, 192, 168, 1, 120, 173, 194, 42, 21, + 163, 42, 1, 187, 162, 113, 212, 162, 132, 15, 66, 219, 80, 16, 16, + 194, 34, 54, 0, 0, 0, 0, 0, 0, 0, 0 }; address_type EthernetIITest::src_addr("8a:8b:8c:8d:8e:8f"); @@ -125,11 +125,11 @@ TEST_F(EthernetIITest, Serialize) { } TEST_F(EthernetIITest, SerializeSmallEthernetWithPadding) { - EthernetII eth(smallip_packet, sizeof(smallip_packet)); - ASSERT_TRUE(eth.inner_pdu()); - PDU::serialization_type serialized = eth.serialize(); - EXPECT_EQ(serialized.size(), sizeof(smallip_packet)); - EXPECT_TRUE(std::equal(serialized.begin(), serialized.end(), smallip_packet)); + EthernetII eth(smallip_packet, sizeof(smallip_packet)); + ASSERT_TRUE(eth.inner_pdu()); + PDU::serialization_type serialized = eth.serialize(); + EXPECT_EQ(serialized.size(), sizeof(smallip_packet)); + EXPECT_TRUE(std::equal(serialized.begin(), serialized.end(), smallip_packet)); } TEST_F(EthernetIITest, ConstructorFromBuffer) { @@ -152,9 +152,9 @@ TEST_F(EthernetIITest, ConstructorFromIPv6Buffer) { } TEST_F(EthernetIITest, EliminateEthernetPadding) { - EthernetII eth(smallip_packet, sizeof(smallip_packet)); - ASSERT_TRUE(eth.inner_pdu()); - ASSERT_TRUE(eth.find_pdu()); - ASSERT_TRUE(eth.find_pdu()); - ASSERT_FALSE(eth.find_pdu()); + EthernetII eth(smallip_packet, sizeof(smallip_packet)); + ASSERT_TRUE(eth.inner_pdu()); + ASSERT_TRUE(eth.find_pdu()); + ASSERT_TRUE(eth.find_pdu()); + ASSERT_FALSE(eth.find_pdu()); } diff --git a/tests/src/pppoe.cpp b/tests/src/pppoe.cpp index 4aaa429..83095fd 100644 --- a/tests/src/pppoe.cpp +++ b/tests/src/pppoe.cpp @@ -48,14 +48,14 @@ TEST_F(PPPoETest, StackedOnEthernet) { TEST_F(PPPoETest, StackedOnEthernetSerializationWithTags) { PPPoE pdu(expected_packet, sizeof(expected_packet)); - EthernetII eth = EthernetII() / pdu; + EthernetII eth = EthernetII() / pdu; PDU::serialization_type buffer = eth.serialize(); EthernetII eth2(&buffer[0], buffer.size()); PPPoE* unserialized = eth2.find_pdu(); ASSERT_TRUE(unserialized); EXPECT_EQ( - PPPoE::serialization_type(expected_packet, expected_packet + sizeof(expected_packet)), - unserialized->serialize() + PPPoE::serialization_type(expected_packet, expected_packet + sizeof(expected_packet)), + unserialized->serialize() ); }