[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 187220daa26431f15d5962d25f6ee21b6888169d..e2807d0f8f898d205987609a99b68a8521ae211c 100644 (file)
--- 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 <loganaden@gmail.com>
+    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 6abe6e27df8e37010db04872efac0fcf5c290898..0388b17083b7beed1b141ddad8d432658ac787ad 100644 (file)
--- 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 d7b9f1897eb0ebc4e571c2e7cf9558818dd4cf3c..75b1cb14d29dfdfa963bf5625a8a4670f345e12d 100644 (file)
--- 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 b49fdbc327f15ee4a07f56d620f1ceb98f1a86ea..395b550218fcf6720c280332074614a5104c0d0f 100644 (file)
--- 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 (file)
index 0000000..e6af42e
--- /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 <config.h>
+#include <limits.h>
+#include <isc/string.h>
+
+/* 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 914badb5f64b8b35fefe1f76edf5e04088502ac2..f55cbfd5b1635fc07099c1438a1eb1c5fb090159 100644 (file)
--- 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 beaf6fd35b05e3317e3521454e7590d62922fde2..618ccd9de10288fc0da51a696989fa7a419dfa94 100644 (file)
--- 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 fdc33744a18da2b7abdcc7b4459efc517b4deeb6..01f7f500f5138e11e91504466f52a61be5657362 100644 (file)
--- a/ports/winnt/vs2005/libntp.vcproj
+++ b/ports/winnt/vs2005/libntp.vcproj
@@ -532,6 +532,10 @@
                                RelativePath="..\..\..\libntp\timevalops.c"
                                >
                        </File>
+                       <File
+                               RelativePath="..\..\..\lib\isc\tsmemcmp.c"
+                               >
+                       </File>
                        <File
                                RelativePath="..\..\..\libntp\uglydate.c"
                                >

diff --git a/ports/winnt/vs2008/libntp/libntp.vcproj b/ports/winnt/vs2008/libntp/libntp.vcproj
index 191cdfcf76f9d96405b29dd52b5fc8e5ec86cccb..c92258ec4e879f2c69c5bd11aff9a60596a958fb 100644 (file)
--- a/ports/winnt/vs2008/libntp/libntp.vcproj
+++ b/ports/winnt/vs2008/libntp/libntp.vcproj
@@ -647,6 +647,10 @@
                                RelativePath="..\..\..\..\libntp\timevalops.c"
                                >
                        </File>
+                       <File
+                               RelativePath="..\..\..\..\lib\isc\tsmemcmp.c"
+                               >
+                       </File>
                        <File
                                RelativePath="..\..\..\..\libntp\uglydate.c"
                                >

diff --git a/ports/winnt/vs2013/libntp/libntp.vcxproj b/ports/winnt/vs2013/libntp/libntp.vcxproj
index 158e9f1f24703a923800e8139ab5bb7d9594540b..47dcb2ad530ddd96dff2e5986d44689c60464565 100644 (file)
--- a/ports/winnt/vs2013/libntp/libntp.vcxproj
+++ b/ports/winnt/vs2013/libntp/libntp.vcxproj
@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>
 <Project DefaultTargets="Build" ToolsVersion="12.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <ItemGroup Label="ProjectConfigurations">
     <ProjectConfiguration Include="DebugXP|Win32">
@@ -295,6 +295,7 @@
     <ClCompile Include="..\..\..\..\lib\isc\sha1.c" />
     <ClCompile Include="..\..\..\..\lib\isc\sockaddr.c" />
     <ClCompile Include="..\..\..\..\lib\isc\task.c" />
+    <ClCompile Include="..\..\..\..\lib\isc\tsmemcmp.c" />
     <ClCompile Include="..\..\..\..\lib\isc\win32\condition.c" />
     <ClCompile Include="..\..\..\..\lib\isc\win32\interfaceiter.c" />
     <ClCompile Include="..\..\..\..\lib\isc\win32\net.c" />
@@ -426,4 +427,4 @@
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
   <ImportGroup Label="ExtensionTargets">
   </ImportGroup>
-</Project>
\ No newline at end of file
+</Project>

diff --git a/ports/winnt/vs2013/libntp/libntp.vcxproj.filters b/ports/winnt/vs2013/libntp/libntp.vcxproj.filters
index 6cba2702c64f2b86edd4beac4fdd7549c1322e8d..c2bafdc3e71d50c6c38f250db3850bb0e098a02d 100644 (file)
--- a/ports/winnt/vs2013/libntp/libntp.vcxproj.filters
+++ b/ports/winnt/vs2013/libntp/libntp.vcxproj.filters
@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <ItemGroup>
     <Filter Include="Source Files">
@@ -299,6 +299,9 @@
     <ClCompile Include="..\..\..\..\libntp\timevalops.c">
       <Filter>Source Files</Filter>
     </ClCompile>
+    <ClCompile Include="..\..\..\..\libntp\tsmemcmp.c">
+      <Filter>Source Files</Filter>
+    </ClCompile>
     <ClCompile Include="..\..\..\..\libntp\uglydate.c">
       <Filter>Source Files</Filter>
     </ClCompile>
@@ -565,4 +568,4 @@
       <Filter>Resource Files</Filter>
     </CustomBuild>
   </ItemGroup>
-</Project>
\ No newline at end of file
+</Project>

diff --git a/sntp/crypto.c b/sntp/crypto.c
index b178f8c2e15718dea8b19e6c994db0d6be1c9179..a2676a202ca2200b79116ca602c883af37722ce9 100644 (file)
--- a/sntp/crypto.c
+++ b/sntp/crypto.c
@@ -1,6 +1,7 @@
 #include <config.h>
 #include "crypto.h"
 #include <ctype.h>
+#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 7bfe9eea8859cc12f46225fb7f7f6de95c183cca..f8360a9e27e4ba5bb4291bf5a565301231f48e71 100644 (file)
--- 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 (file)
index 0000000..1bcfc9c
--- /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 <setjmp.h>
+#include <stdio.h>
+#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 (file)
index 0000000..7bd9158
--- /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])));
+}
+