Skip to content

Fix out-of-bounds reads in IcmpLayer getters on truncated packets #2240

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Mrmaxmeier wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Fix out-of-bounds reads in IcmpLayer getters on truncated packets #2240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 33 additions & 4 deletions Packet++/header/IcmpLayer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

@seladb seladb Sep 12, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I didn't know that timestamp isn't part of the RFC. In that case we might need a breaking change here:

  • Remove timestamp from the struct (that way we no longer need ICMP_ECHO_MIN_LEN)
  • Add 2 methods to icmp_echo_request that returns the timestamp if exists:
    • bool hasTimestamp()
    • uint64_t getTimetamp()

#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
Expand Down Expand Up @@ -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;
};

Expand Down Expand Up @@ -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 <typename T> T* castMessageData(IcmpMessageType type) const
{
if (!isMessageOfType(type) || m_DataLen < sizeof(T))
return nullptr;

return reinterpret_cast<T*>(m_Data);
}

icmp_echo_request* getEchoData(IcmpMessageType echoType);

bool cleanIcmpLayer();

bool setEchoData(IcmpMessageType echoType, uint16_t id, uint16_t sequence, uint64_t timestamp,
Expand Down Expand Up @@ -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);

@seladb seladb Sep 12, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

If I understand correctly, this is the only change that is related to the PR description. The other changes are separate. Maybe we can break this PR to multiple PRs?

  • The first contains only this change (and tests)
  • Another PR for making timestamp optional in echo request (and tests)
  • A last PR for fixing getRouterAddress() (if you prefer to open less PRs, you can include it in the second PR because it's pretty minor change

// ICMP_TIMESTAMP_REQUEST, ICMP_TIMESTAMP_REPLY
if (type == 13 || type == 14)
return dataLen >= sizeof(icmp_timestamp_request);
Expand Down
110 changes: 44 additions & 66 deletions Packet++/src/IcmpLayer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<uint8_t*>(header);
return reinterpret_cast<icmp_router_address_structure*>(
headerAsByteArr + sizeof(icmp_router_advertisement_hdr) + index * sizeof(icmp_router_address_structure));
return reinterpret_cast<icmp_router_address_structure*>(headerAsByteArr + offset);
}

