Add buffer size checks to Net::Arp

src/net-arp.cpp

@@ -4,24 +4,30 @@
 #include "net-arp.h"
 #include "net-ethernet.h"
 
+#include "debug.h"
 #include "types.h"
 #include <uspi.h>
 
 namespace Net::Arp
 {
-	Packet::Packet()
+	Packet::Packet() {}
-	{}
 
-	Packet::Packet(uint16_t operation) :
+	Packet::Packet(const uint16_t operation) :
 		hardwareType(1), // Ethernet
 		protocolType(Ethernet::EtherType::Ipv4),
 		hardwareAddressLength(6),
 		protocolAddressLength(4),
 		operation(operation)
-	{}
-
-	size_t Packet::Serialize(uint8_t* buffer)
 	{
+	}
+
+	size_t Packet::Serialize(uint8_t* buffer, const size_t bufferSize)
+	{
+		if (bufferSize < SerializedLength())
+		{
+			return 0;
+		}
+
 		buffer[0] = hardwareType >> 8;
 		buffer[1] = hardwareType;
 		buffer[2] = static_cast<uint16_t>(protocolType) >> 8;
@@ -49,33 +55,33 @@ namespace Net::Arp
 	}
 
 	// Static
-	Packet Packet::Deserialize(const uint8_t* buffer)
+	size_t Packet::Deserialize(const uint8_t* buffer, const size_t bufferSize)
 	{
-		Packet self;
+		if (bufferSize < SerializedLength())
+		{
+			return 0;
+		}
 
-		self.hardwareType = buffer[0] << 8 | buffer[1];
+		hardwareType = buffer[0] << 8 | buffer[1];
-		self.protocolType =
+		protocolType = static_cast<Ethernet::EtherType>(buffer[2] << 8 | buffer[3]);
-			static_cast<Ethernet::EtherType>(buffer[2] << 8 | buffer[3]);
+		hardwareAddressLength = buffer[4];
-		self.hardwareAddressLength = buffer[4];
+		protocolAddressLength = buffer[5];
-		self.protocolAddressLength = buffer[5];
+		operation = buffer[6] << 8 | buffer[7];
-		self.operation = buffer[6] << 8 | buffer[7];
 
-		memcpy(self.senderMac.data(), buffer + 8, 6);
+		memcpy(senderMac.data(), buffer + 8, 6);
-		self.senderIp =
+		senderIp = buffer[14] << 24 | buffer[15] << 16 | buffer[16] << 8 | buffer[17];
-			buffer[14] << 24 | buffer[15] << 16 | buffer[16] << 8 | buffer[17];
+		memcpy(targetMac.data(), buffer + 18, 6);
-		memcpy(self.targetMac.data(), buffer + 18, 6);
+		targetIp = buffer[24] << 24 | buffer[25] << 16 | buffer[26] << 8 | buffer[27];
-		self.targetIp =
-			buffer[24] << 24 | buffer[25] << 16 | buffer[26] << 8 | buffer[27];
 
-		return self;
+		return 28;
 	}
 
 	void SendPacket(
-		Operation operation,
+		const Operation operation,
-		Utils::MacAddress targetMac,
+		const Utils::MacAddress targetMac,
-		Utils::MacAddress senderMac,
+		const Utils::MacAddress senderMac,
-		uint32_t targetIp,
+		const uint32_t targetIp,
-		uint32_t senderIp)
+		const uint32_t senderIp)
 	{
 		Packet arpPacket(operation);
 		arpPacket.targetMac = targetMac;
@@ -83,16 +89,14 @@ namespace Net::Arp
 		arpPacket.targetIp = targetIp;
 		arpPacket.senderIp = senderIp;
 
-		Ethernet::Header ethernetHeader(
+		Ethernet::Header ethernetHeader(senderMac, targetMac, Ethernet::EtherType::Arp);
-			senderMac, targetMac, Ethernet::EtherType::Arp);
 
 		uint8_t buffer[USPI_FRAME_BUFFER_SIZE];
 		size_t size = 0;
 		size += ethernetHeader.Serialize(buffer + size, sizeof(buffer) - size);
-		size += arpPacket.Serialize(buffer + size);
+		size += arpPacket.Serialize(buffer + size, sizeof(buffer) - size);
 
-		const auto expectedSize =
+		const auto expectedSize = ethernetHeader.SerializedLength() + arpPacket.SerializedLength();
-			ethernetHeader.SerializedLength() + arpPacket.SerializedLength();
 		assert(size == expectedSize);
 		assert(size <= sizeof(buffer));
 
@@ -100,56 +104,65 @@ namespace Net::Arp
 	}
 
 	void SendRequest(
-		Utils::MacAddress targetMac,
+		const Utils::MacAddress targetMac,
-		Utils::MacAddress senderMac,
+		const Utils::MacAddress senderMac,
-		uint32_t targetIp,
+		const uint32_t targetIp,
-		uint32_t senderIp
+		const uint32_t senderIp)
-	) {
+	{
 		SendPacket(ARP_OPERATION_REQUEST, targetMac, senderMac, targetIp, senderIp);
 	}
 
 	void SendReply(
-		Utils::MacAddress targetMac,
+		const Utils::MacAddress targetMac,
-		Utils::MacAddress senderMac,
+		const Utils::MacAddress senderMac,
-		uint32_t targetIp,
+		const uint32_t targetIp,
-		uint32_t senderIp
+		const uint32_t senderIp)
-	) {
+	{
 		SendPacket(ARP_OPERATION_REPLY, targetMac, senderMac, targetIp, senderIp);
 	}
 
-	void SendAnnouncement(Utils::MacAddress mac, uint32_t ip)
+	void SendAnnouncement(const Utils::MacAddress mac, const uint32_t ip)
 	{
 		SendReply(Utils::MacBroadcast, mac, ip, ip);
 	}
 
 	void HandlePacket(
-		const Ethernet::Header ethernetHeader, uint8_t* buffer
+		const Ethernet::Header ethernetHeader, const uint8_t* buffer, const size_t bufferSize)
-	) {
+	{
 		const auto macAddress = Utils::GetMacAddress();
-		const auto arpPacket = Packet::Deserialize(buffer);
 
-		if (
+		Packet arpPacket;
-			arpPacket.hardwareType == 1 &&
+		size_t arpSize = arpPacket.Deserialize(buffer, bufferSize);
-			arpPacket.protocolType == Ethernet::EtherType::Ipv4 &&
+		if (arpSize == 0 || arpSize != arpPacket.SerializedLength())
-			arpPacket.operation == ARP_OPERATION_REQUEST &&
-			arpPacket.targetIp == Utils::Ipv4Address)
 		{
-			SendReply(
+			DEBUG_LOG(
-				arpPacket.senderMac,
+				"Dropped ARP packet (invalid buffer size %lu, expected %lu)\r\n",
-				macAddress,
+				bufferSize,
-				arpPacket.senderIp,
+				arpPacket.SerializedLength());
-				Utils::Ipv4Address
+			return;
-			);
 		}
 
-		else if (
+		if (arpPacket.hardwareType != 1 || arpPacket.protocolType != Ethernet::EtherType::Ipv4 ||
-			arpPacket.hardwareType == 1 &&
+			arpPacket.targetIp != Utils::Ipv4Address)
-			arpPacket.protocolType == Ethernet::EtherType::Ipv4 &&
-			arpPacket.operation == ARP_OPERATION_REPLY &&
-			arpPacket.targetIp == Utils::Ipv4Address &&
-			arpPacket.targetMac == macAddress)
 		{
+			// Might want to disable because of spamminess
+			DEBUG_LOG("Dropped ARP packet (invalid parameters)\r\n");
+			return;
+		}
+
+		switch (arpPacket.operation)
+		{
+		case ARP_OPERATION_REQUEST:
+			SendReply(arpPacket.senderMac, macAddress, arpPacket.senderIp, Utils::Ipv4Address);
+			break;
+
+		case ARP_OPERATION_REPLY:
 			ArpTable.insert(std::make_pair(arpPacket.senderIp, arpPacket.senderMac));
+			break;
+
+		default:
+			DEBUG_LOG("Dropped ARP packet (invalid operation %d)\r\n", arpPacket.operation);
+			break;
 		}
 	}
 

src/net-arp.h

@@ -25,7 +25,7 @@ namespace Net::Arp
 		uint32_t targetIp;
 
 		Packet();
-		Packet(uint16_t operation);
+		Packet(const uint16_t operation);
 
 		constexpr size_t SerializedLength() const
 		{
@@ -41,36 +41,36 @@ namespace Net::Arp
 				sizeof(targetIp);
 		}
 
-		size_t Serialize(uint8_t* buffer);
+		size_t Serialize(uint8_t* buffer, const size_t bufferSize);
-
+		size_t Deserialize(const uint8_t* buffer, const size_t bufferSize);
-		static Packet Deserialize(const uint8_t* buffer);
 	};
 
