Blob Blame History Raw
From f27598743ab6e03271e26f23da4beba748d19c60 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= <ondrej@sury.org>
Date: Wed, 25 Apr 2018 14:04:31 +0200
Subject: [PATCH] Replace isc_safe routines with their OpenSSL counter parts

(cherry picked from commit 66ba2fdad583d962a1f4971c85d58381f0849e4d)

Remove isc_safe_memcompare, it's not needed anywhere and can't be replaced with CRYPTO_memcmp()

(cherry picked from commit b105ccee68ccc3c18e6ea530063b3c8e5a42571c)

Fix the isc_safe_memwipe() usage with (NULL, >0)

(cherry picked from commit 083461d3329ff6f2410745848a926090586a9846)
---
 bin/dnssec/dnssec-signzone.c |  2 +-
 lib/dns/nsec3.c              |  4 +-
 lib/dns/spnego.c             |  4 +-
 lib/isc/Makefile.in          |  8 +---
 lib/isc/include/isc/safe.h   | 18 ++------
 lib/isc/safe.c               | 83 ------------------------------------
 lib/isc/tests/safe_test.c    | 18 --------
 7 files changed, 11 insertions(+), 126 deletions(-)
 delete mode 100644 lib/isc/safe.c

diff --git a/bin/dnssec/dnssec-signzone.c b/bin/dnssec/dnssec-signzone.c
index 6dded0c..a9c5557 100644
--- a/bin/dnssec/dnssec-signzone.c
+++ b/bin/dnssec/dnssec-signzone.c
@@ -784,7 +784,7 @@ hashlist_add_dns_name(hashlist_t *l, /*const*/ dns_name_t *name,
 
 static int
 hashlist_comp(const void *a, const void *b) {
-	return (isc_safe_memcompare(a, b, hash_length + 1));
+	return (memcmp(a, b, hash_length + 1));
 }
 
 static void
diff --git a/lib/dns/nsec3.c b/lib/dns/nsec3.c
index 6ae7ca8..01426d6 100644
--- a/lib/dns/nsec3.c
+++ b/lib/dns/nsec3.c
@@ -1963,7 +1963,7 @@ dns_nsec3_noexistnodata(dns_rdatatype_t type, dns_name_t* name,
 	 * Work out what this NSEC3 covers.
 	 * Inside (<0) or outside (>=0).
 	 */
-	scope = isc_safe_memcompare(owner, nsec3.next, nsec3.next_length);
+	scope = memcmp(owner, nsec3.next, nsec3.next_length);
 
 	/*
 	 * Prepare to compute all the hashes.
@@ -1987,7 +1987,7 @@ dns_nsec3_noexistnodata(dns_rdatatype_t type, dns_name_t* name,
 			return (ISC_R_IGNORE);
 		}
 
-		order = isc_safe_memcompare(hash, owner, length);
+		order = memcmp(hash, owner, length);
 		if (first && order == 0) {
 			/*
 			 * The hashes are the same.
diff --git a/lib/dns/spnego.c b/lib/dns/spnego.c
index ad77f24..670982a 100644
--- a/lib/dns/spnego.c
+++ b/lib/dns/spnego.c
@@ -371,7 +371,7 @@ gssapi_spnego_decapsulate(OM_uint32 *,
 
 /* mod_auth_kerb.c */
 
-static int
+static isc_boolean_t
 cmp_gss_type(gss_buffer_t token, gss_OID gssoid)
 {
 	unsigned char *p;
@@ -395,7 +395,7 @@ cmp_gss_type(gss_buffer_t token, gss_OID gssoid)
 	if (((OM_uint32) *p++) != gssoid->length)
 		return (GSS_S_DEFECTIVE_TOKEN);
 
-	return (isc_safe_memcompare(p, gssoid->elements, gssoid->length));
+	return (!isc_safe_memequal(p, gssoid->elements, gssoid->length));
 }
 
 /* accept_sec_context.c */
diff --git a/lib/isc/Makefile.in b/lib/isc/Makefile.in
index 149552a..8529a86 100644
--- a/lib/isc/Makefile.in
+++ b/lib/isc/Makefile.in
@@ -60,7 +60,7 @@ OBJS =		@ISC_EXTRA_OBJS@ @ISC_PK11_O@ @ISC_PK11_RESULT_O@ \
 		parseint.@O@ portset.@O@ quota.@O@ radix.@O@ random.@O@ \
 		ratelimiter.@O@ refcount.@O@ region.@O@ regex.@O@ result.@O@ \
 		rwlock.@O@ \
-		safe.@O@ serial.@O@ siphash.@O@ sha1.@O@ sha2.@O@ sockaddr.@O@ stats.@O@ \
+		serial.@O@ siphash.@O@ sha1.@O@ sha2.@O@ sockaddr.@O@ stats.@O@ \
 		string.@O@ strtoul.@O@ symtab.@O@ task.@O@ taskpool.@O@ \
 		tm.@O@ timer.@O@ utf8.@O@ version.@O@ \
 		${UNIXOBJS} ${NLSOBJS} ${THREADOBJS}
@@ -79,7 +79,7 @@ SRCS =		@ISC_EXTRA_SRCS@ @ISC_PK11_C@ @ISC_PK11_RESULT_C@ \
 		netaddr.c netscope.c pool.c ondestroy.c \
 		parseint.c portset.c quota.c radix.c random.c ${CHACHASRCS} \
 		ratelimiter.c refcount.c region.c regex.c result.c rwlock.c \
-		safe.c serial.c siphash.c sha1.c sha2.c sockaddr.c stats.c string.c \
+		serial.c siphash.c sha1.c sha2.c sockaddr.c stats.c string.c \
 		strtoul.c symtab.c task.c taskpool.c timer.c \
 		tm.c utf8.c version.c
 
@@ -95,10 +95,6 @@ TESTDIRS =	@UNITTESTS@
 
 @BIND9_MAKE_RULES@
 
-safe.@O@: safe.c
-	${LIBTOOL_MODE_COMPILE} ${CC} ${ALL_CFLAGS} @CCNOOPT@ \
-		-c ${srcdir}/safe.c
-
 version.@O@: version.c
 	${LIBTOOL_MODE_COMPILE} ${CC} ${ALL_CFLAGS} \
 		-DVERSION=\"${VERSION}\" \
diff --git a/lib/isc/include/isc/safe.h b/lib/isc/include/isc/safe.h
index 66ed08b..88b8f47 100644
--- a/lib/isc/include/isc/safe.h
+++ b/lib/isc/include/isc/safe.h
@@ -15,29 +15,19 @@
 
 /*! \file isc/safe.h */
 
-#include <stdbool.h>
-
-#include <isc/types.h>
-#include <stdlib.h>
+#include <isc/lang.h>
+#include <openssl/crypto.h>
 
 ISC_LANG_BEGINDECLS
 
-bool
-isc_safe_memequal(const void *s1, const void *s2, size_t n);
+#define isc_safe_memequal(s1, s2, n) !CRYPTO_memcmp(s1, s2, n)
 /*%<
  * Returns true iff. two blocks of memory are equal, otherwise
  * false.
  *
  */
 
-int
-isc_safe_memcompare(const void *b1, const void *b2, size_t len);
-/*%<
- * Clone of libc memcmp() which is safe to differential timing attacks.
- */
-
-void
-isc_safe_memwipe(void *ptr, size_t len);
+#define isc_safe_memwipe(ptr, len) OPENSSL_cleanse(ptr, len)
 /*%<
  * Clear the memory of length `len` pointed to by `ptr`.
  *
diff --git a/lib/isc/safe.c b/lib/isc/safe.c
deleted file mode 100644
index 7a464b6..0000000
--- a/lib/isc/safe.c
+++ /dev/null
@@ -1,83 +0,0 @@
-/*
- * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
- *
- * This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/.
- *
- * See the COPYRIGHT file distributed with this work for additional
- * information regarding copyright ownership.
- */
-
-/*! \file */
-
-#include <config.h>
-
-#include <stdbool.h>
-
-#include <isc/safe.h>
-#include <isc/string.h>
-#include <isc/util.h>
-
-#ifdef WIN32
-#include <windows.h>
-#endif
-
-#ifdef _MSC_VER
-#pragma optimize("", off)
-#endif
-
-bool
-isc_safe_memequal(const void *s1, const void *s2, size_t n) {
-	uint8_t acc = 0;
-
-	if (n != 0U) {
-		const uint8_t *p1 = s1, *p2 = s2;
-
-		do {
-			acc |= *p1++ ^ *p2++;
-		} while (--n != 0U);
-	}
-	return (acc == 0);
-}
-
-
-int
-isc_safe_memcompare(const void *b1, const void *b2, size_t len) {
-	const unsigned char *p1 = b1, *p2 = b2;
-	size_t i;
-	int res = 0, done = 0;
-
-	for (i = 0; i < len; i++) {
-		/* lt is -1 if p1[i] < p2[i]; else 0. */
-		int lt = (p1[i] - p2[i]) >> CHAR_BIT;
-
-		/* gt is -1 if p1[i] > p2[i]; else 0. */
-		int gt = (p2[i] - p1[i]) >> CHAR_BIT;
-
-		/* cmp is 1 if p1[i] > p2[i]; -1 if p1[i] < p2[i]; else 0. */
-		int cmp = lt - gt;
-
-		/* set res = cmp if !done. */
-		res |= cmp & ~done;
-
-		/* set done if p1[i] != p2[i]. */
-		done |= lt | gt;
-	}
-
-	return (res);
-}
-
-void
-isc_safe_memwipe(void *ptr, size_t len) {
-	if (ISC_UNLIKELY(ptr == NULL || len == 0))
-		return;
-
-#ifdef WIN32
-	SecureZeroMemory(ptr, len);
-#elif HAVE_EXPLICIT_BZERO
-	explicit_bzero(ptr, len);
-#else
-	memset(ptr, 0, len);
-#endif
-}
diff --git a/lib/isc/tests/safe_test.c b/lib/isc/tests/safe_test.c
index 266ac75..60e9181 100644
--- a/lib/isc/tests/safe_test.c
+++ b/lib/isc/tests/safe_test.c
@@ -45,22 +45,6 @@ isc_safe_memequal_test(void **state) {
 				       "\x00\x00\x00\x00", 4));
 }
 
