From 7569a48cef2ce7acf628c26e97ef20ac1d95a6d5 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Fri, 24 Jan 2020 10:41:35 +1300 Subject: [PATCH 1/9] librpc ndr: Heap-buffer-overflow in lzxpress_decompress Reproducer for oss-fuzz Issue 20083 Project: samba Fuzzing Engine: libFuzzer Fuzz Target: fuzz_ndr_drsuapi_TYPE_OUT Job Type: libfuzzer_asan_samba Platform Id: linux Crash Type: Heap-buffer-overflow READ 1 Crash Address: 0x6040000002fd Crash State: lzxpress_decompress ndr_pull_compression_xpress_chunk ndr_pull_compression_start Sanitizer: address (ASAN) Recommended Security Severity: Medium Credit to OSS-Fuzz REF: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20083 BUG: https://bugzilla.samba.org/show_bug.cgi?id=14236 Signed-off-by: Gary Lockyer --- python/samba/tests/blackbox/ndrdump.py | 13 +++++++++++++ selftest/knownfail.d/bug-14236 | 1 + 2 files changed, 14 insertions(+) create mode 100644 selftest/knownfail.d/bug-14236 diff --git a/python/samba/tests/blackbox/ndrdump.py b/python/samba/tests/blackbox/ndrdump.py index b3c837819b1..ef5a3edb04e 100644 --- a/python/samba/tests/blackbox/ndrdump.py +++ b/python/samba/tests/blackbox/ndrdump.py @@ -437,3 +437,16 @@ dump OK self.fail(e) self.assertEqual(actual, expected) + + def test_ndrdump_fuzzed_ndr_compression(self): + expected = 'pull returned Buffer Size Error' + command = ( + "ndrdump drsuapi 3 out --base64-input " + "--input BwAAAAcAAAAGAAAAAwAgICAgICAJAAAAICAgIAkAAAAgIAAA//////8=") + try: + actual = self.check_exit_code(command, 2) + except BlackboxProcessError as e: + self.fail(e) + # check_output will return bytes + # convert expected to bytes for python 3 + self.assertRegex(actual.decode('utf8'), expected) diff --git a/selftest/knownfail.d/bug-14236 b/selftest/knownfail.d/bug-14236 new file mode 100644 index 00000000000..64b956997a6 --- /dev/null +++ b/selftest/knownfail.d/bug-14236 @@ -0,0 +1 @@ +^samba.tests.blackbox.ndrdump.samba.tests.blackbox.ndrdump.NdrDumpTests.test_ndrdump_fuzzed_ndr_compression -- 2.17.1 From adbf4848404d5fc62734656a559e97d9354a7ac1 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Wed, 22 Jan 2020 14:19:54 +1300 Subject: [PATCH 2/9] librpc ndr: Check size in ndr_pull_compression Add a length check to fix oss-fuzz Issue 20083. Credit to OSS-Fuzz REF: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20083 BUG: https://bugzilla.samba.org/show_bug.cgi?id=14236 Signed-off-by: Gary Lockyer --- librpc/ndr/ndr_compression.c | 1 + selftest/knownfail.d/bug-14236 | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) delete mode 100644 selftest/knownfail.d/bug-14236 diff --git a/librpc/ndr/ndr_compression.c b/librpc/ndr/ndr_compression.c index 8838c2fad72..949bb68311f 100644 --- a/librpc/ndr/ndr_compression.c +++ b/librpc/ndr/ndr_compression.c @@ -557,6 +557,7 @@ static enum ndr_err_code ndr_pull_compression_xpress_chunk(struct ndr_pull *ndrp } NDR_CHECK(ndr_pull_uint32(ndrpull, NDR_SCALARS, &comp_chunk_size)); + NDR_PULL_NEED_BYTES(ndrpull, comp_chunk_size); comp_chunk_offset = ndrpull->offset; NDR_CHECK(ndr_pull_advance(ndrpull, comp_chunk_size)); diff --git a/selftest/knownfail.d/bug-14236 b/selftest/knownfail.d/bug-14236 deleted file mode 100644 index 64b956997a6..00000000000 --- a/selftest/knownfail.d/bug-14236 +++ /dev/null @@ -1 +0,0 @@ -^samba.tests.blackbox.ndrdump.samba.tests.blackbox.ndrdump.NdrDumpTests.test_ndrdump_fuzzed_ndr_compression -- 2.17.1 From f71cc7e3c49472e1354f8c0934b05cb4f0842fe9 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Fri, 24 Jan 2020 13:26:43 +1300 Subject: [PATCH 3/9] librpc ndr tests: uint32 overflow in NDR_NEED_BYTES Check that uint32 overflow is handled correctly by NDR_NEED_BYTES. Credit to OSS-Fuzz REF: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20083 BUG: https://bugzilla.samba.org/show_bug.cgi?id=14236 Signed-off-by: Gary Lockyer --- librpc/tests/test_ndr.c | 84 ++++++++++++++++++++++++++++++++++ librpc/wscript_build | 8 ++++ selftest/knownfail.d/bug-14236 | 1 + source4/selftest/tests.py | 2 + 4 files changed, 95 insertions(+) create mode 100644 librpc/tests/test_ndr.c create mode 100644 selftest/knownfail.d/bug-14236 diff --git a/librpc/tests/test_ndr.c b/librpc/tests/test_ndr.c new file mode 100644 index 00000000000..1c074d71023 --- /dev/null +++ b/librpc/tests/test_ndr.c @@ -0,0 +1,84 @@ +/* + * Tests for librpc ndr functions + * + * Copyright (C) Catalyst.NET Ltd 2020 + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +/* + * from cmocka.c: + * These headers or their equivalents should be included prior to + * including + * this header file. + * + * #include + * #include + * #include + * + * This allows test applications to use custom definitions of C standard + * library functions and types. + * + */ +#include +#include +#include +#include +#include + +#include "librpc/ndr/libndr.h" + +/* + * Test NDR_PULL_NEED_BYTES integer overflow handling. + */ +static enum ndr_err_code wrap_NDR_PULL_NEED_BYTES( + struct ndr_pull *ndr, + uint32_t bytes) { + + NDR_PULL_NEED_BYTES(ndr, bytes); + return NDR_ERR_SUCCESS; +} + +static void test_NDR_PULL_NEED_BYTES(void **state) +{ + struct ndr_pull ndr = {0}; + enum ndr_err_code err; + + ndr.data_size = UINT32_MAX; + ndr.offset = UINT32_MAX -1; + + /* + * This will not cause an overflow + */ + err = wrap_NDR_PULL_NEED_BYTES(&ndr, 1); + assert_int_equal(NDR_ERR_SUCCESS, err); + + /* + * This will cause an overflow + * and (offset + n) will be less than data_size + */ + err = wrap_NDR_PULL_NEED_BYTES(&ndr, 2); + assert_int_equal(NDR_ERR_BUFSIZE, err); +} + +int main(int argc, const char **argv) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_NDR_PULL_NEED_BYTES), + }; + + cmocka_set_message_output(CM_OUTPUT_SUBUNIT); + return cmocka_run_group_tests(tests, NULL, NULL); +} diff --git a/librpc/wscript_build b/librpc/wscript_build index 5eb78e6010a..ec8697fbcc5 100644 --- a/librpc/wscript_build +++ b/librpc/wscript_build @@ -698,3 +698,11 @@ bld.SAMBA_BINARY('test_ndr_string', ndr ''', for_selftest=True) + +bld.SAMBA_BINARY('test_ndr', + source='tests/test_ndr.c', + deps=''' + cmocka + ndr + ''', + for_selftest=True) diff --git a/selftest/knownfail.d/bug-14236 b/selftest/knownfail.d/bug-14236 new file mode 100644 index 00000000000..91ef5e249be --- /dev/null +++ b/selftest/knownfail.d/bug-14236 @@ -0,0 +1 @@ +^librpc.ndr.ndr.test_NDR_PULL_NEED_BYTES diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index f570d35dfba..ab2c4f69da0 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -1334,6 +1334,8 @@ plantestsuite("libcli.drsuapi.repl_decrypt", "none", [os.path.join(bindir(), "test_repl_decrypt")]) plantestsuite("librpc.ndr.ndr_string", "none", [os.path.join(bindir(), "test_ndr_string")]) +plantestsuite("librpc.ndr.ndr", "none", + [os.path.join(bindir(), "test_ndr")]) # process restart and limit tests, these break the environment so need to run # in their own specific environment -- 2.17.1 From 63b125bb74986929116e43ac500e9879beaf35a3 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Wed, 22 Jan 2020 14:13:50 +1300 Subject: [PATCH 4/9] librpc ndr: NDR_NEED_BYTES check for unsigned overflow. Handle uint32 overflow in NDR_NEED_BYTES Credit to OSS-Fuzz REF: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20083 BUG: https://bugzilla.samba.org/show_bug.cgi?id=14236 Signed-off-by: Gary Lockyer --- librpc/ndr/libndr.h | 5 ++++- selftest/knownfail.d/bug-14236 | 1 - 2 files changed, 4 insertions(+), 2 deletions(-) delete mode 100644 selftest/knownfail.d/bug-14236 diff --git a/librpc/ndr/libndr.h b/librpc/ndr/libndr.h index 58ef517d363..b7cccf3dfc5 100644 --- a/librpc/ndr/libndr.h +++ b/librpc/ndr/libndr.h @@ -309,7 +309,10 @@ enum ndr_compression_alg { } while (0) #define NDR_PULL_NEED_BYTES(ndr, n) do { \ - if (unlikely((n) > ndr->data_size || ndr->offset + (n) > ndr->data_size)) { \ + if (unlikely(\ + (n) > ndr->data_size || \ + ndr->offset + (n) > ndr->data_size || \ + ndr->offset + (n) < ndr->offset)) { \ if (ndr->flags & LIBNDR_FLAG_INCOMPLETE_BUFFER) { \ uint32_t _available = ndr->data_size - ndr->offset; \ uint32_t _missing = n - _available; \ diff --git a/selftest/knownfail.d/bug-14236 b/selftest/knownfail.d/bug-14236 deleted file mode 100644 index 91ef5e249be..00000000000 --- a/selftest/knownfail.d/bug-14236 +++ /dev/null @@ -1 +0,0 @@ -^librpc.ndr.ndr.test_NDR_PULL_NEED_BYTES -- 2.17.1 From ea15419edd3ac9857045e43f28502982f64adf53 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Fri, 24 Jan 2020 15:21:47 +1300 Subject: [PATCH 5/9] librpc ndr tests: uint32 overflow in NDR_PULL_ALIGN Check that uint32 overflow is handled correctly by NDR_NEED_BYTES. Credit to OSS-Fuzz REF: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20083 BUG: https://bugzilla.samba.org/show_bug.cgi?id=14236 Signed-off-by: Gary Lockyer --- librpc/tests/test_ndr.c | 34 ++++++++++++++++++++++++++++++++++ selftest/knownfail.d/bug-14236 | 1 + 2 files changed, 35 insertions(+) create mode 100644 selftest/knownfail.d/bug-14236 diff --git a/librpc/tests/test_ndr.c b/librpc/tests/test_ndr.c index 1c074d71023..a2a3834385d 100644 --- a/librpc/tests/test_ndr.c +++ b/librpc/tests/test_ndr.c @@ -73,10 +73,44 @@ static void test_NDR_PULL_NEED_BYTES(void **state) assert_int_equal(NDR_ERR_BUFSIZE, err); } +/* + * Test NDR_PULL_ALIGN integer overflow handling. + */ +static enum ndr_err_code wrap_NDR_PULL_ALIGN( + struct ndr_pull *ndr, + uint32_t bytes) { + + NDR_PULL_ALIGN(ndr, bytes); + return NDR_ERR_SUCCESS; +} + +static void test_NDR_PULL_ALIGN(void **state) +{ + struct ndr_pull ndr = {0}; + enum ndr_err_code err; + + ndr.data_size = UINT32_MAX; + ndr.offset = UINT32_MAX -1; + + /* + * This will not cause an overflow + */ + err = wrap_NDR_PULL_ALIGN(&ndr, 2); + assert_int_equal(NDR_ERR_SUCCESS, err); + + /* + * This will cause an overflow + * and (offset + n) will be less than data_size + */ + err = wrap_NDR_PULL_ALIGN(&ndr, 4); + assert_int_equal(NDR_ERR_BUFSIZE, err); +} + int main(int argc, const char **argv) { const struct CMUnitTest tests[] = { cmocka_unit_test(test_NDR_PULL_NEED_BYTES), + cmocka_unit_test(test_NDR_PULL_ALIGN), }; cmocka_set_message_output(CM_OUTPUT_SUBUNIT); diff --git a/selftest/knownfail.d/bug-14236 b/selftest/knownfail.d/bug-14236 new file mode 100644 index 00000000000..ddd3c1c3d94 --- /dev/null +++ b/selftest/knownfail.d/bug-14236 @@ -0,0 +1 @@ +^librpc.ndr.ndr.test_NDR_PULL_ALIGN -- 2.17.1 From 1faf96f7e3c5ca524d603a3891a9960f6b7bc804 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Wed, 22 Jan 2020 14:16:02 +1300 Subject: [PATCH 6/9] librpc ndr: NDR_PULL_ALIGN check for unsigned overflow Handle uint32 overflow in NDR_PULL_ALIGN Credit to OSS-Fuzz REF: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20083 BUG: https://bugzilla.samba.org/show_bug.cgi?id=14236 Signed-off-by: Gary Lockyer --- librpc/ndr/libndr.h | 7 +++++++ selftest/knownfail.d/bug-14236 | 1 - 2 files changed, 7 insertions(+), 1 deletion(-) delete mode 100644 selftest/knownfail.d/bug-14236 diff --git a/librpc/ndr/libndr.h b/librpc/ndr/libndr.h index b7cccf3dfc5..c2c7e263049 100644 --- a/librpc/ndr/libndr.h +++ b/librpc/ndr/libndr.h @@ -331,6 +331,13 @@ enum ndr_compression_alg { if (unlikely(ndr->flags & LIBNDR_FLAG_PAD_CHECK)) { \ ndr_check_padding(ndr, n); \ } \ + if(unlikely( \ + ((ndr->offset + (n-1)) & (~(n-1))) < ndr->offset)) {\ + return ndr_pull_error( \ + ndr, \ + NDR_ERR_BUFSIZE, \ + "Pull align (overflow) %u", (unsigned)n); \ + } \ ndr->offset = (ndr->offset + (n-1)) & ~(n-1); \ } \ if (unlikely(ndr->offset > ndr->data_size)) { \ diff --git a/selftest/knownfail.d/bug-14236 b/selftest/knownfail.d/bug-14236 deleted file mode 100644 index ddd3c1c3d94..00000000000 --- a/selftest/knownfail.d/bug-14236 +++ /dev/null @@ -1 +0,0 @@ -^librpc.ndr.ndr.test_NDR_PULL_ALIGN -- 2.17.1 From 81d70433126e4e1d66b4e4cc160a8a10f9ca5150 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Mon, 27 Jan 2020 10:06:55 +1300 Subject: [PATCH 7/9] librpc ndr tests: Unsigned overflow in ndr_pull_advance Check that uint32 overflow is handled correctly by ndr_pull_advance. Credit to OSS-Fuzz REF: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20083 BUG: https://bugzilla.samba.org/show_bug.cgi?id=14236 Signed-off-by: Gary Lockyer --- librpc/tests/test_ndr.c | 26 ++++++++++++++++++++++++++ selftest/knownfail.d/bug-14236 | 1 + 2 files changed, 27 insertions(+) create mode 100644 selftest/knownfail.d/bug-14236 diff --git a/librpc/tests/test_ndr.c b/librpc/tests/test_ndr.c index a2a3834385d..316c54368a0 100644 --- a/librpc/tests/test_ndr.c +++ b/librpc/tests/test_ndr.c @@ -106,11 +106,37 @@ static void test_NDR_PULL_ALIGN(void **state) assert_int_equal(NDR_ERR_BUFSIZE, err); } +/* + * Test ndr_pull_advance integer overflow handling. + */ +static void test_ndr_pull_advance(void **state) +{ + struct ndr_pull ndr = {0}; + enum ndr_err_code err; + + ndr.data_size = UINT32_MAX; + ndr.offset = UINT32_MAX -1; + + /* + * This will not cause an overflow + */ + err = ndr_pull_advance(&ndr, 1); + assert_int_equal(NDR_ERR_SUCCESS, err); + + /* + * This will cause an overflow + * and (offset + n) will be less than data_size + */ + err = ndr_pull_advance(&ndr, 2); + assert_int_equal(NDR_ERR_BUFSIZE, err); +} + int main(int argc, const char **argv) { const struct CMUnitTest tests[] = { cmocka_unit_test(test_NDR_PULL_NEED_BYTES), cmocka_unit_test(test_NDR_PULL_ALIGN), + cmocka_unit_test(test_ndr_pull_advance), }; cmocka_set_message_output(CM_OUTPUT_SUBUNIT); diff --git a/selftest/knownfail.d/bug-14236 b/selftest/knownfail.d/bug-14236 new file mode 100644 index 00000000000..39079dc6be5 --- /dev/null +++ b/selftest/knownfail.d/bug-14236 @@ -0,0 +1 @@ +^librpc.ndr.ndr.test_ndr_pull_advance -- 2.17.1 From a34a1654338f7087824428f21e8c7a4fd2d7bb0e Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Wed, 15 Jan 2020 12:37:06 +1300 Subject: [PATCH 8/9] librpc ndr: ndr_pull_advance check for unsigned overflow. Handle uint32 overflow in ndr_pull_advance Credit to OSS-Fuzz REF: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20083 BUG: https://bugzilla.samba.org/show_bug.cgi?id=14236 Signed-off-by: Gary Lockyer --- librpc/ndr/ndr.c | 2 +- selftest/knownfail.d/bug-14236 | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) delete mode 100644 selftest/knownfail.d/bug-14236 diff --git a/librpc/ndr/ndr.c b/librpc/ndr/ndr.c index c772d53f6ed..28c6d68ef51 100644 --- a/librpc/ndr/ndr.c +++ b/librpc/ndr/ndr.c @@ -200,7 +200,7 @@ _PUBLIC_ enum ndr_err_code ndr_pull_pop(struct ndr_pull *ndr) _PUBLIC_ enum ndr_err_code ndr_pull_advance(struct ndr_pull *ndr, uint32_t size) { ndr->offset += size; - if (ndr->offset > ndr->data_size) { + if (ndr->offset > ndr->data_size || ndr->offset < size) { return ndr_pull_error(ndr, NDR_ERR_BUFSIZE, "ndr_pull_advance by %u failed", size); diff --git a/selftest/knownfail.d/bug-14236 b/selftest/knownfail.d/bug-14236 deleted file mode 100644 index 39079dc6be5..00000000000 --- a/selftest/knownfail.d/bug-14236 +++ /dev/null @@ -1 +0,0 @@ -^librpc.ndr.ndr.test_ndr_pull_advance -- 2.17.1 From 602f8248053ce033d52d594f84106b9f1c25a06c Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Wed, 22 Jan 2020 14:18:00 +1300 Subject: [PATCH 9/9] librpc ndr: Change loop index to size_t Change the loop index in ndr_check_padding to size_t. Credit to OSS-Fuzz REF: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20083 BUG: https://bugzilla.samba.org/show_bug.cgi?id=14236 Signed-off-by: Gary Lockyer --- librpc/ndr/ndr_basic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/librpc/ndr/ndr_basic.c b/librpc/ndr/ndr_basic.c index 0811a590971..693d9c86e3a 100644 --- a/librpc/ndr/ndr_basic.c +++ b/librpc/ndr/ndr_basic.c @@ -44,7 +44,7 @@ static void ndr_dump_data(struct ndr_print *ndr, const uint8_t *buf, int len); _PUBLIC_ void ndr_check_padding(struct ndr_pull *ndr, size_t n) { size_t ofs2 = (ndr->offset + (n-1)) & ~(n-1); - int i; + size_t i; for (i=ndr->offset;idata[i] != 0) { break; -- 2.17.1