-	void HandlePacket(Ethernet::Header header, uint8_t* buffer);
+	void HandlePacket(
+		const Ethernet::Header header, const uint8_t* buffer, const size_t bufferSize);
 
 	void SendPacket(
-		Operation operation,
+		const Operation operation,
-		Utils::MacAddress targetMac,
+		const Utils::MacAddress targetMac,
-		Utils::MacAddress senderMac,
+		const Utils::MacAddress senderMac,
-		uint32_t senderIp,
+		const uint32_t senderIp,
-		uint32_t targetIp
+		const uint32_t targetIp
 	);
 
 	void SendRequest(
-		Utils::MacAddress targetMac,
+		const Utils::MacAddress targetMac,
-		Utils::MacAddress senderMac,
+		const Utils::MacAddress senderMac,
-		uint32_t senderIp,
+		const uint32_t senderIp,
-		uint32_t targetIp
+		const uint32_t targetIp
 	);
 
 	void SendReply(
-		Utils::MacAddress targetMac,
+		const Utils::MacAddress targetMac,
-		Utils::MacAddress senderMac,
+		const Utils::MacAddress senderMac,
-		uint32_t senderIp,
+		const uint32_t senderIp,
-		uint32_t targetIp
+		const uint32_t targetIp
 	);
 
-	void SendAnnouncement(Utils::MacAddress mac, uint32_t ip);
+	void SendAnnouncement(const Utils::MacAddress mac, const uint32_t ip);
 
 	extern std::unordered_map<uint32_t, Utils::MacAddress> ArpTable;
 } // namespace Net::Arp