-/* test isc_safe_memcompare() */
-static void
-isc_safe_memcompare_test(void **state) {
-	UNUSED(state);
-
-	assert_int_equal(isc_safe_memcompare("test", "test", 4), 0);
-	assert_true(isc_safe_memcompare("test", "tesc", 4) > 0);
-	assert_true(isc_safe_memcompare("test", "tesy", 4) < 0);
-	assert_int_equal(isc_safe_memcompare("\x00\x00\x00\x00",
-					     "\x00\x00\x00\x00", 4), 0);
-	assert_true(isc_safe_memcompare("\x00\x00\x00\x00",
-					"\x00\x00\x00\x01", 4) < 0);
-	assert_true(isc_safe_memcompare("\x00\x00\x00\x02",
-					"\x00\x00\x00\x00", 4) > 0);
-}
-
 /* test isc_safe_memwipe() */
 static void
 isc_safe_memwipe_test(void **state) {
@@ -69,7 +53,6 @@ isc_safe_memwipe_test(void **state) {
 	/* These should pass. */
 	isc_safe_memwipe(NULL, 0);
 	isc_safe_memwipe((void *) -1, 0);
-	isc_safe_memwipe(NULL, 42);
 
 	/*
 	 * isc_safe_memwipe(ptr, size) should function same as
@@ -108,7 +91,6 @@ main(void) {
 	const struct CMUnitTest tests[] = {
 		cmocka_unit_test(isc_safe_memequal_test),
 		cmocka_unit_test(isc_safe_memwipe_test),
-		cmocka_unit_test(isc_safe_memcompare_test),
 	};
 
 	return (cmocka_run_group_tests(tests, NULL, NULL));
-- 
2.26.2