From 94409101870b689f77452b6324968687d9f3c72f Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Mon, 17 Oct 2022 08:45:09 +0000 Subject: [PATCH 1/5] Fix a logical bug in cfg_print_duration() The cfg_print_duration() function prints a ISO 8601 duration value converted from an array of integers, where the parts of the date and time are stored. durationlen[6], which holds the "seconds" part of the duration, has a special case in cfg_print_duration() to ensure that when there are no values in the duration, the result still can be printed as "PT0S", instead of just "P", so it can be a valid ISO 8601 duration value. There is a logical error in one of the two special case code paths, when it checks that no value from the "date" part is defined, and no "hour" or "minute" from the "time" part are defined. Because of the error, durationlen[6] can be used uninitialized, in which case the second parameter passed to snprintf() (which is the maximum allowed length) can contain a garbage value. This can not be exploited because the buffer is still big enough to hold the maximum possible amount of characters generated by the "%u%c" format string. Fix the logical bug, and initialize the 'durationlen' array to zeros to be a little safer from other similar errors. --- lib/isccfg/parser.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/isccfg/parser.c b/lib/isccfg/parser.c index 4ea849e64f..d27e6e599a 100644 --- a/lib/isccfg/parser.c +++ b/lib/isccfg/parser.c @@ -1034,7 +1034,7 @@ cfg_print_duration(cfg_printer_t *pctx, const cfg_obj_t *obj) { char *str; const char *indicators = "YMWDHMS"; int count, i; - int durationlen[7]; + int durationlen[7] = { 0 }; isccfg_duration_t duration; /* * D ? The duration has a date part. @@ -1066,10 +1066,8 @@ cfg_print_duration(cfg_printer_t *pctx, const cfg_obj_t *obj) { } else { T = true; } - } else { - durationlen[i] = 0; + count += durationlen[i]; } - count += durationlen[i]; } /* * Special case for seconds which is not taken into account in the @@ -1107,7 +1105,7 @@ cfg_print_duration(cfg_printer_t *pctx, const cfg_obj_t *obj) { } /* Special case for seconds. */ if (duration.parts[6] > 0 || - (!D && !duration.parts[4] && !duration.parts[3])) { + (!D && !duration.parts[4] && !duration.parts[5])) { snprintf(str, durationlen[6] + 2, "%u%c", (uint32_t)duration.parts[6], indicators[6]); } From 39290bb7cd6757e1ae4126ad42a5c7b7287dd2b1 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Mon, 17 Oct 2022 08:45:18 +0000 Subject: [PATCH 2/5] Test cfg_print_duration() in duration_test.c Currently the 'duration_test' unit test checks only the cfg_obj_asduration() function. Extend the test so it checks also the reverse operation using the cfg_print_duration() function, which is used in named-checkconf. --- tests/isccfg/duration_test.c | 109 +++++++++++++++++++++++++++++------ 1 file changed, 91 insertions(+), 18 deletions(-) diff --git a/tests/isccfg/duration_test.c b/tests/isccfg/duration_test.c index 7cb6653414..b152578b82 100644 --- a/tests/isccfg/duration_test.c +++ b/tests/isccfg/duration_test.c @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -84,17 +85,34 @@ ISC_TEARDOWN_TEST_IMPL(group) { struct duration_conf { const char *string; uint32_t time; + const char *out; }; typedef struct duration_conf duration_conf_t; -/* test cfg_obj_asduration() */ -ISC_RUN_TEST_IMPL(cfg_obj_asduration) { +static void +output(void *closure, const char *text, int textlen) { + int r; + + REQUIRE(textlen >= 0 && textlen < CFG_DURATION_MAXLEN); + + r = snprintf(closure, CFG_DURATION_MAXLEN, "%s", text); + INSIST(r == textlen); +} + +/* test cfg_obj_asduration() and cfg_print_duration() */ +ISC_RUN_TEST_IMPL(duration) { isc_result_t result; + /* + * When 'out' is NULL, the printed result is expected to be the same as + * the input 'string', compared by ignoring the case of the characters. + */ duration_conf_t durations[] = { + { .string = "unlimited", .time = 0 }, { .string = "PT0S", .time = 0 }, { .string = "PT42S", .time = 42 }, { .string = "PT10m", .time = 600 }, { .string = "PT10m4S", .time = 604 }, + { .string = "PT3600S", .time = 3600 }, { .string = "pT2H", .time = 7200 }, { .string = "Pt2H3S", .time = 7203 }, { .string = "PT2h1m3s", .time = 7263 }, @@ -106,26 +124,56 @@ ISC_RUN_TEST_IMPL(cfg_obj_asduration) { { .string = "p5y", .time = 157680000 }, { .string = "P5YT2H", .time = 157687200 }, { .string = "P1Y1M1DT1H1M1S", .time = 34304461 }, + { .string = "P99Y399M999DT3999H9999M2911754S", + .time = UINT32_MAX - 1 }, + { .string = "P99Y399M999DT3999H9999M2911755S", + .time = UINT32_MAX }, + { .string = "P4294967295Y4294967295M4294967295D" + "T4294967295H4294967295M4294967295S", + .time = UINT32_MAX }, + { .string = "PT4294967294S", .time = UINT32_MAX - 1 }, + { .string = "PT4294967295S", .time = UINT32_MAX }, { .string = "0", .time = 0 }, { .string = "30", .time = 30 }, - { .string = "42s", .time = 42 }, - { .string = "10m", .time = 600 }, - { .string = "2H", .time = 7200 }, - { .string = "7d", .time = 604800 }, - { .string = "2w", .time = 1209600 }, + { .string = "42s", .time = 42, .out = "42" }, + { .string = "10m", .time = 600, .out = "600" }, + { .string = "2H", .time = 7200, .out = "7200" }, + { .string = "7d", .time = 604800, .out = "604800" }, + { .string = "2w", .time = 1209600, .out = "1209600" }, + { 0 }, /* Indicates that the remaining durations are invalid. */ + { .string = "PT4Y" }, + { .string = "P-4Y2M" }, + { .string = "P5H1M30S" }, + { .string = "P7Y4W" }, + { .string = "X7Y4M" }, + { .string = "T7H4M" }, + { .string = "1Y6M" }, + { .string = "PT4294967296S" }, + { .string = "PT99999999999S" }, + { .string = "P99999999999Y99999999999M99999999999D" + "T99999999999H99999999999M99999999999S" }, }; - int num = 22; isc_buffer_t buf1; cfg_parser_t *p1 = NULL; cfg_obj_t *c1 = NULL; + bool must_fail = false; - for (int i = 0; i < num; i++) { + for (size_t i = 0; i < ARRAY_SIZE(durations); i++) { const cfg_listelt_t *element; const cfg_obj_t *kasps = NULL; - char conf[64]; - sprintf(&conf[0], - "dnssec-policy \"dp\"\n{\nsignatures-refresh %s;\n};\n", - durations[i].string); + const char cfg_tpl[] = + "dnssec-policy \"dp\"\n" + "{\nkeys {csk lifetime %s algorithm rsasha256;};\n};\n"; + char conf[sizeof(cfg_tpl) + CFG_DURATION_MAXLEN] = { 0 }; + char out[CFG_DURATION_MAXLEN] = { 0 }; + cfg_printer_t pctx = { .f = output, .closure = out }; + + if (durations[i].string == NULL) { + must_fail = true; + continue; + } + + snprintf(&conf[0], sizeof(conf), cfg_tpl, durations[i].string); isc_buffer_init(&buf1, conf, strlen(conf) - 1); isc_buffer_add(&buf1, strlen(conf) - 1); @@ -136,6 +184,11 @@ ISC_RUN_TEST_IMPL(cfg_obj_asduration) { result = cfg_parse_buffer(p1, &buf1, "text1", 0, &cfg_type_namedconf, 0, &c1); + if (must_fail) { + assert_int_equal(result, DNS_R_BADTTL); + cfg_parser_destroy(&p1); + continue; + } assert_int_equal(result, ISC_R_SUCCESS); (void)cfg_map_get(c1, "dnssec-policy", &kasps); @@ -143,17 +196,37 @@ ISC_RUN_TEST_IMPL(cfg_obj_asduration) { for (element = cfg_list_first(kasps); element != NULL; element = cfg_list_next(element)) { - const cfg_obj_t *d1 = NULL; + const cfg_listelt_t *key_element; + const cfg_obj_t *lifetime = NULL; + const cfg_obj_t *keys = NULL; + const cfg_obj_t *key = NULL; const cfg_obj_t *kopts = NULL; cfg_obj_t *kconf = cfg_listelt_value(element); + int cmp; + assert_non_null(kconf); kopts = cfg_tuple_get(kconf, "options"); - result = cfg_map_get(kopts, "signatures-refresh", &d1); - assert_int_equal(result, ISC_R_SUCCESS); + result = cfg_map_get(kopts, "keys", &keys); + + key_element = cfg_list_first(keys); + assert_non_null(key_element); + + key = cfg_listelt_value(key_element); + assert_non_null(key); + + lifetime = cfg_tuple_get(key, "lifetime"); + assert_non_null(lifetime); assert_int_equal(durations[i].time, - cfg_obj_asduration(d1)); + cfg_obj_asduration(lifetime)); + + cfg_print_duration_or_unlimited(&pctx, lifetime); + cmp = strncasecmp(durations[i].out != NULL + ? durations[i].out + : durations[i].string, + out, strlen(durations[i].string)); + assert_int_equal(cmp, 0); } cfg_obj_destroy(p1, &c1); @@ -163,7 +236,7 @@ ISC_RUN_TEST_IMPL(cfg_obj_asduration) { ISC_TEST_LIST_START -ISC_TEST_ENTRY(cfg_obj_asduration) +ISC_TEST_ENTRY(duration) ISC_TEST_LIST_END From dc55f1ebb91e01979883678e620aa999443522c1 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Mon, 17 Oct 2022 08:45:26 +0000 Subject: [PATCH 3/5] Fix an off-by-one error in cfg_print_duration() The cfg_print_duration() checks added previously in the 'duration_test' unit test uncovered a bug in cfg_print_duration(). When calculating the current 'str' pointer of the generated text in the buffer 'buf', it erroneously adds 1 byte to compensate for that part's indicator character. For example, to add 12 minutes, it needs to add 2 + 1 = 3 characters, where 2 is the length of "12", and 1 is the length of "M" (for minute). The mistake was that the length of the indicator is already included in 'durationlen[i]', so there is no need to calculate it again. In the result of this mistake the current pointer can advance further than needed and end up after the zero-byte instead of right on it, which essentially cuts off any further generated text. For example, for a 5 minutes and 30 seconds duration, instead of having this: 'P', 'T', '5', 'M', '3', '0', 'S', '\0' The function generates this: 'P', 'T', '5', 'M', '\0', '3', '0', 'S', '\0' Fix the bug by adding to 'str' just 'durationlen[i]' instead of 'durationlen[i] + 1'. --- lib/isccfg/parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/isccfg/parser.c b/lib/isccfg/parser.c index d27e6e599a..4ce73dd0b9 100644 --- a/lib/isccfg/parser.c +++ b/lib/isccfg/parser.c @@ -1096,7 +1096,7 @@ cfg_print_duration(cfg_printer_t *pctx, const cfg_obj_t *obj) { if (duration.parts[i] > 0) { snprintf(str, durationlen[i] + 2, "%u%c", (uint32_t)duration.parts[i], indicators[i]); - str += durationlen[i] + 1; + str += durationlen[i]; } if (i == 3 && T) { snprintf(str, 2, "T"); From 190aab84d7b90e6ebd860494efbd160b76dac6de Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Mon, 17 Oct 2022 08:45:34 +0000 Subject: [PATCH 4/5] Add a CHANGES note for [GL !6880] --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index 009e0ce6c8..7b80719474 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +5996. [bug] Fix a couple of bugs in cfg_print_duration(), which + could result in generating incomplete duration values + when printing the configuration using named-checkconf. + [GL !6880] + 5995. [performance] A new algorithm for DNS name compression based on a hash set of message offsets. Name compression is now more complete as well as being generally faster, and From fddaebb285df4a7d00ff12f8a1bd9beef21a2a9d Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Mon, 17 Oct 2022 08:45:45 +0000 Subject: [PATCH 5/5] Handle large numbers when parsing/printing a duration The isccfg_duration_fromtext() function is truncating large numbers to 32 bits instead of capping or rejecting them, i.e. 64424509445, which is 0xf00000005, gets parsed as 32-bit value 5 (0x00000005). Fail parsing a duration if any of its components is bigger than 32 bits. Using those kind of big numbers has no practical use case for a duration. The isccfg_duration_toseconds() function can overflow the 32 bit seconds variable when calculating the duration from its component parts. To avoid that, use 64-bit calculation and return UINT32_MAX if the calculated value is bigger than UINT32_MAX. Again, a number this big has no practical use case anyway. The buffer for the generated duration string is limited to 64 bytes, which, in theory, is smaller than the longest possible generated duration string. Use 80 bytes instead, calculated by the '7 x (10 + 1) + 3' formula, where '7' is the count of the duration's parts (year, month, etc.), '10' is their maximum length when printed as a decimal number, '1' is their indicator character (Y, M, etc.), and 3 is two more indicators (P and T) and the terminating NUL character. --- lib/isccfg/duration.c | 75 +++++++++++++++++++++------- lib/isccfg/include/isccfg/duration.h | 6 ++- lib/isccfg/parser.c | 4 +- 3 files changed, 63 insertions(+), 22 deletions(-) diff --git a/lib/isccfg/duration.c b/lib/isccfg/duration.c index 305047e1af..9ed9d6f657 100644 --- a/lib/isccfg/duration.c +++ b/lib/isccfg/duration.c @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -39,14 +40,14 @@ isc_result_t isccfg_duration_fromtext(isc_textregion_t *source, isccfg_duration_t *duration) { - char buf[DURATION_MAXLEN]; + char buf[CFG_DURATION_MAXLEN] = { 0 }; char *P, *X, *T, *W, *str; bool not_weeks = false; int i; + long long int lli; /* * Copy the buffer as it may not be NULL terminated. - * Anyone having a duration longer than 63 characters is crazy. */ if (source->length > sizeof(buf) - 1) { return (ISC_R_BADNUMBER); @@ -74,7 +75,12 @@ isccfg_duration_fromtext(isc_textregion_t *source, /* Record years. */ X = strpbrk(str, "Yy"); if (X != NULL) { - duration->parts[0] = atoi(str + 1); + errno = 0; + lli = strtoll(str + 1, NULL, 10); + if (errno != 0 || lli < 0 || lli > UINT32_MAX) { + return (ISC_R_BADNUMBER); + } + duration->parts[0] = (uint32_t)lli; str = X; not_weeks = true; } @@ -87,7 +93,12 @@ isccfg_duration_fromtext(isc_textregion_t *source, * part, or this M indicator is before the time indicator. */ if (X != NULL && (T == NULL || (size_t)(X - P) < (size_t)(T - P))) { - duration->parts[1] = atoi(str + 1); + errno = 0; + lli = strtoll(str + 1, NULL, 10); + if (errno != 0 || lli < 0 || lli > UINT32_MAX) { + return (ISC_R_BADNUMBER); + } + duration->parts[1] = (uint32_t)lli; str = X; not_weeks = true; } @@ -95,7 +106,12 @@ isccfg_duration_fromtext(isc_textregion_t *source, /* Record days. */ X = strpbrk(str, "Dd"); if (X != NULL) { - duration->parts[3] = atoi(str + 1); + errno = 0; + lli = strtoll(str + 1, NULL, 10); + if (errno != 0 || lli < 0 || lli > UINT32_MAX) { + return (ISC_R_BADNUMBER); + } + duration->parts[3] = (uint32_t)lli; str = X; not_weeks = true; } @@ -109,7 +125,12 @@ isccfg_duration_fromtext(isc_textregion_t *source, /* Record hours. */ X = strpbrk(str, "Hh"); if (X != NULL && T != NULL) { - duration->parts[4] = atoi(str + 1); + errno = 0; + lli = strtoll(str + 1, NULL, 10); + if (errno != 0 || lli < 0 || lli > UINT32_MAX) { + return (ISC_R_BADNUMBER); + } + duration->parts[4] = (uint32_t)lli; str = X; not_weeks = true; } @@ -122,7 +143,12 @@ isccfg_duration_fromtext(isc_textregion_t *source, * part and the M indicator is behind the time indicator. */ if (X != NULL && T != NULL && (size_t)(X - P) > (size_t)(T - P)) { - duration->parts[5] = atoi(str + 1); + errno = 0; + lli = strtoll(str + 1, NULL, 10); + if (errno != 0 || lli < 0 || lli > UINT32_MAX) { + return (ISC_R_BADNUMBER); + } + duration->parts[5] = (uint32_t)lli; str = X; not_weeks = true; } @@ -130,7 +156,12 @@ isccfg_duration_fromtext(isc_textregion_t *source, /* Record seconds. */ X = strpbrk(str, "Ss"); if (X != NULL && T != NULL) { - duration->parts[6] = atoi(str + 1); + errno = 0; + lli = strtoll(str + 1, NULL, 10); + if (errno != 0 || lli < 0 || lli > UINT32_MAX) { + return (ISC_R_BADNUMBER); + } + duration->parts[6] = (uint32_t)lli; str = X; not_weeks = true; } @@ -142,7 +173,12 @@ isccfg_duration_fromtext(isc_textregion_t *source, /* Mix of weeks and other indicators is not allowed */ return (ISC_R_BADNUMBER); } else { - duration->parts[2] = atoi(str + 1); + errno = 0; + lli = strtoll(str + 1, NULL, 10); + if (errno != 0 || lli < 0 || lli > UINT32_MAX) { + return (ISC_R_BADNUMBER); + } + duration->parts[2] = (uint32_t)lli; str = W; } } @@ -183,20 +219,21 @@ isccfg_parse_duration(isc_textregion_t *source, isccfg_duration_t *duration) { uint32_t isccfg_duration_toseconds(const isccfg_duration_t *duration) { - uint32_t seconds = 0; + uint64_t seconds = 0; REQUIRE(duration != NULL); - seconds += duration->parts[6]; /* Seconds */ - seconds += duration->parts[5] * 60; /* Minutes */ - seconds += duration->parts[4] * 3600; /* Hours */ - seconds += duration->parts[3] * 86400; /* Days */ - seconds += duration->parts[2] * 86400 * 7; /* Weeks */ + seconds += (uint64_t)duration->parts[6]; /* Seconds */ + seconds += (uint64_t)duration->parts[5] * 60; /* Minutes */ + seconds += (uint64_t)duration->parts[4] * 3600; /* Hours */ + seconds += (uint64_t)duration->parts[3] * 86400; /* Days */ + seconds += (uint64_t)duration->parts[2] * 86400 * 7; /* Weeks */ /* * The below additions are not entirely correct - * because days may very per month and per year. + * because days may vary per month and per year. */ - seconds += duration->parts[1] * 86400 * 31; /* Months */ - seconds += duration->parts[0] * 86400 * 365; /* Years */ - return (seconds); + seconds += (uint64_t)duration->parts[1] * 86400 * 31; /* Months */ + seconds += (uint64_t)duration->parts[0] * 86400 * 365; /* Years */ + + return (seconds > UINT32_MAX ? UINT32_MAX : (uint32_t)seconds); } diff --git a/lib/isccfg/include/isccfg/duration.h b/lib/isccfg/include/isccfg/duration.h index b64bea520f..bd0c35bad8 100644 --- a/lib/isccfg/include/isccfg/duration.h +++ b/lib/isccfg/include/isccfg/duration.h @@ -24,7 +24,7 @@ ISC_LANG_BEGINDECLS -#define DURATION_MAXLEN 64 +#define CFG_DURATION_MAXLEN 80 /*% * A configuration object to store ISO 8601 durations. @@ -76,6 +76,10 @@ isccfg_duration_toseconds(const isccfg_duration_t *duration); * - Months will be treated as 31 days. * - Years will be treated as 365 days. * + * Notes: + *\li If the duration in seconds is greater than UINT32_MAX, the return value + * will be UINT32_MAX. + * * Returns: *\li The duration in seconds. */ diff --git a/lib/isccfg/parser.c b/lib/isccfg/parser.c index 4ce73dd0b9..e35b4e7f3b 100644 --- a/lib/isccfg/parser.c +++ b/lib/isccfg/parser.c @@ -1030,7 +1030,7 @@ numlen(uint32_t num) { */ void cfg_print_duration(cfg_printer_t *pctx, const cfg_obj_t *obj) { - char buf[DURATION_MAXLEN]; + char buf[CFG_DURATION_MAXLEN]; char *str; const char *indicators = "YMWDHMS"; int count, i; @@ -1085,7 +1085,7 @@ cfg_print_duration(cfg_printer_t *pctx, const cfg_obj_t *obj) { if (T) { count++; } - INSIST(count < DURATION_MAXLEN); + INSIST(count < CFG_DURATION_MAXLEN); /* Now print the duration. */ for (i = 0; i < 6; i++) {