From: Juergen Perlinger Date: Sat, 12 Dec 2015 10:24:19 +0000 (+0000) Subject: [Bug 2879] Improve NTP security against timing attacks. X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=413f1dd62ae8aead737cf3e15fcc2d6ac6aa8bba;p=thirdparty%2Fntp.git [Bug 2879] Improve NTP security against timing attacks. - use timing-safe memcmp() for digest tests bk: 566bf5d3dZ5HASyXGnVlxig-qUuvqw --- diff --git a/ChangeLog b/ChangeLog index 187220daa..e2807d0f8 100644 --- a/ChangeLog +++ b/ChangeLog @@ -18,6 +18,9 @@ * CID 1341682: Nit in libntp/authreadkeys.c. HStenn. * CID 1341684: Nit in tests/ntpd/t-ntp_signd.c. HStenn. * [Bug 2829] Look at pipe_fds in ntpd.c (did so. perlinger@ntp.org) +* [Bug 2879] Improve NTP security against timing attacks. perlinger@ntp.org + - integrated patches by Loganaden Velvidron + with some modifications & unit tests * [Bug 2887] stratum -1 config results as showing value 99 - fudge stratum only accepts values [0..16]. perlinger@ntp.org * [Bug 2932] Update leapsecond file info in miscopt.html. CWoodbury, HStenn. diff --git a/lib/isc/hmacmd5.c b/lib/isc/hmacmd5.c index 6abe6e27d..0388b1708 100644 --- a/lib/isc/hmacmd5.c +++ b/lib/isc/hmacmd5.c @@ -145,5 +145,5 @@ isc_hmacmd5_verify2(isc_hmacmd5_t *ctx, unsigned char *digest, size_t len) { REQUIRE(len <= ISC_MD5_DIGESTLENGTH); isc_hmacmd5_sign(ctx, newdigest); - return (ISC_TF(memcmp(digest, newdigest, len) == 0)); + return (ISC_TF(isc_tsmemcmp(digest, newdigest, len) == 0)); } diff --git a/lib/isc/hmacsha.c b/lib/isc/hmacsha.c index d7b9f1897..75b1cb14d 100644 --- a/lib/isc/hmacsha.c +++ b/lib/isc/hmacsha.c @@ -538,7 +538,7 @@ isc_hmacsha1_verify(isc_hmacsha1_t *ctx, unsigned char *digest, size_t len) { REQUIRE(len <= ISC_SHA1_DIGESTLENGTH); isc_hmacsha1_sign(ctx, newdigest, ISC_SHA1_DIGESTLENGTH); - return (ISC_TF(memcmp(digest, newdigest, len) == 0)); + return (ISC_TF(isc_tsmemcmp(digest, newdigest, len) == 0)); } /* @@ -551,7 +551,7 @@ isc_hmacsha224_verify(isc_hmacsha224_t *ctx, unsigned char *digest, size_t len) REQUIRE(len <= ISC_SHA224_DIGESTLENGTH); isc_hmacsha224_sign(ctx, newdigest, ISC_SHA224_DIGESTLENGTH); - return (ISC_TF(memcmp(digest, newdigest, len) == 0)); + return (ISC_TF(isc_tsmemcmp(digest, newdigest, len) == 0)); } /* @@ -564,7 +564,7 @@ isc_hmacsha256_verify(isc_hmacsha256_t *ctx, unsigned char *digest, size_t len) REQUIRE(len <= ISC_SHA256_DIGESTLENGTH); isc_hmacsha256_sign(ctx, newdigest, ISC_SHA256_DIGESTLENGTH); - return (ISC_TF(memcmp(digest, newdigest, len) == 0)); + return (ISC_TF(isc_tsmemcmp(digest, newdigest, len) == 0)); } /* @@ -577,7 +577,7 @@ isc_hmacsha384_verify(isc_hmacsha384_t *ctx, unsigned char *digest, size_t len) REQUIRE(len <= ISC_SHA384_DIGESTLENGTH); isc_hmacsha384_sign(ctx, newdigest, ISC_SHA384_DIGESTLENGTH); - return (ISC_TF(memcmp(digest, newdigest, len) == 0)); + return (ISC_TF(isc_tsmemcmp(digest, newdigest, len) == 0)); } /* @@ -590,5 +590,5 @@ isc_hmacsha512_verify(isc_hmacsha512_t *ctx, unsigned char *digest, size_t len) REQUIRE(len <= ISC_SHA512_DIGESTLENGTH); isc_hmacsha512_sign(ctx, newdigest, ISC_SHA512_DIGESTLENGTH); - return (ISC_TF(memcmp(digest, newdigest, len) == 0)); + return (ISC_TF(isc_tsmemcmp(digest, newdigest, len) == 0)); } diff --git a/lib/isc/include/isc/string.h b/lib/isc/include/isc/string.h index b49fdbc32..395b55021 100644 --- a/lib/isc/include/isc/string.h +++ b/lib/isc/include/isc/string.h @@ -199,6 +199,24 @@ isc_string_regiondup(isc_mem_t *mctx, const isc_region_t *source); * */ +int +isc_tsmemcmp(const void *p1, const void *p2, size_t len); +/* + * Lexicographic compare 'len' unsigned bytes from 'p1' and 'p2' + * like 'memcmp()'. + * + * This function is safe from timing attacks as it has a runtime that + * only depends on 'len' and has no early-out option. + * + * Use this to check MACs and other material that is security sensitive. + * + * Returns: + * (let x be the byte offset of the first different byte) + * -1 if (u_char)p1[x] < (u_char)p2[x] + * 1 if (u_char)p1[x] > (u_char)p2[x] + * 0 if byte series are equal + */ + char * isc_string_separate(char **stringp, const char *delim); diff --git a/lib/isc/tsmemcmp.c b/lib/isc/tsmemcmp.c new file mode 100644 index 000000000..e6af42eaf --- /dev/null +++ b/lib/isc/tsmemcmp.c @@ -0,0 +1,55 @@ +/* + * Copyright (C) 2004-2007, 2011, 2012 Internet Systems Consortium, Inc. ("ISC") + * Copyright (C) 1999-2001, 2003 Internet Software Consortium. + * + * Permission to use, copy, modify, and/or distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH + * REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY + * AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, + * INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM + * LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE + * OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR + * PERFORMANCE OF THIS SOFTWARE. + */ + +/* $Id$ */ + +/*! \file */ + +#include +#include +#include + +/* Making a portable memcmp that has no internal branches and loops always + * once for every byte without early-out shortcut has a few challenges. + * + * Inspired by 'timingsafe_memcmp()' from the BSD system and + * https://github.com/libressl-portable/openbsd/blob/master/src/lib/libc/string/timingsafe_memcmp.c + * + * Sadly, that one is not portable C: It makes assumptions on the representation + * of negative integers and assumes sign-preserving right-shift of negative + * signed values. This is a rewrite from scratch that should not suffer from + * such issues. + * + * 2015-12-12, J. Perlinger (perlinger-at-ntp-dot-org) + */ +int +isc_tsmemcmp(const void *p1, const void *p2, size_t nb) { + const unsigned char *ucp1 = p1; + const unsigned char *ucp2 = p2; + unsigned int isLT = 0u; + unsigned int isGT = 0u; + volatile unsigned int mask = (1u << CHAR_BIT); + + for (/*NOP*/; 0 != nb; --nb, ++ucp1, ++ucp2) { + isLT |= mask & + ((unsigned int)*ucp1 - (unsigned int)*ucp2); + isGT |= mask & + ((unsigned int)*ucp2 - (unsigned int)*ucp1); + mask &= ~(isLT | isGT); + } + return (int)(isGT >> CHAR_BIT) - (int)(isLT >> CHAR_BIT); +} diff --git a/libntp/Makefile.am b/libntp/Makefile.am index 914badb5f..f55cbfd5b 100644 --- a/libntp/Makefile.am +++ b/libntp/Makefile.am @@ -36,6 +36,7 @@ libisc_SRCS = \ $(srcdir)/../lib/isc/unix/time.c \ $(srcdir)/../lib/isc/sha1.c \ $(srcdir)/../lib/isc/sockaddr.c \ + $(srcdir)/../lib/isc/tsmemcmp.c \ $(NULL) if PTHREADS diff --git a/libntp/a_md5encrypt.c b/libntp/a_md5encrypt.c index beaf6fd35..618ccd9de 100644 --- a/libntp/a_md5encrypt.c +++ b/libntp/a_md5encrypt.c @@ -10,7 +10,7 @@ #include "ntp_stdlib.h" #include "ntp.h" #include "ntp_md5.h" /* provides OpenSSL digest API */ - +#include "isc/string.h" /* * MD5authencrypt - generate message digest * @@ -92,7 +92,7 @@ MD5authdecrypt( "MAC decrypt: MAC length error"); return (0); } - return !memcmp(digest, (const char *)pkt + length + 4, len); + return !isc_tsmemcmp(digest, (const char *)pkt + length + 4, len); } /* diff --git a/ports/winnt/vs2005/libntp.vcproj b/ports/winnt/vs2005/libntp.vcproj index fdc33744a..01f7f500f 100644 --- a/ports/winnt/vs2005/libntp.vcproj +++ b/ports/winnt/vs2005/libntp.vcproj @@ -532,6 +532,10 @@ RelativePath="..\..\..\libntp\timevalops.c" > + + diff --git a/ports/winnt/vs2008/libntp/libntp.vcproj b/ports/winnt/vs2008/libntp/libntp.vcproj index 191cdfcf7..c92258ec4 100644 --- a/ports/winnt/vs2008/libntp/libntp.vcproj +++ b/ports/winnt/vs2008/libntp/libntp.vcproj @@ -647,6 +647,10 @@ RelativePath="..\..\..\..\libntp\timevalops.c" > + + diff --git a/ports/winnt/vs2013/libntp/libntp.vcxproj b/ports/winnt/vs2013/libntp/libntp.vcxproj index 158e9f1f2..47dcb2ad5 100644 --- a/ports/winnt/vs2013/libntp/libntp.vcxproj +++ b/ports/winnt/vs2013/libntp/libntp.vcxproj @@ -1,4 +1,4 @@ - + @@ -295,6 +295,7 @@ + @@ -426,4 +427,4 @@ - \ No newline at end of file + diff --git a/ports/winnt/vs2013/libntp/libntp.vcxproj.filters b/ports/winnt/vs2013/libntp/libntp.vcxproj.filters index 6cba2702c..c2bafdc3e 100644 --- a/ports/winnt/vs2013/libntp/libntp.vcxproj.filters +++ b/ports/winnt/vs2013/libntp/libntp.vcxproj.filters @@ -1,4 +1,4 @@ - + @@ -299,6 +299,9 @@ Source Files + + Source Files + Source Files @@ -565,4 +568,4 @@ Resource Files - \ No newline at end of file + diff --git a/sntp/crypto.c b/sntp/crypto.c index b178f8c2e..a2676a202 100644 --- a/sntp/crypto.c +++ b/sntp/crypto.c @@ -1,6 +1,7 @@ #include #include "crypto.h" #include +#include "isc/string.h" struct key *key_ptr; size_t key_cnt = 0; @@ -58,7 +59,7 @@ auth_md5( if (!hash_len) authentic = FALSE; else - authentic = !memcmp(digest, pkt_data + pkt_size + 4, + authentic = !isc_tsmemcmp(digest, pkt_data + pkt_size + 4, hash_len); return authentic; } diff --git a/tests/libntp/Makefile.am b/tests/libntp/Makefile.am index 7bfe9eea8..f8360a9e2 100644 --- a/tests/libntp/Makefile.am +++ b/tests/libntp/Makefile.am @@ -42,6 +42,7 @@ check_PROGRAMS = \ test-strtolfp \ test-timespecops \ test-timevalops \ + test-tsafememcmp \ test-tstotv \ test-tvtots \ test-uglydate \ @@ -102,6 +103,7 @@ BUILT_SOURCES += \ $(srcdir)/run-strtolfp.c \ $(srcdir)/run-timevalops.c \ $(srcdir)/run-timespecops.c \ + $(srcdir)/run-tsafememcmp.c \ $(srcdir)/run-tstotv.c \ $(srcdir)/run-tvtots.c \ $(srcdir)/run-uglydate.c \ @@ -477,6 +479,16 @@ $(srcdir)/run-timevalops.c: $(srcdir)/timevalops.c $(std_unity_list) ### +test_tsafememcmp_SOURCES = \ + tsafememcmp.c \ + run-tsafememcmp.c \ + $(NULL) + +$(srcdir)/run-tsafememcmp.c: $(srcdir)/tsafememcmp.c $(std_unity_list) + $(run_unity) tsafememcmp.c run-tsafememcmp.c + +### + test_tstotv_SOURCES = \ tstotv.c \ run-tstotv.c \ diff --git a/tests/libntp/run-tsafememcmp.c b/tests/libntp/run-tsafememcmp.c new file mode 100644 index 000000000..1bcfc9cda --- /dev/null +++ b/tests/libntp/run-tsafememcmp.c @@ -0,0 +1,64 @@ +/* AUTOGENERATED FILE. DO NOT EDIT. */ + +//=======Test Runner Used To Run Each Test Below===== +#define RUN_TEST(TestFunc, TestLineNum) \ +{ \ + Unity.CurrentTestName = #TestFunc; \ + Unity.CurrentTestLineNumber = TestLineNum; \ + Unity.NumberOfTests++; \ + if (TEST_PROTECT()) \ + { \ + setUp(); \ + TestFunc(); \ + } \ + if (TEST_PROTECT() && !TEST_IS_IGNORED) \ + { \ + tearDown(); \ + } \ + UnityConcludeTest(); \ +} + +//=======Automagically Detected Files To Include===== +#include "unity.h" +#include +#include +#include "config.h" +#include "ntp_stdlib.h" +#include "isc/string.h" + +//=======External Functions This Runner Calls===== +extern void setUp(void); +extern void tearDown(void); +extern void test_Empty(void); +extern void test_Equal(void); +extern void test_FirstByte(void); +extern void test_LastByte(void); +extern void test_MiddleByte(void); +extern void test_MiddleByteUpLo(void); + + +//=======Test Reset Option===== +void resetTest(void); +void resetTest(void) +{ + tearDown(); + setUp(); +} + +char const *progname; + + +//=======MAIN===== +int main(int argc, char *argv[]) +{ + progname = argv[0]; + UnityBegin("tsafememcmp.c"); + RUN_TEST(test_Empty, 10); + RUN_TEST(test_Equal, 11); + RUN_TEST(test_FirstByte, 12); + RUN_TEST(test_LastByte, 13); + RUN_TEST(test_MiddleByte, 14); + RUN_TEST(test_MiddleByteUpLo, 15); + + return (UnityEnd()); +} diff --git a/tests/libntp/tsafememcmp.c b/tests/libntp/tsafememcmp.c new file mode 100644 index 000000000..7bd91584e --- /dev/null +++ b/tests/libntp/tsafememcmp.c @@ -0,0 +1,85 @@ +#include "config.h" + +#include "ntp_stdlib.h" +#include "isc/string.h" + +#include "unity.h" + +/* Basisc test for timingsafe_memcmp() */ + +void test_Empty(void); +void test_Equal(void); +void test_FirstByte(void); +void test_LastByte(void); +void test_MiddleByte(void); +void test_MiddleByteUpLo(void); + +void test_Empty(void) +{ + static const char dummy[1]; + TEST_ASSERT_EQUAL_INT(0, isc_tsmemcmp(NULL , NULL , 0)); + TEST_ASSERT_EQUAL_INT(0, isc_tsmemcmp(dummy, dummy, 0)); +} + +void test_Equal(void) +{ + static const char dummy[2][4] = { + "blob", "blob" + }; + TEST_ASSERT_EQUAL_INT(0, isc_tsmemcmp(dummy[0], + dummy[1], + sizeof(dummy[0]))); +} + +void test_FirstByte(void) +{ + static const char dummy[2][4] = { + "Blob", "Clob" + }; + TEST_ASSERT_EQUAL_INT(-1, isc_tsmemcmp(dummy[0], + dummy[1], + sizeof(dummy[0]))); + TEST_ASSERT_EQUAL_INT( 1, isc_tsmemcmp(dummy[1], + dummy[0], + sizeof(dummy[0]))); +} + +void test_LastByte(void) +{ + static const char dummy[2][4] = { + "Blob", "Bloc" + }; + TEST_ASSERT_EQUAL_INT(-1, isc_tsmemcmp(dummy[0], + dummy[1], + sizeof(dummy[0]))); + TEST_ASSERT_EQUAL_INT( 1, isc_tsmemcmp(dummy[1], + dummy[0], + sizeof(dummy[0]))); +} + +void test_MiddleByte(void) +{ + static const char dummy[2][4] = { + "Blob", "Blpb" + }; + TEST_ASSERT_EQUAL_INT(-1, isc_tsmemcmp(dummy[0], + dummy[1], + sizeof(dummy[0]))); + TEST_ASSERT_EQUAL_INT( 1, isc_tsmemcmp(dummy[1], + dummy[0], + sizeof(dummy[0]))); +} + +void test_MiddleByteUpLo(void) +{ + static const char dummy[2][4] = { + "Blob", "Blpa" + }; + TEST_ASSERT_EQUAL_INT(-1, isc_tsmemcmp(dummy[0], + dummy[1], + sizeof(dummy[0]))); + TEST_ASSERT_EQUAL_INT( 1, isc_tsmemcmp(dummy[1], + dummy[0], + sizeof(dummy[0]))); +} +