From: Juergen Perlinger Date: Sat, 15 Apr 2023 08:54:14 +0000 (+0200) Subject: [Bug 3806] libntp/mstolfp.c needs bounds checking X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=55daad8ec30647babd21bb6c86b6bf744a71d771;p=thirdparty%2Fntp.git [Bug 3806] libntp/mstolfp.c needs bounds checking bk: 643a66366g1aqNfzQ8ESMtTToiQCGw --- diff --git a/ChangeLog b/ChangeLog index d6af39782..0c942b5de 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,5 @@ +* [Bug 3806] libntp/mstolfp.c needs bounds checking + - solved numerically instead of using string manipulation --- * [Bug 3802] ntp-keygen -I default identity modulus bits too small for OpenSSL 3. Reported by rmsh1216@163.com diff --git a/libntp/mstolfp.c b/libntp/mstolfp.c index 3dfc4efd4..daf24a768 100644 --- a/libntp/mstolfp.c +++ b/libntp/mstolfp.c @@ -14,86 +14,63 @@ mstolfp( l_fp *lfp ) { - register const char *cp; - register char *bp; - register const char *cpdec; - char buf[100]; + int ch, neg = 0; + u_int32 q, r; /* * We understand numbers of the form: * * [spaces][-|+][digits][.][digits][spaces|\n|\0] * - * This is one enormous hack. Since I didn't feel like - * rewriting the decoding routine for milliseconds, what - * is essentially done here is to make a copy of the string - * with the decimal moved over three places so the seconds - * decoding routine can be used. + * This is kinda hack. We use 'atolfp' to do the basic parsing + * (after some initial checks) and then divide the result by + * 1000. The original implementation avoided that by + * hacking up the input string to move the decimal point, but + * that needed string manipulations prone to buffer overruns. + * To avoid that trouble we do the conversion first and adjust + * the result. */ - bp = buf; - cp = str; - while (isspace((unsigned char)*cp)) - cp++; - if (*cp == '-' || *cp == '+') { - *bp++ = *cp++; - } - - if (*cp != '.' && !isdigit((unsigned char)*cp)) - return 0; - - - /* - * Search forward for the decimal point or the end of the string. - */ - cpdec = cp; - while (isdigit((unsigned char)*cpdec)) - cpdec++; - - /* - * Found something. If we have more than three digits copy the - * excess over, else insert a leading 0. - */ - if ((cpdec - cp) > 3) { - do { - *bp++ = (char)*cp++; - } while ((cpdec - cp) > 3); - } else { - *bp++ = '0'; + while (isspace(ch = *(const unsigned char*)str)) + ++str; + + switch (ch) { + case '-': neg = TRUE; + case '+': ++str; + default : break; } + + if (!isdigit(ch = *(const unsigned char*)str) && (ch != '.')) + return 0; + if (!atolfp(str, lfp)) + return 0; - /* - * Stick the decimal in. If we've got less than three digits in - * front of the millisecond decimal we insert the appropriate number - * of zeros. + /* now do a chained/overlapping division by 1000 to get from + * seconds to msec. 1000 is small enough to go with temporary + * 32bit accus for Q and R. */ - *bp++ = '.'; - if ((cpdec - cp) < 3) { - size_t i = 3 - (cpdec - cp); - do { - *bp++ = '0'; - } while (--i > 0); - } + q = lfp->l_ui / 1000u; + r = lfp->l_ui - (q * 1000u); + lfp->l_ui = q; - /* - * Copy the remainder up to the millisecond decimal. If cpdec - * is pointing at a decimal point, copy in the trailing number too. - */ - while (cp < cpdec) - *bp++ = (char)*cp++; - - if (*cp == '.') { - cp++; - while (isdigit((unsigned char)*cp)) - *bp++ = (char)*cp++; - } - *bp = '\0'; + r = (r << 16) | (lfp->l_uf >> 16); + q = r / 1000u; + r = ((r - q * 1000) << 16) | (lfp->l_uf & 0x0FFFFu); + lfp->l_uf = q << 16; + q = r / 1000; + lfp->l_uf |= q; + r -= q * 1000u; - /* - * Check to make sure the string is properly terminated. If - * so, give the buffer to the decoding routine. + /* numerically, rounding should happen *after* the sign correction + * but that would produce different bit patterns from the previous + * implementation. (off-by-one in the fraction. Neglectible, but + * messy to fix in the unit tests. So we stay with the slightly + * suboptimal calculation...) */ - if (*cp != '\0' && !isspace((unsigned char)*cp)) - return 0; - return atolfp(buf, lfp); + if (r >= 500) + L_ADDUF(lfp, 1u); + /* fix sign */ + if (neg) + L_NEG(lfp); + return 1; } diff --git a/tests/libntp/strtolfp.c b/tests/libntp/strtolfp.c index 6855d9ba3..9090159a6 100644 --- a/tests/libntp/strtolfp.c +++ b/tests/libntp/strtolfp.c @@ -26,6 +26,13 @@ setUp(void) return; } +static const char* fmtLFP(const l_fp *e, const l_fp *a) +{ + static char buf[100]; + snprintf(buf, sizeof(buf), "e=$%08x.%08x, a=$%08x.%08x", + e->l_ui, e->l_uf, a->l_ui, a->l_uf); + return buf; +} void test_PositiveInteger(void) { const char *str = "500"; @@ -37,8 +44,8 @@ void test_PositiveInteger(void) { TEST_ASSERT_TRUE(atolfp(str, &actual)); TEST_ASSERT_TRUE(mstolfp(str_ms, &actual_ms)); - TEST_ASSERT_TRUE(IsEqual(expected, actual)); - TEST_ASSERT_TRUE(IsEqual(expected, actual_ms)); + TEST_ASSERT_TRUE_MESSAGE(IsEqual(expected, actual), fmtLFP(&expected, &actual)); + TEST_ASSERT_TRUE_MESSAGE(IsEqual(expected, actual_ms), fmtLFP(&expected, &actual_ms)); } void test_NegativeInteger(void) { @@ -54,8 +61,8 @@ void test_NegativeInteger(void) { TEST_ASSERT_TRUE(atolfp(str, &actual)); TEST_ASSERT_TRUE(mstolfp(str_ms, &actual_ms)); - TEST_ASSERT_TRUE(IsEqual(expected, actual)); - TEST_ASSERT_TRUE(IsEqual(expected, actual_ms)); + TEST_ASSERT_TRUE_MESSAGE(IsEqual(expected, actual), fmtLFP(&expected, &actual)); + TEST_ASSERT_TRUE_MESSAGE(IsEqual(expected, actual_ms), fmtLFP(&expected, &actual_ms)); } void test_PositiveFraction(void) { @@ -68,8 +75,8 @@ void test_PositiveFraction(void) { TEST_ASSERT_TRUE(atolfp(str, &actual)); TEST_ASSERT_TRUE(mstolfp(str_ms, &actual_ms)); - TEST_ASSERT_TRUE(IsEqual(expected, actual)); - TEST_ASSERT_TRUE(IsEqual(expected, actual_ms)); + TEST_ASSERT_TRUE_MESSAGE(IsEqual(expected, actual), fmtLFP(&expected, &actual)); + TEST_ASSERT_TRUE_MESSAGE(IsEqual(expected, actual_ms), fmtLFP(&expected, &actual_ms)); } void test_NegativeFraction(void) { @@ -85,8 +92,8 @@ void test_NegativeFraction(void) { TEST_ASSERT_TRUE(atolfp(str, &actual)); TEST_ASSERT_TRUE(mstolfp(str_ms, &actual_ms)); - TEST_ASSERT_TRUE(IsEqual(expected, actual)); - TEST_ASSERT_TRUE(IsEqual(expected, actual_ms)); + TEST_ASSERT_TRUE_MESSAGE(IsEqual(expected, actual), fmtLFP(&expected, &actual)); + TEST_ASSERT_TRUE_MESSAGE(IsEqual(expected, actual_ms), fmtLFP(&expected, &actual_ms)); } void test_PositiveMsFraction(void) { @@ -100,9 +107,8 @@ void test_PositiveMsFraction(void) { TEST_ASSERT_TRUE(atolfp(str, &actual)); TEST_ASSERT_TRUE(mstolfp(str_ms, &actual_ms)); - TEST_ASSERT_TRUE(IsEqual(expected, actual)); - TEST_ASSERT_TRUE(IsEqual(expected, actual_ms)); - + TEST_ASSERT_TRUE_MESSAGE(IsEqual(expected, actual), fmtLFP(&expected, &actual)); + TEST_ASSERT_TRUE_MESSAGE(IsEqual(expected, actual_ms), fmtLFP(&expected, &actual_ms)); } void test_NegativeMsFraction(void) { @@ -118,9 +124,8 @@ void test_NegativeMsFraction(void) { TEST_ASSERT_TRUE(atolfp(str, &actual)); TEST_ASSERT_TRUE(mstolfp(str_ms, &actual_ms)); - TEST_ASSERT_TRUE(IsEqual(expected, actual)); - TEST_ASSERT_TRUE(IsEqual(expected, actual_ms)); - + TEST_ASSERT_TRUE_MESSAGE(IsEqual(expected, actual), fmtLFP(&expected, &actual)); + TEST_ASSERT_TRUE_MESSAGE(IsEqual(expected, actual_ms), fmtLFP(&expected, &actual_ms)); } void test_InvalidChars(void) {