Skip to content
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

Jump to bottom

Can not build gratuitous ARP request properly #1682

Open
wangchong2023 opened this issue Jan 5, 2025 · 6 comments · May be fixed by #1684
Open

Can not build gratuitous ARP request properly #1682

wangchong2023 opened this issue Jan 5, 2025 · 6 comments · May be fixed by #1684
Labels
bug

Comments

@wangchong2023
Copy link
Contributor

wangchong2023 commented Jan 5, 2025 •
edited
Loading

Bug description

Describe the bug
RFC 5227 stipulates that gratuitous ARP can be a request, which is also described in the wireshark wiki.
The characteristics of gratuitous ARP are that the sending and source IP addresses are both the IP address of the prober, and the target MAC and source MAC addresses are both the MAC address of the prober.
Currently, the function 'computeCalculateFields' in Arplayer implemented by PcapPlusPlusPlus will set the targetMacAddr of the ARP request message to all zeros, which will cause the scenario of constructing a gratuitous ARP message to not work properly.

Code example to reproduce

int NeighAdvSendArpPacket(PcapLiveDevice* dev,
								const MacAddress& sourceMacAddr, const MacAddress& dstMacAddr, const MacAddress& targetMacAddr,
								const IPv4Address& senderIpAddr, const IPv4Address& targetIP, ArpOpcode arpOperCode)
	{
		Packet advPacket(100);

		// Ethernet layer
		EthLayer ethLayer(sourceMacAddr, dstMacAddr);
		if (!advPacket.addLayer(&ethLayer))
		{
			PCPP_LOG_ERROR("Couldn't build Eth layer for ARP packet");
			return 1;
		}

		//ARP layer
		ArpLayer arpLayer(arpOperCode, sourceMacAddr, targetMacAddr, senderIpAddr, targetIP);
		if (!advPacket.addLayer(&arpLayer))
		{
			PCPP_LOG_ERROR("Couldn't build ARP layer for ARP packet");
			return 1;
		}

		advPacket.computeCalculateFields();

		dev->sendPacket(&advPacket);

		return 0;
	}

Expected behavior
It is recommended to remove this default behavior (the targetMacAddr field of the ARP request type is set to all zeros by default) or to control whether to clear it through an additional field.

PcapPlusPlus versions tested on

PcapPlusPlus master branch

Other PcapPlusPlus version (if applicable)

No response

Operating systems tested on

Windows

Other operation systems (if applicable)

No response

Compiler version

GCC 10.2.0

Packet capture backend (if applicable)

WinPcap
@wangchong2023 wangchong2023 changed the title Gratuitous ARP Can not build gratuitous ARP request properly Jan 5, 2025
@tigercosmos
Copy link
Collaborator

I think we have two options: (1) adding a "gratuitous" flag in the current ARPLayer and (2) adding a new GratuitousARPLayer. Both sound reasonable to me. cc @seladb @Dimi1010

@Dimi1010
Copy link
Collaborator

Dimi1010 commented Jan 7, 2025 •
edited
Loading

I think we have two options: (1) adding a "gratuitous" flag in the current ARPLayer and (2) adding a new GratuitousARPLayer. Both sound reasonable to me. cc @seladb @Dimi1010

I think it might be better to move the conditional zeroing to the non-raw data constructor. Afterwards we can have a bool flag to skip the zeroing in the constructor, or add a new constructor that takes a special tag struct (e.g. GratuitusRequestTag) as first parameter, that mirrors the setup but does not zero the mac address.

Also I think it might be beneficial to add factory methods or constructors that take precisely the parameters needed to form the request, instead of relying the user to input all the data correctly. For example, from what I understand, a normal ARP request should zero the target mac address. So we can have a factory method that only takes the minimum required parameters to form the request (senderMacAddr, senderIPAddr, targetIPAddr). Notice how opCode and targetIpAddr are missing for the factory method, as they are either not needed or can be inferred by the method's use case.

@Dimi1010 Dimi1010 linked a pull request Jan 7, 2025 that will close this issue
Draft
@Dimi1010 Dimi1010 linked a pull request Jan 7, 2025 that will close this issue
Allow gratuitous ARP requests. #1684
Draft
@Dimi1010
Copy link
Collaborator

Dimi1010 commented Jan 7, 2025 •
edited
Loading

Opened a PR (#1684) with potential implementation on how it can be done, if you could look it over @tigercosmos.

@seladb
Copy link
Owner

seladb commented Jan 8, 2025

I think the simplest option is adding a boolean flag to ArpLayer: m_IsGratuitous that is set false by default. Then add a new c'tor that accepts opCode, senderMacAddr and senderIpAddr and sets m_IsGratuitous to true. Then computeCalculateFields() can use this flag to calculate targetMacAddr according to the flag

@Dimi1010
Copy link
Collaborator

Dimi1010 commented Jan 8, 2025 •
edited
Loading

I think the simplest option is adding a boolean flag to ArpLayer: m_IsGratuitous that is set false by default. Then add a new c'tor that accepts opCode, senderMacAddr and senderIpAddr and sets m_IsGratuitous to true. Then computeCalculateFields() can use this flag to calculate targetMacAddr according to the flag

Why are we actually zeroing the target MAC in the compute instead of checking it in the ctor?

Also, could you look at the draft PR I linked? I know the ARP test fails, but I would appreciate feedback on the concept.

@tigercosmos
Copy link
Collaborator

I left some comments on @Dimi1010's PR. I think it's fine. It is even clearer than adding another argument flag in the constructor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow gratuitous ARP requests. Dimi1010/PcapPlusPlus
4 participants
@seladb @tigercosmos @Dimi1010 @wangchong2023