void icmp_router_address_structure::setRouterAddress(IPv4Address addr, uint32_t preference)
Expand Down Expand Up @@ -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<icmp_echo_hdr*>(m_Data);
m_EchoData.data = reinterpret_cast<uint8_t*>(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)
{
Expand All @@ -142,14 +155,7 @@ namespace pcpp

icmp_echo_reply* IcmpLayer::getEchoReplyData()
{
if (!isMessageOfType(ICMP_ECHO_REPLY))
return nullptr;

m_EchoData.header = reinterpret_cast<icmp_echo_hdr*>(m_Data);
m_EchoData.data = reinterpret_cast<uint8_t*>(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,
Expand All @@ -163,10 +169,7 @@ namespace pcpp

icmp_timestamp_request* IcmpLayer::getTimestampRequestData()
{
if (!isMessageOfType(ICMP_TIMESTAMP_REQUEST))
return nullptr;

return reinterpret_cast<icmp_timestamp_request*>(m_Data);
return castMessageData<icmp_timestamp_request>(ICMP_TIMESTAMP_REQUEST);
}

icmp_timestamp_request* IcmpLayer::setTimestampRequestData(uint16_t id, uint16_t sequence,
Expand All @@ -193,10 +196,7 @@ namespace pcpp

icmp_timestamp_reply* IcmpLayer::getTimestampReplyData()
{
if (!isMessageOfType(ICMP_TIMESTAMP_REPLY))
return nullptr;

return reinterpret_cast<icmp_timestamp_reply*>(m_Data);
return castMessageData<icmp_timestamp_reply>(ICMP_TIMESTAMP_REPLY);
}

icmp_timestamp_reply* IcmpLayer::setTimestampReplyData(uint16_t id, uint16_t sequence, timeval originateTimestamp,
Expand All @@ -223,10 +223,7 @@ namespace pcpp

icmp_destination_unreachable* IcmpLayer::getDestUnreachableData()
{
if (!isMessageOfType(ICMP_DEST_UNREACHABLE))
return nullptr;

return reinterpret_cast<icmp_destination_unreachable*>(m_Data);
return castMessageData<icmp_destination_unreachable>(ICMP_DEST_UNREACHABLE);
}

icmp_destination_unreachable* IcmpLayer::setDestUnreachableData(IcmpDestUnreachableCodes code, uint16_t nextHopMTU,
Expand All @@ -253,10 +250,7 @@ namespace pcpp

icmp_source_quench* IcmpLayer::getSourceQuenchdata()
{
if (!isMessageOfType(ICMP_SOURCE_QUENCH))
return nullptr;

return reinterpret_cast<icmp_source_quench*>(m_Data);
return castMessageData<icmp_source_quench>(ICMP_SOURCE_QUENCH);
}

icmp_source_quench* IcmpLayer::setSourceQuenchdata(IPv4Layer* ipHeader, Layer* l4Header)
Expand All @@ -280,10 +274,7 @@ namespace pcpp

icmp_redirect* IcmpLayer::getRedirectData()
{
if (!isMessageOfType(ICMP_REDIRECT))
return nullptr;

return reinterpret_cast<icmp_redirect*>(m_Data);
return castMessageData<icmp_redirect>(ICMP_REDIRECT);
}

icmp_redirect* IcmpLayer::setRedirectData(uint8_t code, IPv4Address gatewayAddress, IPv4Layer* ipHeader,
Expand Down Expand Up @@ -315,10 +306,12 @@ namespace pcpp

icmp_router_advertisement* IcmpLayer::getRouterAdvertisementData() const
{
if (!isMessageOfType(ICMP_ROUTER_ADV))
auto* header = castMessageData<icmp_router_advertisement_hdr>(ICMP_ROUTER_ADV);
if (header == nullptr)
return nullptr;

m_RouterAdvData.header = reinterpret_cast<icmp_router_advertisement_hdr*>(m_Data);
m_RouterAdvData.header = header;
m_RouterAdvData.dataLength = m_DataLen;

return &m_RouterAdvData;
}
Expand Down Expand Up @@ -363,10 +356,7 @@ namespace pcpp

icmp_router_solicitation* IcmpLayer::getRouterSolicitationData()
{
if (!isMessageOfType(ICMP_ROUTER_SOL))
return nullptr;

return reinterpret_cast<icmp_router_solicitation*>(m_Data);
return castMessageData<icmp_router_solicitation>(ICMP_ROUTER_SOL);
}

icmp_router_solicitation* IcmpLayer::setRouterSolicitationData()
Expand All @@ -384,10 +374,7 @@ namespace pcpp

icmp_time_exceeded* IcmpLayer::getTimeExceededData()
{
if (!isMessageOfType(ICMP_TIME_EXCEEDED))
return nullptr;

return reinterpret_cast<icmp_time_exceeded*>(m_Data);
return castMessageData<icmp_time_exceeded>(ICMP_TIME_EXCEEDED);
}

icmp_time_exceeded* IcmpLayer::setTimeExceededData(uint8_t code, IPv4Layer* ipHeader, Layer* l4Header)
Expand Down Expand Up @@ -418,10 +405,7 @@ namespace pcpp

icmp_param_problem* IcmpLayer::getParamProblemData()
{
if (!isMessageOfType(ICMP_PARAM_PROBLEM))
return nullptr;

return reinterpret_cast<icmp_param_problem*>(m_Data);
return castMessageData<icmp_param_problem>(ICMP_PARAM_PROBLEM);
}

icmp_param_problem* IcmpLayer::setParamProblemData(uint8_t code, uint8_t errorOctetPointer, IPv4Layer* ipHeader,
Expand Down Expand Up @@ -455,10 +439,7 @@ namespace pcpp

icmp_address_mask_request* IcmpLayer::getAddressMaskRequestData()
{
if (!isMessageOfType(ICMP_ADDRESS_MASK_REQUEST))
return nullptr;

return reinterpret_cast<icmp_address_mask_request*>(m_Data);
return castMessageData<icmp_address_mask_request>(ICMP_ADDRESS_MASK_REQUEST);
}

icmp_address_mask_request* IcmpLayer::setAddressMaskRequestData(uint16_t id, uint16_t sequence, IPv4Address mask)
Expand All @@ -482,10 +463,7 @@ namespace pcpp

icmp_address_mask_reply* IcmpLayer::getAddressMaskReplyData()
{
if (!isMessageOfType(ICMP_ADDRESS_MASK_REPLY))
return nullptr;

return reinterpret_cast<icmp_address_mask_reply*>(m_Data);
return castMessageData<icmp_address_mask_reply>(ICMP_ADDRESS_MASK_REPLY);
}

icmp_address_mask_reply* IcmpLayer::setAddressMaskReplyData(uint16_t id, uint16_t sequence, IPv4Address mask)
Expand All @@ -509,10 +487,7 @@ namespace pcpp

icmp_info_request* IcmpLayer::getInfoRequestData()
{
if (!isMessageOfType(ICMP_INFO_REQUEST))
return nullptr;

return reinterpret_cast<icmp_info_request*>(m_Data);
return castMessageData<icmp_info_request>(ICMP_INFO_REQUEST);
}

icmp_info_request* IcmpLayer::setInfoRequestData(uint16_t id, uint16_t sequence)
Expand All @@ -535,10 +510,7 @@ namespace pcpp

icmp_info_reply* IcmpLayer::getInfoReplyData()
{
if (!isMessageOfType(ICMP_INFO_REPLY))
return nullptr;

return reinterpret_cast<icmp_info_reply*>(m_Data);
return castMessageData<icmp_info_reply>(ICMP_INFO_REPLY);
}

icmp_info_reply* IcmpLayer::setInfoReplyData(uint16_t id, uint16_t sequence)
Expand Down Expand Up @@ -589,7 +561,6 @@ namespace pcpp
size_t IcmpLayer::getHeaderLen() const
{
IcmpMessageType type = getMessageType();
size_t routerAdvSize = 0;
switch (type)
{
case ICMP_ECHO_REQUEST:
Expand All @@ -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);
}
Expand Down
15 changes: 12 additions & 3 deletions Tests/Fuzzers/ReadParsedPacket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand Down
Binary file added Tests/Fuzzers/RegressionTests/regression_samples/crash-icmp-info-request-truncated
View file
Open in desktop
Binary file not shown.
1 change: 1 addition & 0 deletions Tests/Packet++Test/PacketExamples/IcmpInfoRequestTruncated.dat
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
86bc7fd77f154090c5725840080045000027b8b10000400100000a000036c00f0f0f0f0f0f0f0f0f0f

@seladb seladb Sep 12, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can you include the pcap file for this packet? That way it'd be easier to view it in Wireshark

1 change: 1 addition & 0 deletions Tests/Packet++Test/TestDefinition.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Loading
Loading