diff --git a/Packet++/header/IcmpLayer.h b/Packet++/header/IcmpLayer.h index 940aa8ce3d..0e14299a1c 100644 --- a/Packet++/header/IcmpLayer.h +++ b/Packet++/header/IcmpLayer.h @@ -118,12 +118,17 @@ namespace pcpp uint16_t id; /// the echo (ping) request sequence number uint16_t sequence; - /// a timestamp of when the message was sent + /// a timestamp of when the message was sent. Not part of the RFC 792 echo header, these are the first 8 + /// bytes of the echo payload, and they are absent from a message shorter than sizeof(icmp_echo_hdr) uint64_t timestamp; } icmp_echo_hdr; #pragma pack(pop) static_assert(sizeof(icmp_echo_hdr) == 16, "icmp_echo_hdr size is not 16 bytes"); + /// The smallest valid ICMP echo request/reply message, i.e. the RFC 792 echo header. Smaller than + /// sizeof(icmp_echo_hdr), which also covers the first 8 payload bytes + static const size_t ICMP_ECHO_MIN_LEN = sizeof(icmphdr) + 2 * sizeof(uint16_t); + /// @struct icmp_echo_request /// ICMP echo (ping) request/reply message structure typedef struct icmp_echo_request @@ -274,12 +279,15 @@ namespace pcpp { /// a pointer to the header data on the packet icmp_router_advertisement_hdr* header; + /// the number of bytes available on the packet from icmp_router_advertisement#header onwards, used to bound + /// the wire-controlled advertisementCount + size_t dataLength; /// Extract router advertisement at a given index /// @param[in] index The index of the router advertisement /// @return A pointer to the router advertisement on the packet or null if index is out of range (less than zero /// or greater than the number of router advertisement records on this message, determined by advertisementCount - /// field) + /// field), or if the record is not fully present in the packet data icmp_router_address_structure* getRouterAddress(int index) const; }; @@ -328,6 +336,18 @@ namespace pcpp icmp_echo_request m_EchoData; mutable icmp_router_advertisement m_RouterAdvData; + /// Cast the layer data to a fixed-size ICMP message structure, or nullptr if the message is of a different + /// type or the layer is too short to hold the whole structure (a truncated packet) + template T* castMessageData(IcmpMessageType type) const + { + if (!isMessageOfType(type) || m_DataLen < sizeof(T)) + return nullptr; + + return reinterpret_cast(m_Data); + } + + icmp_echo_request* getEchoData(IcmpMessageType echoType); + bool cleanIcmpLayer(); bool setEchoData(IcmpMessageType echoType, uint16_t id, uint16_t sequence, uint64_t timestamp, @@ -623,10 +643,19 @@ namespace pcpp uint8_t type = data[0]; - // ICMP_ECHO_REQUEST, ICMP_ECHO_REPLY, ICMP_ROUTER_SOL, ICMP_INFO_REQUEST, ICMP_INFO_REPLY - if (type == 8 || type == 0 || type == 10 || type == 15 || type == 16) + // ICMP_ROUTER_SOL, which is just the base header + if (type == 10) return true; + // ICMP_ECHO_REQUEST, ICMP_ECHO_REPLY. An echo message carries a variable-length payload, so only the RFC 792 + // echo header can be required here + if (type == 8 || type == 0) + return dataLen >= ICMP_ECHO_MIN_LEN; + + // ICMP_INFO_REQUEST, ICMP_INFO_REPLY + if (type == 15 || type == 16) + return dataLen >= sizeof(icmp_info_request); + // ICMP_TIMESTAMP_REQUEST, ICMP_TIMESTAMP_REPLY if (type == 13 || type == 14) return dataLen >= sizeof(icmp_timestamp_request); diff --git a/Packet++/src/IcmpLayer.cpp b/Packet++/src/IcmpLayer.cpp index 85cab975c1..6b38886f27 100644 --- a/Packet++/src/IcmpLayer.cpp +++ b/Packet++/src/IcmpLayer.cpp @@ -16,9 +16,13 @@ namespace pcpp if (index < 0 || index >= header->advertisementCount) return nullptr; + // advertisementCount comes off the wire, so it can claim more records than the message carries + size_t offset = sizeof(icmp_router_advertisement_hdr) + index * sizeof(icmp_router_address_structure); + if (offset + sizeof(icmp_router_address_structure) > dataLength) + return nullptr; + uint8_t* headerAsByteArr = reinterpret_cast(header); - return reinterpret_cast( - headerAsByteArr + sizeof(icmp_router_advertisement_hdr) + index * sizeof(icmp_router_address_structure)); + return reinterpret_cast(headerAsByteArr + offset); } void icmp_router_address_structure::setRouterAddress(IPv4Address addr, uint32_t preference) @@ -119,18 +123,27 @@ namespace pcpp return true; } - icmp_echo_request* IcmpLayer::getEchoRequestData() + icmp_echo_request* IcmpLayer::getEchoData(IcmpMessageType echoType) { - if (!isMessageOfType(ICMP_ECHO_REQUEST)) + // an echo message is at least the RFC 792 echo header: the base header plus the identifier and sequence + // number. It can still be shorter than icmp_echo_hdr, which also folds in the first 8 payload bytes as a + // timestamp - the copy quoted inside an ICMP error message is exactly ICMP_ECHO_MIN_LEN bytes + if (!isMessageOfType(echoType) || m_DataLen < ICMP_ECHO_MIN_LEN) return nullptr; + bool hasEchoHdr = m_DataLen >= sizeof(icmp_echo_hdr); m_EchoData.header = reinterpret_cast(m_Data); - m_EchoData.data = reinterpret_cast(m_Data + sizeof(icmp_echo_hdr)); - m_EchoData.dataLength = m_DataLen - sizeof(icmp_echo_hdr); + m_EchoData.data = hasEchoHdr ? m_Data + sizeof(icmp_echo_hdr) : nullptr; + m_EchoData.dataLength = hasEchoHdr ? m_DataLen - sizeof(icmp_echo_hdr) : 0; return &m_EchoData; } + icmp_echo_request* IcmpLayer::getEchoRequestData() + { + return getEchoData(ICMP_ECHO_REQUEST); + } + icmp_echo_request* IcmpLayer::setEchoRequestData(uint16_t id, uint16_t sequence, uint64_t timestamp, const uint8_t* data, size_t dataLen) { @@ -142,14 +155,7 @@ namespace pcpp icmp_echo_reply* IcmpLayer::getEchoReplyData() { - if (!isMessageOfType(ICMP_ECHO_REPLY)) - return nullptr; - - m_EchoData.header = reinterpret_cast(m_Data); - m_EchoData.data = reinterpret_cast(m_Data + sizeof(icmp_echo_hdr)); - m_EchoData.dataLength = m_DataLen - sizeof(icmp_echo_hdr); - - return &m_EchoData; + return getEchoData(ICMP_ECHO_REPLY); } icmp_echo_reply* IcmpLayer::setEchoReplyData(uint16_t id, uint16_t sequence, uint64_t timestamp, @@ -163,10 +169,7 @@ namespace pcpp icmp_timestamp_request* IcmpLayer::getTimestampRequestData() { - if (!isMessageOfType(ICMP_TIMESTAMP_REQUEST)) - return nullptr; - - return reinterpret_cast(m_Data); + return castMessageData(ICMP_TIMESTAMP_REQUEST); } icmp_timestamp_request* IcmpLayer::setTimestampRequestData(uint16_t id, uint16_t sequence, @@ -193,10 +196,7 @@ namespace pcpp icmp_timestamp_reply* IcmpLayer::getTimestampReplyData() { - if (!isMessageOfType(ICMP_TIMESTAMP_REPLY)) - return nullptr; - - return reinterpret_cast(m_Data); + return castMessageData(ICMP_TIMESTAMP_REPLY); } icmp_timestamp_reply* IcmpLayer::setTimestampReplyData(uint16_t id, uint16_t sequence, timeval originateTimestamp, @@ -223,10 +223,7 @@ namespace pcpp icmp_destination_unreachable* IcmpLayer::getDestUnreachableData() { - if (!isMessageOfType(ICMP_DEST_UNREACHABLE)) - return nullptr; - - return reinterpret_cast(m_Data); + return castMessageData(ICMP_DEST_UNREACHABLE); } icmp_destination_unreachable* IcmpLayer::setDestUnreachableData(IcmpDestUnreachableCodes code, uint16_t nextHopMTU, @@ -253,10 +250,7 @@ namespace pcpp icmp_source_quench* IcmpLayer::getSourceQuenchdata() { - if (!isMessageOfType(ICMP_SOURCE_QUENCH)) - return nullptr; - - return reinterpret_cast(m_Data); + return castMessageData(ICMP_SOURCE_QUENCH); } icmp_source_quench* IcmpLayer::setSourceQuenchdata(IPv4Layer* ipHeader, Layer* l4Header) @@ -280,10 +274,7 @@ namespace pcpp icmp_redirect* IcmpLayer::getRedirectData() { - if (!isMessageOfType(ICMP_REDIRECT)) - return nullptr; - - return reinterpret_cast(m_Data); + return castMessageData(ICMP_REDIRECT); } icmp_redirect* IcmpLayer::setRedirectData(uint8_t code, IPv4Address gatewayAddress, IPv4Layer* ipHeader, @@ -315,10 +306,12 @@ namespace pcpp icmp_router_advertisement* IcmpLayer::getRouterAdvertisementData() const { - if (!isMessageOfType(ICMP_ROUTER_ADV)) + auto* header = castMessageData(ICMP_ROUTER_ADV); + if (header == nullptr) return nullptr; - m_RouterAdvData.header = reinterpret_cast(m_Data); + m_RouterAdvData.header = header; + m_RouterAdvData.dataLength = m_DataLen; return &m_RouterAdvData; } @@ -363,10 +356,7 @@ namespace pcpp icmp_router_solicitation* IcmpLayer::getRouterSolicitationData() { - if (!isMessageOfType(ICMP_ROUTER_SOL)) - return nullptr; - - return reinterpret_cast(m_Data); + return castMessageData(ICMP_ROUTER_SOL); } icmp_router_solicitation* IcmpLayer::setRouterSolicitationData() @@ -384,10 +374,7 @@ namespace pcpp icmp_time_exceeded* IcmpLayer::getTimeExceededData() { - if (!isMessageOfType(ICMP_TIME_EXCEEDED)) - return nullptr; - - return reinterpret_cast(m_Data); + return castMessageData(ICMP_TIME_EXCEEDED); } icmp_time_exceeded* IcmpLayer::setTimeExceededData(uint8_t code, IPv4Layer* ipHeader, Layer* l4Header) @@ -418,10 +405,7 @@ namespace pcpp icmp_param_problem* IcmpLayer::getParamProblemData() { - if (!isMessageOfType(ICMP_PARAM_PROBLEM)) - return nullptr; - - return reinterpret_cast(m_Data); + return castMessageData(ICMP_PARAM_PROBLEM); } icmp_param_problem* IcmpLayer::setParamProblemData(uint8_t code, uint8_t errorOctetPointer, IPv4Layer* ipHeader, @@ -455,10 +439,7 @@ namespace pcpp icmp_address_mask_request* IcmpLayer::getAddressMaskRequestData() { - if (!isMessageOfType(ICMP_ADDRESS_MASK_REQUEST)) - return nullptr; - - return reinterpret_cast(m_Data); + return castMessageData(ICMP_ADDRESS_MASK_REQUEST); } icmp_address_mask_request* IcmpLayer::setAddressMaskRequestData(uint16_t id, uint16_t sequence, IPv4Address mask) @@ -482,10 +463,7 @@ namespace pcpp icmp_address_mask_reply* IcmpLayer::getAddressMaskReplyData() { - if (!isMessageOfType(ICMP_ADDRESS_MASK_REPLY)) - return nullptr; - - return reinterpret_cast(m_Data); + return castMessageData(ICMP_ADDRESS_MASK_REPLY); } icmp_address_mask_reply* IcmpLayer::setAddressMaskReplyData(uint16_t id, uint16_t sequence, IPv4Address mask) @@ -509,10 +487,7 @@ namespace pcpp icmp_info_request* IcmpLayer::getInfoRequestData() { - if (!isMessageOfType(ICMP_INFO_REQUEST)) - return nullptr; - - return reinterpret_cast(m_Data); + return castMessageData(ICMP_INFO_REQUEST); } icmp_info_request* IcmpLayer::setInfoRequestData(uint16_t id, uint16_t sequence) @@ -535,10 +510,7 @@ namespace pcpp icmp_info_reply* IcmpLayer::getInfoReplyData() { - if (!isMessageOfType(ICMP_INFO_REPLY)) - return nullptr; - - return reinterpret_cast(m_Data); + return castMessageData(ICMP_INFO_REPLY); } icmp_info_reply* IcmpLayer::setInfoReplyData(uint16_t id, uint16_t sequence) @@ -589,7 +561,6 @@ namespace pcpp size_t IcmpLayer::getHeaderLen() const { IcmpMessageType type = getMessageType(); - size_t routerAdvSize = 0; switch (type) { case ICMP_ECHO_REQUEST: @@ -616,12 +587,19 @@ namespace pcpp case ICMP_PARAM_PROBLEM: return sizeof(icmp_param_problem); case ICMP_ROUTER_ADV: + { + // null if the layer is too short to hold icmp_router_advertisement_hdr + auto* routerAdvData = getRouterAdvertisementData(); + if (routerAdvData == nullptr) + return m_DataLen; + // clang-format off - routerAdvSize = sizeof(icmp_router_advertisement_hdr) + (getRouterAdvertisementData()->header->advertisementCount * sizeof(icmp_router_address_structure)); + size_t routerAdvSize = sizeof(icmp_router_advertisement_hdr) + (routerAdvData->header->advertisementCount * sizeof(icmp_router_address_structure)); // clang-format on if (routerAdvSize > m_DataLen) return m_DataLen; return routerAdvSize; + } default: return sizeof(icmphdr); } diff --git a/Tests/Fuzzers/ReadParsedPacket.h b/Tests/Fuzzers/ReadParsedPacket.h index ad17867c86..5169f33d39 100644 --- a/Tests/Fuzzers/ReadParsedPacket.h +++ b/Tests/Fuzzers/ReadParsedPacket.h @@ -349,17 +349,26 @@ static void readParsedPacket(pcpp::Packet parsedPacket, pcpp::Layer* layer) else if (icmpLayer->isMessageOfType(pcpp::ICMP_INFO_REPLY)) { auto layerData = icmpLayer->getInfoReplyData(); - icmpLayer2.setInfoReplyData(layerData->id, layerData->sequence); + if (layerData != nullptr) + { + icmpLayer2.setInfoReplyData(layerData->id, layerData->sequence); + } } else if (icmpLayer->isMessageOfType(pcpp::ICMP_INFO_REQUEST)) { auto layerData = icmpLayer->getInfoRequestData(); - icmpLayer2.setInfoRequestData(layerData->id, layerData->sequence); + if (layerData != nullptr) + { + icmpLayer2.setInfoRequestData(layerData->id, layerData->sequence); + } } else if (icmpLayer->isMessageOfType(pcpp::ICMP_PARAM_PROBLEM)) { auto layerData = icmpLayer->getParamProblemData(); - icmpLayer2.setParamProblemData(layerData->code, layerData->pointer, nullptr, nullptr); + if (layerData != nullptr) + { + icmpLayer2.setParamProblemData(layerData->code, layerData->pointer, nullptr, nullptr); + } } else if (icmpLayer->isMessageOfType(pcpp::ICMP_TIME_EXCEEDED)) { diff --git a/Tests/Fuzzers/RegressionTests/regression_samples/crash-icmp-info-request-truncated b/Tests/Fuzzers/RegressionTests/regression_samples/crash-icmp-info-request-truncated new file mode 100644 index 0000000000..c75b46881c Binary files /dev/null and b/Tests/Fuzzers/RegressionTests/regression_samples/crash-icmp-info-request-truncated differ diff --git a/Tests/Packet++Test/PacketExamples/IcmpInfoRequestTruncated.dat b/Tests/Packet++Test/PacketExamples/IcmpInfoRequestTruncated.dat new file mode 100644 index 0000000000..6b6f2aef3c --- /dev/null +++ b/Tests/Packet++Test/PacketExamples/IcmpInfoRequestTruncated.dat @@ -0,0 +1 @@ +86bc7fd77f154090c5725840080045000027b8b10000400100000a000036c00f0f0f0f0f0f0f0f0f0f \ No newline at end of file diff --git a/Tests/Packet++Test/TestDefinition.h b/Tests/Packet++Test/TestDefinition.h index 71eccbeac6..8a3171176d 100644 --- a/Tests/Packet++Test/TestDefinition.h +++ b/Tests/Packet++Test/TestDefinition.h @@ -132,6 +132,7 @@ PTF_TEST_CASE(DoIpInvalidPackets); // Implemented in IcmpTests.cpp PTF_TEST_CASE(IcmpParsingTest); +PTF_TEST_CASE(IcmpTruncatedPacketTest); PTF_TEST_CASE(IcmpCreationTest); PTF_TEST_CASE(IcmpEditTest); diff --git a/Tests/Packet++Test/Tests/IcmpTests.cpp b/Tests/Packet++Test/Tests/IcmpTests.cpp index 6cc1517bd6..c73bcec3f6 100644 --- a/Tests/Packet++Test/Tests/IcmpTests.cpp +++ b/Tests/Packet++Test/Tests/IcmpTests.cpp @@ -6,8 +6,10 @@ #include "EthLayer.h" #include "IcmpLayer.h" #include "IPv4Layer.h" +#include "PayloadLayer.h" #include "UdpLayer.h" #include "SystemUtils.h" +#include using pcpp_tests::utils::createPacketFromHexResource; @@ -255,6 +257,79 @@ PTF_TEST_CASE(IcmpParsingTest) PTF_ASSERT_EQUAL(routerAddr->preferenceLevel, 0); } // IcmpParsingTest +PTF_TEST_CASE(IcmpTruncatedPacketTest) +{ + // The get*Data() methods only checked the ICMP message type before casting the layer to the fixed-size structure + // that type implies, so a truncated message let callers read fields past the end of the layer + + // an information request/reply is 8 bytes, but isDataValid() accepted one carrying only the 4-byte base header + const uint8_t infoRequest[] = { 0x0f, 0x00, 0x00, 0x00, 0x12, 0x34, 0x56, 0x78 }; + PTF_ASSERT_FALSE(pcpp::IcmpLayer::isDataValid(infoRequest, 7)); + PTF_ASSERT_TRUE(pcpp::IcmpLayer::isDataValid(infoRequest, 8)); + + // an echo message folds the first 8 payload bytes into icmp_echo_hdr as a timestamp, so it can be valid and still + // be shorter than that structure - the 8-byte copy quoted inside an ICMP error message is the common case + const uint8_t echoRequest[] = { 0x08, 0x00, 0x00, 0x00, 0x12, 0x34, 0x56, 0x78 }; + PTF_ASSERT_FALSE(pcpp::IcmpLayer::isDataValid(echoRequest, 7)); + PTF_ASSERT_TRUE(pcpp::IcmpLayer::isDataValid(echoRequest, 8)); + + // the getters bounds-check the layer themselves too, since an IcmpLayer can be built over any buffer + auto icmpLayer = [](uint8_t type, size_t dataLen) { + auto* data = new uint8_t[dataLen](); // the layer takes ownership of the buffer and frees it + data[0] = type; + return std::unique_ptr(new pcpp::IcmpLayer(data, dataLen, nullptr, nullptr)); + }; + + // one byte short of icmp_echo_hdr's 8-byte minimum, of the 20-byte timestamp and 12-byte address mask + // structures, and of the 8-byte structures behind the remaining types + PTF_ASSERT_NULL(icmpLayer(8, 7)->getEchoRequestData()); + PTF_ASSERT_NULL(icmpLayer(0, 7)->getEchoReplyData()); + PTF_ASSERT_NULL(icmpLayer(13, 19)->getTimestampRequestData()); + PTF_ASSERT_NULL(icmpLayer(14, 19)->getTimestampReplyData()); + PTF_ASSERT_NULL(icmpLayer(17, 11)->getAddressMaskRequestData()); + PTF_ASSERT_NULL(icmpLayer(18, 11)->getAddressMaskReplyData()); + PTF_ASSERT_NULL(icmpLayer(3, 7)->getDestUnreachableData()); + PTF_ASSERT_NULL(icmpLayer(4, 7)->getSourceQuenchdata()); + PTF_ASSERT_NULL(icmpLayer(5, 7)->getRedirectData()); + PTF_ASSERT_NULL(icmpLayer(9, 7)->getRouterAdvertisementData()); + PTF_ASSERT_NULL(icmpLayer(11, 7)->getTimeExceededData()); + PTF_ASSERT_NULL(icmpLayer(12, 7)->getParamProblemData()); + PTF_ASSERT_NULL(icmpLayer(15, 7)->getInfoRequestData()); + PTF_ASSERT_NULL(icmpLayer(16, 7)->getInfoReplyData()); + + // a router solicitation is just the base header, so it is complete at the minimum layer size + PTF_ASSERT_NOT_NULL(icmpLayer(10, 4)->getRouterSolicitationData()); + + // getHeaderLen() used to dereference getRouterAdvertisementData() without a null check + PTF_ASSERT_EQUAL(icmpLayer(9, 7)->getHeaderLen(), 7); + + // an echo message between the RFC 792 header and icmp_echo_hdr has no payload, and the length of that payload + // used to underflow into a huge value + auto shortEchoLayer = icmpLayer(8, 15); + auto* echoData = shortEchoLayer->getEchoRequestData(); + PTF_ASSERT_NOT_NULL(echoData); + PTF_ASSERT_NULL(echoData->data); + PTF_ASSERT_EQUAL(echoData->dataLength, 0); + + // advertisementCount can claim more router records than the message carries + auto routerAdvLayer = icmpLayer(9, 16); + auto* routerAdvData = routerAdvLayer->getRouterAdvertisementData(); + PTF_ASSERT_NOT_NULL(routerAdvData); + routerAdvData->header->advertisementCount = 5; // only one 8-byte record follows the 8-byte header + PTF_ASSERT_NOT_NULL(routerAdvData->getRouterAddress(0)); + PTF_ASSERT_NULL(routerAdvData->getRouterAddress(1)); + + // end to end: an IPv4 packet whose ICMP information request (type 15) carries 7 bytes instead of 8 falls back + // to a payload layer + auto truncatedRawPacket = createPacketFromHexResource("PacketExamples/IcmpInfoRequestTruncated.dat"); + pcpp::Packet truncatedPacket(truncatedRawPacket.get()); + + PTF_ASSERT_FALSE(truncatedPacket.isPacketOfType(pcpp::ICMP)); + auto* payloadLayer = truncatedPacket.getLayerOfType(); + PTF_ASSERT_NOT_NULL(payloadLayer); + PTF_ASSERT_EQUAL(payloadLayer->getPayloadLen(), 7); +} // IcmpTruncatedPacketTest + PTF_TEST_CASE(IcmpCreationTest) { auto resource1 = pcpp_tests::loadHexResourceToVector("PacketExamples/IcmpEchoRequest.dat"); diff --git a/Tests/Packet++Test/main.cpp b/Tests/Packet++Test/main.cpp index c35c696fde..725e1cd1e8 100644 --- a/Tests/Packet++Test/main.cpp +++ b/Tests/Packet++Test/main.cpp @@ -236,6 +236,7 @@ int main(int argc, char* argv[]) PTF_RUN_TEST(DoIpInvalidPackets, "doip"); PTF_RUN_TEST(IcmpParsingTest, "icmp"); + PTF_RUN_TEST(IcmpTruncatedPacketTest, "icmp"); PTF_RUN_TEST(IcmpCreationTest, "icmp"); PTF_RUN_TEST(IcmpEditTest, "icmp");