From 3d53b2f2a0369c2af83c738d4e8194077315cbb4 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 13 May 2021 13:22:29 -0400 Subject: [#182] Corrected CVE: CVE-2021-25217 Addressed buffer overwrite in parse_X() Added Release Note common/parse.c parse_X() - reworked to avoid buffer overwrite on over-sized hex literals common/tests/option_unittest.c ATF_TC_BODY(parse_X) - new test which verifies parse_X() logic. --- RELNOTES | 11 +++++- common/parse.c | 5 ++- common/tests/option_unittest.c | 84 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 4 deletions(-) diff --git a/RELNOTES b/RELNOTES index 76f75093..e408e656 100644 --- a/RELNOTES +++ b/RELNOTES @@ -27,7 +27,7 @@ ISC DHCP is open source software maintained by Internet Systems Consortium. This product includes cryptographic software written by Eric Young (eay@cryptsoft.com). - Changes since 4.4.2 (New Features) + Changes since 4.4.2-P1 (New Features) - BIND libraries updated to the latest version, 9.11.36. This fixes a number of compilation issues on various systems, including OpenWRT. Thanks to @@ -39,7 +39,7 @@ by Eric Young (eay@cryptsoft.com). and the client Linux script sample was updated. [Gitlab #132] - Changes since 4.4.2 (Bug Fixes) + Changes since 4.4.2-P1 (Bug Fixes) - Minor corrections to allow compilation under gcc 10. [Gitlab #117] @@ -73,6 +73,13 @@ by Eric Young (eay@cryptsoft.com). an object to fail. [Gitlab #148] + Changes since 4.4.2 (Bug Fixes) + +- Corrected a buffer overwrite possible when parsing hexadecimal + literals with more than 1024 octets. + [Gitlab #182] + CVE: CVE-2021-25217 + Changes since 4.4.2b1 (Bug Fixes) - Added a clarification on DHCPINFORMs and server authority to diff --git a/common/parse.c b/common/parse.c index 5e3cce7d..b123a6c7 100644 --- a/common/parse.c +++ b/common/parse.c @@ -5556,13 +5556,14 @@ int parse_X (cfile, buf, max) skip_to_semi (cfile); return 0; } - convert_num (cfile, &buf [len], val, 16, 8); - if (len++ > max) { + if (len >= max) { parse_warn (cfile, "hexadecimal constant too long."); skip_to_semi (cfile); return 0; } + convert_num (cfile, &buf [len], val, 16, 8); + len++; token = peek_token (&val, (unsigned *)0, cfile); if (token == COLON) token = next_token (&val, diff --git a/common/tests/option_unittest.c b/common/tests/option_unittest.c index cd52cfb4..bea60837 100644 --- a/common/tests/option_unittest.c +++ b/common/tests/option_unittest.c @@ -129,6 +129,89 @@ ATF_TC_BODY(pretty_print_option, tc) } } +ATF_TC(parse_X); + +ATF_TC_HEAD(parse_X, tc) +{ + atf_tc_set_md_var(tc, "descr", + "Verify parse_X survices option too big."); +} + +/* Initializes a parse struct from an input buffer of data. */ +static void init_parse(struct parse *cfile, char* name, char *input) { + memset(cfile, 0, sizeof(struct parse)); + cfile->tlname = name; + cfile->lpos = cfile->line = 1; + cfile->cur_line = cfile->line1; + cfile->prev_line = cfile->line2; + cfile->token_line = cfile->cur_line; + cfile->cur_line[0] = cfile->prev_line[0] = 0; + cfile->file = -1; + cfile->eol_token = 0; + + cfile->inbuf = input; + cfile->buflen = strlen(input); + cfile->bufsiz = 0; +} + +/* + * This test verifies that parse_X does not overwrite the output + * buffer when given input data that exceeds the output buffer + * capacity. +*/ +ATF_TC_BODY(parse_X, tc) +{ + struct parse cfile; + u_int8_t output[10]; + unsigned len; + + /* Input hex literal */ + char *input = "01:02:03:04:05:06:07:08"; + unsigned expected_len = 8; + + /* Normal output plus two filler bytes */ + u_int8_t expected_plus_two[] = { + 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0xff, 0xff + }; + + /* Safe output when option is too long */ + unsigned short_buf_len = 4; + u_int8_t expected_too_long[] = { + 0x01, 0x02, 0x03, 0x04, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff + }; + + /* First we'll run one that works normally */ + memset(output, 0xff, sizeof(output)); + init_parse(&cfile, "hex_fits", input); + + len = parse_X(&cfile, output, expected_len); + + // Len should match the expected len. + if (len != expected_len) { + atf_tc_fail("parse_X failed, output len: %d", len); + } + + // We should not have written anything past the end of the buffer. + if (memcmp(output, expected_plus_two, sizeof(output))) { + atf_tc_fail("parse_X failed, output does not match expected"); + } + + // Now we'll try it with a buffer that's too small. + init_parse(&cfile, "hex_too_long", input); + memset(output, 0xff, sizeof(output)); + + len = parse_X(&cfile, output, short_buf_len); + + // On errors, len should be zero. + if (len != 0) { + atf_tc_fail("parse_X failed, we should have had an error"); + } + + // We should not have written anything past the end of the buffer. + if (memcmp(output, expected_too_long, sizeof(output))) { + atf_tc_fail("parse_X overwrote buffer!"); + } +} /* This macro defines main() method that will call specified test cases. tp and simple_test_case names can be whatever you want @@ -137,6 +220,7 @@ ATF_TP_ADD_TCS(tp) { ATF_TP_ADD_TC(tp, option_refcnt); ATF_TP_ADD_TC(tp, pretty_print_option); + ATF_TP_ADD_TC(tp, parse_X); return (atf_no_error()); } -- cgit v1.2.1