Fix SOCK_DGRAM implementation issues - #47
Merged
Merged
Conversation
Currently there are a few implementation issues preventing use of pingtop without root privs (or access to raw sockets one way or another). - receive_one_ping function assumes that the received packet will have the ip header. This will not be the case when using SOCK_DGRAM. - receive_one_ping and ping_once incorrectly assume that packet_id will be treated the same by SOCK_RAW and SOCK_DGRAM. When using SOCK_DGRAM, the ICMP header will be validated by the networking stack and the set packet_id will be overwritten with the local source port instead. - ping_once checks for SOCK_RAW privs using the uid. This approach will fail for when using CAP_NET_RAW and possibly SUID. Signed-off-by: Paliak <91493239+Paliak@users.noreply.github.com>
Contributor
Author
|
Diff for the icmp engine as the file rename probably makes it harder to review: diff --git a/raw_icmp.py b/icmp.py
index 545e148..815391f 100644
--- a/raw_icmp.py
+++ b/icmp.py
@@ -10,7 +10,8 @@ import time
from pingtop.models import PingResult
ICMP_ECHO_REQUEST = 8
-
+ICMP_HEADER_SIZE = 8
+ICMP_ID_HEADER_OFFSET = 4
def checksum(source_bytes: bytes) -> int:
total = 0
@@ -26,10 +27,11 @@ def checksum(source_bytes: bytes) -> int:
answer = ~total & 0xFFFF
return answer >> 8 | ((answer << 8) & 0xFF00)
-
async def receive_one_ping(
- loop: asyncio.AbstractEventLoop, sock: socket.socket, packet_id: int, timeout: float
-) -> float | None:
+ loop: asyncio.AbstractEventLoop,
+ sock: socket.socket,
+ packet_id: int,
+ timeout: float) -> float | None:
deadline = loop.time() + timeout
while True:
remaining = deadline - loop.time()
@@ -39,17 +41,67 @@ async def receive_one_ping(
received_packet = await asyncio.wait_for(loop.sock_recv(sock, 1024), remaining)
except TimeoutError:
return None
+
time_received = time.time()
- icmp_header = received_packet[20:28]
- _type, _code, _checksum, this_packet_id, _sequence = struct.unpack(
- "bbHHh", icmp_header
- )
+
+ # Determine whether we received a full IPv4 packet (with IP header)
+ # or a raw ICMP packet (no IP header) as produced by SOCK_DGRAM on some systems.
+ #
+ # IPv4 first byte: 4 bits Version | 4 bits IHL
+ # - If first_nibble (version) > 0 => looks like an IPv4/IPv6 packet (expect IP header)
+ # - If first byte == 0 (0 for Echo Reply) => not an IP header
+ #
+ # IPv4 packet header:
+ #
+ # +---------------------+-------------------------------------------+
+ # | Byte 0 | Byte 1 |
+ # | V(4) | IHL(4) | Type of Service |
+ # +---------------------+-------------------------------------------+
+ # | Bytes 2-3: Total Length |
+ # +------------------------------------------------------------+
+ # | ... IP header (IHL*4 bytes) ... |
+ # +------------------------------------------------------------+
+ # | ICMP header starts here (offset = IHL*4) |
+ # +------------------------------------------------------------+
+ #
+ # ICMP packet (no IP header):
+ #
+ # +------------------------------------------------------------+
+ # | Byte 0: Type (e.g., 0 = Echo Reply) |
+ # +------------------------------------------------------------+
+ # | Byte 1: Code |
+ # +------------------------------------------------------------+
+ # | Bytes 2-3: Checksum |
+ # +------------------------------------------------------------+
+ # | Bytes 4-5: Identifier (packet_id) |
+ # +------------------------------------------------------------+
+ # | Bytes 6-7: Sequence number |
+ # +------------------------------------------------------------+
+ # | Bytes 8-: Payload (we store a 'double' timestamp at offset 0 of payload)
+ # +------------------------------------------------------------+
+
+ # get IP header length (IHL) in 32-bit words -> bytes = IHL * 4
+ icmp_header_offset = (received_packet[0] & 0x0F) * 4
+
+ # Sanity: Ensure we at least have ICMP header (8 bytes) + timestamp (double)
+ if len(received_packet) < (icmp_header_offset + ICMP_HEADER_SIZE + struct.calcsize("d")):
+ continue
+
+ # Sanity: Ensure the checksum checks out before further parsing
+ if checksum(received_packet[icmp_header_offset:]) != 0:
+ continue
+
+ # Strip IP header if present (icmp_header_offset == 0 when no IP header)
+ this_packet_id = struct.unpack_from("!H",
+ received_packet,
+ offset=(icmp_header_offset + ICMP_ID_HEADER_OFFSET))[0]
if this_packet_id == packet_id:
- bytes_size = struct.calcsize("d")
- time_sent = struct.unpack("d", received_packet[28 : 28 + bytes_size])[0]
+ # unpack the timestamp
+ time_sent = struct.unpack_from("d",
+ received_packet,
+ offset=(icmp_header_offset + ICMP_HEADER_SIZE))[0]
return time_received - time_sent
-
async def send_one_ping(
loop: asyncio.AbstractEventLoop,
sock: socket.socket,
@@ -57,18 +109,18 @@ async def send_one_ping(
packet_id: int,
packet_size: int,
) -> None:
- my_checksum = 0
- header = struct.pack("bbHHh", ICMP_ECHO_REQUEST, 0, my_checksum, packet_id, 1)
+ # payload: timestamp + padding
bytes_size = struct.calcsize("d")
- data = (packet_size - bytes_size) * b"Q"
- data = struct.pack("d", time.time()) + data
- my_checksum = checksum(header + data)
- header = struct.pack("bbHHh", ICMP_ECHO_REQUEST, 0, socket.htons(my_checksum), packet_id, 1)
- sock.connect((resolved_ip, 1))
- await loop.sock_sendall(sock, header + data)
+ data = struct.pack("d", time.time()) + (packet_size - bytes_size) * b"Q"
+ # initial header with zero checksum
+ # sequence hard coded to 1 since we're not re-using the socket
+ header = struct.pack("!BBHHH", ICMP_ECHO_REQUEST, 0, 0, packet_id, 1)
+ my_checksum = checksum(header + data)
+ header = struct.pack("!BBHHH", ICMP_ECHO_REQUEST, 0, my_checksum, packet_id, 1)
+ return await loop.sock_sendall(sock, header + data)
-class RawIcmpEngine:
+class IcmpEngine:
async def ping_once(
self, target: str, timeout: float, packet_size: int, flag: int
) -> PingResult:
@@ -79,26 +131,32 @@ class RawIcmpEngine:
return PingResult(success=False, error_message=str(exc))
icmp_proto = socket.getprotobyname("icmp")
+ packet_id = None
+ RTT = None
try:
- if os.getuid() != 0:
- sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM, icmp_proto)
- else:
+ try:
sock = socket.socket(socket.AF_INET, socket.SOCK_RAW, icmp_proto)
- except OSError as exc:
- return PingResult(success=False, resolved_ip=resolved_ip, error_message=str(exc))
-
- sock.setblocking(False)
- packet_id = (os.getpid() & 0xFF00) | (flag & 0x00FF)
- try:
+ packet_id = (os.getpid() & 0xFF00) | (flag & 0x00FF)
+ except PermissionError:
+ sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM, icmp_proto)
+ sock.setblocking(False)
+ # AF_INET requires dest port but since we're sending ICMP it doesn't matter
+ sock.connect((resolved_ip, 0))
+ # Using unprivileged SOCK_DGRAM the kernel will validate/overwrite the ICMP header
+ # This means we do not control the packet_id and it will be set to the port used
+ if packet_id is None:
+ _, port = sock.getsockname()
+ packet_id = port
await send_one_ping(loop, sock, resolved_ip, packet_id, packet_size)
- delay = await receive_one_ping(loop, sock, packet_id, timeout)
+ RTT = await receive_one_ping(loop, sock, packet_id, timeout)
except OSError as exc:
return PingResult(success=False, resolved_ip=resolved_ip, error_message=str(exc))
finally:
sock.close()
- if delay is None:
+
+ if RTT is None:
return PingResult(success=False, resolved_ip=resolved_ip)
- return PingResult(success=True, rtt_ms=delay * 1000, resolved_ip=resolved_ip)
+ return PingResult(success=True, rtt_ms=RTT * 1000, resolved_ip=resolved_ip)
async def _resolve_target(
self, loop: asyncio.AbstractEventLoop, target: str
|
Owner
Thanks so much! I need some time to read this |
Owner
|
merged, thanks |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently there are a few implementation issues preventing use of pingtop without root privs (or access to raw sockets one way or another).
I also added some documentation around the packet parsing in receive_one_ping and renamed RawIcmpEngine to IcmpEngine as it no longer handles just the raw sockets.