Bug 239975
| Summary: | ping(8) crashes with SIGSEGV - Out-of-Bounds Read of size 2 (global-buffer-overflow) | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Neeraj <neerajpal09> | ||||||||
| Component: | bin | Assignee: | freebsd-bugs (Nobody) <bugs> | ||||||||
| Status: | New --- | ||||||||||
| Severity: | Affects Many People | CC: | ckyzee, maxim, neerajpal09 | ||||||||
| Priority: | --- | Keywords: | crash, patch, security | ||||||||
| Version: | CURRENT | ||||||||||
| Hardware: | Any | ||||||||||
| OS: | Any | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 206713 [details]
proposed patch
Confirmed that the proposed patches from Neeraj on 2019-08-19 in both Bug 239975 and Bug 239974 work. To clear the segmentation fault, additional patch required for /usr/src/sbin/ping/tests/in_cksum_test.c as attached. Created attachment 211225 [details]
Additional patch
Comment on attachment 211225 [details] Additional patch >diff --git a/sbin/ping/tests/in_cksum_test.c b/sbin/ping/tests/in_cksum_test.c >index fc266545b43..d172a4cabc1 100644 >--- a/sbin/ping/tests/in_cksum_test.c >+++ b/sbin/ping/tests/in_cksum_test.c >@@ -1,146 +1,149 @@ > /*- > * SPDX-License-Identifier: BSD-2-Clause-FreeBSD > * > * Copyright (C) 2019 Jan Sucan <jansucan@FreeBSD.org> > * All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions > * are met: > * 1. Redistributions of source code must retain the above copyright > * notice, this list of conditions and the following disclaimer. > * 2. Redistributions in binary form must reproduce the above copyright > * notice, this list of conditions and the following disclaimer in the > * documentation and/or other materials provided with the distribution. > * > * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND > * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE > * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS > * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY > * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > * SUCH DAMAGE. > */ > > #include <sys/cdefs.h> > __FBSDID("$FreeBSD$"); > > #include <sys/param.h> > > #include <atf-c.h> > >+#include <sys/socket.h> >+#include "../../../include/protocols/routed.h" > > #include "../utils.h" > > /* > * Test cases. > */ > > ATF_TC_WITHOUT_HEAD(aligned_even_length_big_endian); > ATF_TC_BODY(aligned_even_length_big_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x12, 0x34, 0x56, 0x78}; > u_short sum; > >- sum = in_cksum(data, nitems(data)); >+ sum = in_cksum(data, sizeof(struct rip), MAXPACKETSIZE, nitems(data)); > ATF_REQUIRE(sum == 0x5397); > } > > ATF_TC_WITHOUT_HEAD(aligned_odd_length_big_endian); > ATF_TC_BODY(aligned_odd_length_big_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x12, 0x34, 0x56, 0x78, 0x9a}; > u_short sum; > >- sum = in_cksum(data, nitems(data)); >+ sum = in_cksum(data, sizeof(struct rip), MAXPACKETSIZE, nitems(data)); > ATF_REQUIRE(sum == 0x52fd); > } > > ATF_TC_WITHOUT_HEAD(aligned_even_length_little_endian); > ATF_TC_BODY(aligned_even_length_little_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x34, 0x12, 0x78, 0x56}; > u_short sum; > >- sum = in_cksum(data, nitems(data)); >+ sum = in_cksum(data, sizeof(struct rip), MAXPACKETSIZE, nitems(data)); > ATF_REQUIRE_MSG(sum == 0x9753, "%d", sum); > } > > ATF_TC_WITHOUT_HEAD(aligned_odd_length_little_endian); > ATF_TC_BODY(aligned_odd_length_little_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x34, 0x12, 0x78, 0x56, 0x00, 0x9a}; > u_short sum; > >- sum = in_cksum(data, nitems(data)); >+ sum = in_cksum(data, sizeof(struct rip), MAXPACKETSIZE, nitems(data)); > ATF_REQUIRE(sum == 0xfd52); > } > > ATF_TC_WITHOUT_HEAD(unaligned_even_length_big_endian); > ATF_TC_BODY(unaligned_even_length_big_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x00, 0x12, 0x34, 0x56, 0x78}; > u_short sum; > >- sum = in_cksum(data + 1, nitems(data) - 1); >+ sum = in_cksum(data + 1, sizeof(struct rip), MAXPACKETSIZE, nitems(data) - 1); > ATF_REQUIRE(sum == 0x5397); > } > > ATF_TC_WITHOUT_HEAD(unaligned_odd_length_big_endian); > ATF_TC_BODY(unaligned_odd_length_big_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x00, 0x12, 0x34, 0x56, 0x78, 0x9a}; > u_short sum; > >- sum = in_cksum(data + 1, nitems(data) - 1); >+ sum = in_cksum(data + 1, sizeof(struct rip), MAXPACKETSIZE, nitems(data) - 1); > ATF_REQUIRE(sum == 0x52fd); > } > > ATF_TC_WITHOUT_HEAD(unaligned_even_length_little_endian); > ATF_TC_BODY(unaligned_even_length_little_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x00, 0x34, 0x12, 0x78, 0x56}; > u_short sum; > >- sum = in_cksum(data + 1, nitems(data) - 1); >+ sum = in_cksum(data + 1, sizeof(struct rip), MAXPACKETSIZE, nitems(data) - 1); > ATF_REQUIRE_MSG(sum == 0x9753, "%d", sum); > } > > ATF_TC_WITHOUT_HEAD(unaligned_odd_length_little_endian); > ATF_TC_BODY(unaligned_odd_length_little_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x00, 0x34, 0x12, 0x78, 0x56, 0x00, 0x9a}; > u_short sum; > >- sum = in_cksum(data + 1, nitems(data) - 1); >+ sum = in_cksum(data + 1, sizeof(struct rip), MAXPACKETSIZE, nitems(data) - 1); > ATF_REQUIRE(sum == 0xfd52); > } > > /* > * Main. > */ > > ATF_TP_ADD_TCS(tp) > { > ATF_TP_ADD_TC(tp, aligned_even_length_big_endian); > ATF_TP_ADD_TC(tp, aligned_odd_length_big_endian); > ATF_TP_ADD_TC(tp, aligned_even_length_little_endian); > ATF_TP_ADD_TC(tp, aligned_odd_length_little_endian); > ATF_TP_ADD_TC(tp, unaligned_even_length_big_endian); > ATF_TP_ADD_TC(tp, unaligned_odd_length_big_endian); > ATF_TP_ADD_TC(tp, unaligned_even_length_little_endian); > ATF_TP_ADD_TC(tp, unaligned_odd_length_little_endian); > > return (atf_no_error()); > } Comment on attachment 211225 [details] Additional patch >diff --git a/sbin/ping/tests/in_cksum_test.c b/sbin/ping/tests/in_cksum_test.c >index fc266545b43..d172a4cabc1 100644 >--- a/sbin/ping/tests/in_cksum_test.c >+++ b/sbin/ping/tests/in_cksum_test.c >@@ -1,146 +1,149 @@ > /*- > * SPDX-License-Identifier: BSD-2-Clause-FreeBSD > * > * Copyright (C) 2019 Jan Sucan <jansucan@FreeBSD.org> > * All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions > * are met: > * 1. Redistributions of source code must retain the above copyright > * notice, this list of conditions and the following disclaimer. > * 2. Redistributions in binary form must reproduce the above copyright > * notice, this list of conditions and the following disclaimer in the > * documentation and/or other materials provided with the distribution. > * > * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND > * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE > * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS > * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY > * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > * SUCH DAMAGE. > */ > > #include <sys/cdefs.h> > __FBSDID("$FreeBSD$"); > > #include <sys/param.h> > > #include <atf-c.h> > >+#include <sys/socket.h> >+#include "../../../include/protocols/routed.h" > > #include "../utils.h" > > /* > * Test cases. > */ > > ATF_TC_WITHOUT_HEAD(aligned_even_length_big_endian); > ATF_TC_BODY(aligned_even_length_big_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x12, 0x34, 0x56, 0x78}; > u_short sum; > >- sum = in_cksum(data, nitems(data)); >+ sum = in_cksum(data, sizeof(struct rip), MAXPACKETSIZE, nitems(data)); > ATF_REQUIRE(sum == 0x5397); > } > > ATF_TC_WITHOUT_HEAD(aligned_odd_length_big_endian); > ATF_TC_BODY(aligned_odd_length_big_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x12, 0x34, 0x56, 0x78, 0x9a}; > u_short sum; > >- sum = in_cksum(data, nitems(data)); >+ sum = in_cksum(data, sizeof(struct rip), MAXPACKETSIZE, nitems(data)); > ATF_REQUIRE(sum == 0x52fd); > } > > ATF_TC_WITHOUT_HEAD(aligned_even_length_little_endian); > ATF_TC_BODY(aligned_even_length_little_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x34, 0x12, 0x78, 0x56}; > u_short sum; > >- sum = in_cksum(data, nitems(data)); >+ sum = in_cksum(data, sizeof(struct rip), MAXPACKETSIZE, nitems(data)); > ATF_REQUIRE_MSG(sum == 0x9753, "%d", sum); > } > > ATF_TC_WITHOUT_HEAD(aligned_odd_length_little_endian); > ATF_TC_BODY(aligned_odd_length_little_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x34, 0x12, 0x78, 0x56, 0x00, 0x9a}; > u_short sum; > >- sum = in_cksum(data, nitems(data)); >+ sum = in_cksum(data, sizeof(struct rip), MAXPACKETSIZE, nitems(data)); > ATF_REQUIRE(sum == 0xfd52); > } > > ATF_TC_WITHOUT_HEAD(unaligned_even_length_big_endian); > ATF_TC_BODY(unaligned_even_length_big_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x00, 0x12, 0x34, 0x56, 0x78}; > u_short sum; > >- sum = in_cksum(data + 1, nitems(data) - 1); >+ sum = in_cksum(data + 1, sizeof(struct rip), MAXPACKETSIZE, nitems(data) - 1); > ATF_REQUIRE(sum == 0x5397); > } > > ATF_TC_WITHOUT_HEAD(unaligned_odd_length_big_endian); > ATF_TC_BODY(unaligned_odd_length_big_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x00, 0x12, 0x34, 0x56, 0x78, 0x9a}; > u_short sum; > >- sum = in_cksum(data + 1, nitems(data) - 1); >+ sum = in_cksum(data + 1, sizeof(struct rip), MAXPACKETSIZE, nitems(data) - 1); > ATF_REQUIRE(sum == 0x52fd); > } > > ATF_TC_WITHOUT_HEAD(unaligned_even_length_little_endian); > ATF_TC_BODY(unaligned_even_length_little_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x00, 0x34, 0x12, 0x78, 0x56}; > u_short sum; > >- sum = in_cksum(data + 1, nitems(data) - 1); >+ sum = in_cksum(data + 1, sizeof(struct rip), MAXPACKETSIZE, nitems(data) - 1); > ATF_REQUIRE_MSG(sum == 0x9753, "%d", sum); > } > > ATF_TC_WITHOUT_HEAD(unaligned_odd_length_little_endian); > ATF_TC_BODY(unaligned_odd_length_little_endian, tc) > { > u_char data[] __aligned(sizeof(u_short)) = > {0x00, 0x34, 0x12, 0x78, 0x56, 0x00, 0x9a}; > u_short sum; > >- sum = in_cksum(data + 1, nitems(data) - 1); >+ sum = in_cksum(data + 1, sizeof(struct rip), MAXPACKETSIZE, nitems(data) - 1); > ATF_REQUIRE(sum == 0xfd52); > } > > /* > * Main. > */ > > ATF_TP_ADD_TCS(tp) > { > ATF_TP_ADD_TC(tp, aligned_even_length_big_endian); > ATF_TP_ADD_TC(tp, aligned_odd_length_big_endian); > ATF_TP_ADD_TC(tp, aligned_even_length_little_endian); > ATF_TP_ADD_TC(tp, aligned_odd_length_little_endian); > ATF_TP_ADD_TC(tp, unaligned_even_length_big_endian); > ATF_TP_ADD_TC(tp, unaligned_odd_length_big_endian); > ATF_TP_ADD_TC(tp, unaligned_even_length_little_endian); > ATF_TP_ADD_TC(tp, unaligned_odd_length_little_endian); > > return (atf_no_error()); > } (In reply to Colin Zee from comment #2) thank you for confirming the patches. And, could you please let me know about how it is not clearing the segfault behaviors. Because the aditional patch that you have attached is responsible for ping test suite modification as from my patch I have modified the in_cksum code a bit, not the test suite code. So, as per my observation segfaults/crash has already cleared. but, also to clear the compilation issues for ping test suite, one additional patch might be required as I haven't modified that one. I have seen that the ping code is modified as compare to the last time when the issue was reported so attaching the modified patch as per the code revision 363556
Index: sbin/ping/ping.c
===================================================================
--- sbin/ping/ping.c (revision 363566)
+++ sbin/ping/ping.c (working copy)
@@ -1066,7 +1066,7 @@
cc = ICMP_MINLEN + phdr_len + datalen;
/* compute ICMP checksum here */
- icp.icmp_cksum = in_cksum(outpack, cc);
+ icp.icmp_cksum = in_cksum(outpack, sizeof(struct icmp), IP_MAXPACKET, cc);
/* Update icmp_cksum in the raw packet data buffer. */
memcpy(outpack + offsetof(struct icmp, icmp_cksum), &icp.icmp_cksum,
sizeof(icp.icmp_cksum));
@@ -1079,7 +1079,7 @@
/* Update ip_len in the raw packet data buffer. */
memcpy(outpackhdr + offsetof(struct ip, ip_len), &ip.ip_len,
sizeof(ip.ip_len));
- ip.ip_sum = in_cksum(outpackhdr, cc);
+ ip.ip_sum = in_cksum(outpackhdr, sizeof(struct ip), IP_MAXPACKET, cc);
/* Update ip_sum in the raw packet data buffer. */
memcpy(outpackhdr + offsetof(struct ip, ip_sum), &ip.ip_sum,
sizeof(ip.ip_sum));
Index: sbin/ping/utils.c
===================================================================
--- sbin/ping/utils.c (revision 363566)
+++ sbin/ping/utils.c (working copy)
@@ -55,7 +55,7 @@
* Checksum routine for Internet Protocol family headers (C Version)
*/
u_short
-in_cksum(u_char *addr, int len)
+in_cksum(u_char *addr, size_t struct_size, int ip_maxpacket, int len)
{
int nleft, sum;
u_char *w;
@@ -74,7 +74,7 @@
* sequential 16 bit words to it, and at the end, fold back all the
* carry bits from the top 16 bits into the lower 16 bits.
*/
- while (nleft > 1) {
+ while ((nleft > 1) && (w < &addr[ip_maxpacket - struct_size - sizeof(u_short)])) {
u_short data;
memcpy(&data, w, sizeof(data));
Index: sbin/ping/utils.h
===================================================================
--- sbin/ping/utils.h (revision 363566)
+++ sbin/ping/utils.h (working copy)
@@ -33,6 +33,6 @@
#include <sys/types.h>
-u_short in_cksum(u_char *, int);
+u_short in_cksum(u_char *, size_t, int, int);
#endif
Created attachment 216784 [details]
latest modified patch as per the lastest code revision 363556 due to some variables modified
Hi Neeraj, Mark suggested less intrusive changes in https://reviews.freebsd.org/D25622 which also fixes other issues reported by you: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=239974 https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=239978 Maxim |
ping(8) crashes with Segmentation Fault due to Out-0f-Bound Read of size 2, causing global-buffer-overflow * in_cksum /usr/src/sbin/ping/utils.c:80:3 Compiled with address sanitizer option "-fsanitize=address" on clang/gcc to verify the reproducibility with detailed log info. [Steps to reproduce] * compile: make CC=clang CFLAGS="-fsanitize=address -O0" * /usr/obj/usr/src/amd64.amd64/sbin/ping/ping -G 123456 -g 123456 localhost without compiling with sanitizer, it crashes when: * ping -G 123456 -g 123456 localhost Sanitizer log as follows: root@freebsd:/usr/src/sbin/ping # /usr/obj/usr/src/amd64.amd64/sbin/ping/ping -G 123456 -g 123456 localhost PING localhost (127.0.0.1): (123456 ... 123456) data bytes ================================================================= ==4196==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000000bad87f at pc 0x0000002414c5 bp 0x7ffffffed310 sp 0x7ffffffecac0 READ of size 2 at 0x000000bad87f thread T0 #0 0x2414c4 in __asan_memcpy /usr/src/contrib/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23:3 #1 0x2ef73a in in_cksum /usr/src/sbin/ping/utils.c:80:3 #2 0x2e8f30 in pinger /usr/src/sbin/ping/ping.c:1050:20 #3 0x2e5dd9 in main /usr/src/sbin/ping/ping.c:874:3 #4 0x23a10e in _start /usr/src/lib/csu/amd64/crt1.c:76:7 0x000000bad87f is located 0 bytes to the right of global variable 'outpackhdr' defined in '/usr/src/sbin/ping/ping.c:172:15' (0xb9d880) of size 65535 SUMMARY: AddressSanitizer: global-buffer-overflow /usr/src/contrib/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23:3 in __asan_memcpy Shadow bytes around the buggy address: 0x400000175ab0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x400000175ac0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x400000175ad0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x400000175ae0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x400000175af0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x400000175b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00[07] 0x400000175b10: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x400000175b20: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x400000175b30: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x400000175b40: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x400000175b50: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==4196==ABORTING Note: root privilege required.