Allow gratuitous ARP requests. #1684

Dimi1010 · 2025-01-07T22:11:59Z

This PR is a potential implementation of #1682.

Specific header constructors

ArpLayer has been expanded with constructors accepting the following structures that build a respective ARP header inside the layer.

ArpRequest
ArpReply
GratuitousArpRequest
GratuitousArpReply

… / reply. - NB: Removed zeroing of MAC address if the ArpLayer contains a opcode ARP_REQUEST. - Added new structs with only required data to construct a certain packet.

Packet++/header/ArpLayer.h

- Added a new private constructor to handle populating the header. The old constructor is forwarded to this one with the caveat that the target MAC address is ignored and zeroed if a request is to be made.

codecov · 2025-01-09T23:50:54Z

Codecov Report

Attention: Patch coverage is 95.94595% with 6 lines in your changes missing coverage. Please review.

Project coverage is 83.15%. Comparing base (d60c699) to head (6cffe96).

Files with missing lines	Patch %	Lines
Examples/ArpSpoofing/main.cpp	0.00%	3 Missing ⚠️
Packet++/src/ArpLayer.cpp	93.61%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1684      +/-   ##
==========================================
+ Coverage   83.11%   83.15%   +0.03%     
==========================================
  Files         277      277              
  Lines       48207    48315     +108     
  Branches    10192     9959     -233     
==========================================
+ Hits        40069    40174     +105     
- Misses       7243     7258      +15     
+ Partials      895      883      -12

Flag	Coverage Δ
alpine320	`75.14% <90.12%> (+0.03%)`	⬆️
fedora40	`75.15% <90.12%> (-0.01%)`	⬇️
macos-13	`80.66% <96.50%> (+0.04%)`	⬆️
macos-14	`80.66% <96.50%> (+0.04%)`	⬆️
macos-15	`80.64% <96.50%> (+0.04%)`	⬆️
mingw32	`70.81% <78.26%> (-0.05%)`	⬇️
mingw64	`70.78% <78.26%> (-0.04%)`	⬇️
npcap	`85.25% <95.23%> (+<0.01%)`	⬆️
rhel94	`75.03% <88.88%> (+0.05%)`	⬆️
ubuntu2004	`58.53% <54.19%> (-0.06%)`	⬇️
ubuntu2004-zstd	`58.65% <54.19%> (-0.04%)`	⬇️
ubuntu2204	`74.92% <88.88%> (+0.02%)`	⬆️
ubuntu2204-icpx	`61.55% <81.48%> (+0.10%)`	⬆️
ubuntu2404	`75.18% <90.12%> (+0.02%)`	⬆️
unittest	`83.15% <95.94%> (+0.03%)`	⬆️
windows-2019	`85.29% <95.23%> (+<0.01%)`	⬆️
windows-2022	`85.31% <95.23%> (+<0.01%)`	⬆️
winpcap	`85.28% <95.23%> (+0.01%)`	⬆️
xdp	`50.60% <85.18%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

seladb · 2025-01-10T08:19:54Z

@Dimi1010 the main issue I see with this approach is that it's quite strict and limits users to specific options to create ARP messages. Creating a non-standard ARP message is not possible unless they use the deprecated c'tor.

My suggestion here keeps the existing c'tor which gives the most freedom while creating another c'tor for gratuitous ARP requests which AFAIK are quite rarely used (no one requested it until now, even though ARP exists for many years in PcapPlusPlus)

Dimi1010 · 2025-01-10T08:24:04Z

@Dimi1010 the main issue I see with this approach is that it's quite strict and limits users to specific options to create ARP messages. Creating a non-standard ARP message is not possible unless they use the deprecated c'tor.

My suggestion here keeps the existing c'tor which gives the most freedom while creating another c'tor for gratuitous ARP requests which AFAIK are quite rarely used (no one requested it until now, even though ARP exists for many years in PcapPlusPlus)

Deprecating the current ctor is not really a requirement, I deprecated it because people might have been relying on the zeroing the target MAC functionality.

The new private ctor can also be exposed to allow ppl to construct whatever packets they want, which would probably work better, as the MAC addr wont be zeroed out under their feet if they dont want it.

seladb · 2025-01-10T08:50:56Z

ing the current ctor is not really a requirement, I deprecated it because people might have been relying on the zeroing the target MAC functionality.

The new private ctor can also be exposed to

When I think about it, maybe we can just remove zeroing out targetMacAddr in computeCalculateFields() because we don't do it anywhere else 🤔

We probably need to update some tests and examples that use ARP (like Arping) which assume an ARP request always has a targetMacAddr of zero, but maybe that's the most flexible solution that provides a viable fix. The downside is that it's a breaking change for everyone who assumes it's zerod...

Dimi1010 · 2025-01-10T09:13:37Z

When I think about it, maybe we can just remove zeroing out targetMacAddr in computeCalculateFields() because we don't do it anywhere else 🤔

We probably need to update some tests and examples that use ARP (like Arping) which assume an ARP request always has a targetMacAddr of zero, but maybe that's the most flexible solution that provides a viable fix. The downside is that it's a breaking change for everyone who assumes it's zerod...

I can make the ArpLayer(ArpOpcode opCode, const MacAddress& senderMacAddr, const IPv4Address& senderIpAddr, const MacAddress& targetMacAddr, const IPv4Address& targetIpAddr); constructor public. It would allow a gentler transition for those that use the zeroing, as the old one will be flagged as deprecated to allow them time to transition.

Also a standard ARP request should have the target mac zeroed. It is in the spec.

…bytes.

… and target.

seladb · 2025-01-11T08:54:16Z

When I think about it, maybe we can just remove zeroing out targetMacAddr in computeCalculateFields() because we don't do it anywhere else 🤔
We probably need to update some tests and examples that use ARP (like Arping) which assume an ARP request always has a targetMacAddr of zero, but maybe that's the most flexible solution that provides a viable fix. The downside is that it's a breaking change for everyone who assumes it's zerod...

I can make the ArpLayer(ArpOpcode opCode, const MacAddress& senderMacAddr, const IPv4Address& senderIpAddr, const MacAddress& targetMacAddr, const IPv4Address& targetIpAddr); constructor public. It would allow a gentler transition for those that use the zeroing, as the old one will be flagged as deprecated to allow them time to transition.

Also a standard ARP request should have the target mac zeroed. It is in the spec.

You convinced me 🙂
If we keep the existing c'tor (not deprecated) then I guess we can add the additional c'tors with specific messages, and remove zeroing targetMacAddr

…e type of ARP message.

…arp-request

seladb

There are also ARP layers generated in 3 places in ArpSpoofing that we can modify: https://github.com/seladb/PcapPlusPlus/blob/feba0b7124a4536ec2a0a947bc96c146ada099d7/Examples/ArpSpoofing/main.cpp

Packet++/header/ArpLayer.h

seladb · 2025-01-12T08:26:30Z

Tests/Packet++Test/Tests/EthAndArpTests.cpp

+	}
+
+	{
+		// TODO: Add an actual packet to test against.


You can use ArpResponsePacket.dat

Sure, but why is the packet size 60 in EthAndArpPacketParsing? 14 (EthLayer) +28 (ArpLayer) makes 42, no?

Good question, the extra is Ethernet trailer, as you can see in ArpResponsePacket.pcap. But it is still a valid ARP response packet

ARP response can have random length padding at the end, which also confused me before.

Packet++/header/ArpLayer.h

…ssage structure based constructors.

…arp-request

Dimi1010 added 4 commits January 7, 2025 23:58

Added additional APR layer constructors that build a specific request…

767d70c

… / reply. - NB: Removed zeroing of MAC address if the ArpLayer contains a opcode ARP_REQUEST. - Added new structs with only required data to construct a certain packet.

Added explicit to ctors.

1e9f389

Added docs

a7b1275

Lint

03525c8

Dimi1010 requested a review from seladb as a code owner January 7, 2025 22:11

Dimi1010 requested a review from tigercosmos January 7, 2025 22:12

Dimi1010 linked an issue Jan 7, 2025 that may be closed by this pull request

Can not build gratuitous ARP request properly #1682

Open

Dimi1010 mentioned this pull request Jan 7, 2025

Can not build gratuitous ARP request properly #1682

Open

Dimi1010 marked this pull request as draft January 7, 2025 22:35

Changed const to constexpr.

d0a507a

Dimi1010 force-pushed the refactor/gratuitous-arp-request branch from 679bdbd to d0a507a Compare January 8, 2025 07:46

tigercosmos reviewed Jan 8, 2025

View reviewed changes

Packet++/header/ArpLayer.h Show resolved Hide resolved

tigercosmos reviewed Jan 8, 2025

View reviewed changes

Packet++/header/ArpLayer.h Show resolved Hide resolved

Dimi1010 added 2 commits January 10, 2025 01:31

Deprecated the old ARPLayer constructor.

c311285

- Added a new private constructor to handle populating the header. The old constructor is forwarded to this one with the caveat that the target MAC address is ignored and zeroed if a request is to be made.

Added documentation to the new structured.

d5c3fad

Dimi1010 added 2 commits January 10, 2025 11:33

Changed visibility of new direct ctor to public.

948e4aa

Merge branch 'dev' into refactor/gratuitous-arp-request

0dc7f05

Dimi1010 marked this pull request as ready for review January 10, 2025 10:13

Dimi1010 requested a review from tigercosmos January 10, 2025 10:13

Dimi1010 added 3 commits January 10, 2025 13:23

Added static assert to ensure the arphdr structure is precisely 28 …

08b31a0

…bytes.

Swapped constructor arguments to pair Mac and IP addresses for source…

da3f639

… and target.

Added unit tests for new constructors.

85b6c74

Dimi1010 added 2 commits January 11, 2025 13:04

Added a mechanism to probe for known header signatures to discover th…

bbe9904

…e type of ARP message.

Merge remote-tracking branch 'upstream/dev' into refactor/gratuitous-…

af37892

…arp-request

Dimi1010 added 2 commits January 11, 2025 13:07

Lint

d4cc2e5

Merge branch 'dev' into refactor/gratuitous-arp-request

0e0a69f

seladb reviewed Jan 12, 2025

View reviewed changes

tigercosmos reviewed Jan 15, 2025

View reviewed changes

Packet++/header/ArpLayer.h Show resolved Hide resolved

tigercosmos approved these changes Jan 16, 2025

View reviewed changes

Dimi1010 added 2 commits January 18, 2025 12:25

Changed ArpLayer constructors in examples and tests to use the new me…

014c063

…ssage structure based constructors.

Merge remote-tracking branch 'upstream/dev' into refactor/gratuitous-…

6cffe96

…arp-request

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow gratuitous ARP requests. #1684

Allow gratuitous ARP requests. #1684

Dimi1010 commented Jan 7, 2025 •

edited

Loading

codecov bot commented Jan 9, 2025 •

edited

Loading

seladb commented Jan 10, 2025

Dimi1010 commented Jan 10, 2025 •

edited

Loading

seladb commented Jan 10, 2025

Dimi1010 commented Jan 10, 2025 •

edited

Loading

seladb commented Jan 11, 2025

seladb left a comment

seladb Jan 12, 2025

Dimi1010 Jan 18, 2025

seladb Jan 19, 2025

tigercosmos Jan 20, 2025

Allow gratuitous ARP requests. #1684

Are you sure you want to change the base?

Allow gratuitous ARP requests. #1684

Conversation

Dimi1010 commented Jan 7, 2025 • edited Loading

Specific header constructors

codecov bot commented Jan 9, 2025 • edited Loading

Codecov Report

seladb commented Jan 10, 2025

Dimi1010 commented Jan 10, 2025 • edited Loading

seladb commented Jan 10, 2025

Dimi1010 commented Jan 10, 2025 • edited Loading

seladb commented Jan 11, 2025

seladb left a comment

Choose a reason for hiding this comment

seladb Jan 12, 2025

Choose a reason for hiding this comment

Dimi1010 Jan 18, 2025

Choose a reason for hiding this comment

seladb Jan 19, 2025

Choose a reason for hiding this comment

tigercosmos Jan 20, 2025

Choose a reason for hiding this comment

Dimi1010 commented Jan 7, 2025 •

edited

Loading

codecov bot commented Jan 9, 2025 •

edited

Loading

Dimi1010 commented Jan 10, 2025 •

edited

Loading

Dimi1010 commented Jan 10, 2025 •

edited

Loading