Integer underflow when parsing zeroed out RTA_MULTIPATH packets #152
Comments
|
The I don't have much time now, will check later. |
|
Oh, you are right. Now I get it. Will fix it once I got free time. |
|
The iproute code while (len >= sizeof(*nh)) {
...
len -= NLMSG_ALIGN(nh->rtnh_len);
nh = RTNH_NEXT(nh);
close_json_object();
}With So it will cause dead loop in iproute2 for data struct above as Hence I think |
For buffer `[0u8; 8]`, the `RouteNextHopBuffer::attributes()` will panic
on `self.length() - 8` as `0u16 -8` will trigger underflow.
Fixed by raising error when `self.length() < PAYLOAD_OFFSET`.
We consider above all zero buffer as invalid data type instead of
skipping it during pasing is because:
1. The length property is invalid, it should always bigger or equal than 8
because it is the length of payload plus header.
2. The same data will cause error in iproute2 also:
The iproute code `print_rta_multipath()` has:
```c
while (len >= sizeof(*nh)) {
...
len -= NLMSG_ALIGN(nh->rtnh_len);
nh = RTNH_NEXT(nh);
close_json_object();
}
```
The `RTNH_NEXT` is defined as
```c
((struct rtnexthop*)(((char*)(rtnh)) + RTNH_ALIGN((rtnh)->rtnh_len)))
```
It will cause dead loop in iproute2 for `[0u8;8]` above as
NLMSG_ALIGN(0) is still 0.
Resolves: rust-netlink#152
Signed-off-by: Gris Ge <[email protected]>
|
FYI, it looks like most code in the Linux kernel actual processes these hops in a |
|
As point I made in PR #153, I think it is a invalid packet and raise error is better than tolerate. @little-dude @JohnTitor your votes? |
For buffer `[0u8; 8]`, the `RouteNextHopBuffer::attributes()` will panic
on `self.length() - 8` as `0u16 -8` will trigger underflow.
Fixed by raising error when `self.length() < PAYLOAD_OFFSET`.
We consider above all zero buffer as invalid data type instead of
skipping it during pasing is because:
1. The length property is invalid, it should always bigger or equal than 8
because it is the length of payload plus header.
2. The same data will cause error in iproute2 also:
The iproute code `print_rta_multipath()` has:
```c
while (len >= sizeof(*nh)) {
...
len -= NLMSG_ALIGN(nh->rtnh_len);
nh = RTNH_NEXT(nh);
close_json_object();
}
```
The `RTNH_NEXT` is defined as
```c
((struct rtnexthop*)(((char*)(rtnh)) + RTNH_ALIGN((rtnh)->rtnh_len)))
```
It will cause dead loop in iproute2 for `[0u8;8]` above as
NLMSG_ALIGN(0) is still 0.
Resolves: rust-netlink#152
Signed-off-by: Gris Ge <[email protected]>
For buffer `[0u8; 8]`, the `RouteNextHopBuffer::attributes()` will panic
on `self.length() - 8` as `0u16 -8` will trigger underflow.
Fixed by raising error when `self.length() < PAYLOAD_OFFSET`.
We consider above all zero buffer as invalid data type instead of
skipping it during pasing is because:
1. The length property is invalid, it should always bigger or equal than 8
because it is the length of payload plus header.
2. The same data will cause error in iproute2 also:
The iproute code `print_rta_multipath()` has:
```c
while (len >= sizeof(*nh)) {
...
len -= NLMSG_ALIGN(nh->rtnh_len);
nh = RTNH_NEXT(nh);
close_json_object();
}
```
The `RTNH_NEXT` is defined as
```c
((struct rtnexthop*)(((char*)(rtnh)) + RTNH_ALIGN((rtnh)->rtnh_len)))
```
It will cause dead loop in iproute2 for `[0u8;8]` above as
NLMSG_ALIGN(0) is still 0.
Resolves: #152
Signed-off-by: Gris Ge <[email protected]>
Hello, I work on Starnix which employs the rust-netlink library. We have a fuzzer that helps catch bugs in our kernel implementation, and recently it identified a bug that I've traced down to rust-netlink. Apologies that I don't have a better reproduction environment, but do let me know if it's a barrier and I can draft something up. In any case, here's a sample C program which exemplifies the issue:
On Linux, this program cleanly completes (including the
sendmmsginvocation). When using the rust-netlink library as the socket implementation for sendmmsg, we see an integer underflow. Our forked code is slightly different from the current source, so I'll skip sharing the exact stacktrace. But the issue lies in the RTA_MULTIPATH implementation, specifically here.'With the above program, the input payload for RouteAttribute parsing will be:
In this case, the first packet is successfully deconstructed. We end up with a remaining buffer like so:
From my reading of the code, the current algorithm interprets the first two bytes as the length, which is zero. This appeases the
RouteNextHopBuffer::check_buffer_length()code, so we attempt to parse the next hop. While performing this parsing, we will attempt to slice out the attributes by taking the length of the input buffer and slicing out the payload offset (source). Since the self-reportedlength()is actually zero, we end up with the equation0 - 8, which causes the integer underflow panic.Identifying the correct fix is a little more tricky than slapping in some guards, since I'm not sure how the Netlink spec indicates we should handle this payload. It seems intuitive to me that we should just breeze past the remainder of the data, since the multi packet self-reported claim is a nothing-packet. But I figured it'd be wise to start a thread here to discuss. Any thoughts?
The text was updated successfully, but these errors were encountered: