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 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 4ea849e64f..e35b4e7f3b 100644 --- a/lib/isccfg/parser.c +++ b/lib/isccfg/parser.c @@ -1030,11 +1030,11 @@ 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; - 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 @@ -1087,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++) { @@ -1098,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"); @@ -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]); } 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