From 59f9097d5c3dc010847c359888d31757d1c97904 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 10 May 2011 16:58:38 -0400 Subject: Hand-conversion and audit phase of memcmp transition Here I looked at the results of the automated conversion and cleaned them up as follows: If there was a tor_memcmp or tor_memeq that was in fact "safe"[*] I changed it to a fast_memcmp or fast_memeq. Otherwise if there was a tor_memcmp that could turn into a tor_memneq or tor_memeq, I converted it. This wants close attention. [*] I'm erring on the side of caution here, and leaving some things as tor_memcmp that could in my opinion use the data-dependent fast_memcmp variant. --- src/common/compat.c | 4 +++- src/common/container.c | 2 +- src/common/crypto.c | 2 +- src/common/di_ops.h | 2 ++ src/common/torgzip.c | 2 +- src/common/util.c | 21 +++++++++++++-------- src/common/util.h | 4 ++-- 7 files changed, 23 insertions(+), 14 deletions(-) (limited to 'src/common') diff --git a/src/common/compat.c b/src/common/compat.c index ea46d43d82..39651084a0 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -312,6 +312,8 @@ tor_vsnprintf(char *str, size_t size, const char *format, va_list args) * needle, return a pointer to the first occurrence of the needle * within the haystack, or NULL if there is no such occurrence. * + * This function is not timing-safe. + * * Requires that nlen be greater than zero. */ const void * @@ -336,7 +338,7 @@ tor_memmem(const void *_haystack, size_t hlen, while ((p = memchr(p, first, end-p))) { if (p+nlen > end) return NULL; - if (tor_memeq(p, needle, nlen)) + if (fast_memeq(p, needle, nlen)) return p; ++p; } diff --git a/src/common/container.c b/src/common/container.c index d1d5ce339d..c741eb0206 100644 --- a/src/common/container.c +++ b/src/common/container.c @@ -223,7 +223,7 @@ smartlist_digest_isin(const smartlist_t *sl, const char *element) int i; if (!sl) return 0; for (i=0; i < sl->num_used; i++) - if (tor_memcmp((const char*)sl->list[i],element,DIGEST_LEN)==0) + if (tor_memeq((const char*)sl->list[i],element,DIGEST_LEN)) return 1; return 0; } diff --git a/src/common/crypto.c b/src/common/crypto.c index 926942897f..f3268fe183 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -845,7 +845,7 @@ crypto_pk_public_checksig_digest(crypto_pk_env_t *env, const char *data, tor_free(buf); return -1; } - if (tor_memcmp(buf, digest, DIGEST_LEN)) { + if (tor_memneq(buf, digest, DIGEST_LEN)) { log_warn(LD_CRYPTO, "Signature mismatched with digest."); tor_free(buf); return -1; diff --git a/src/common/di_ops.h b/src/common/di_ops.h index 1b223d9ab6..4a212b0ca2 100644 --- a/src/common/di_ops.h +++ b/src/common/di_ops.h @@ -24,5 +24,7 @@ int tor_memeq(const void *a, const void *b, size_t sz); * implementation. */ #define fast_memcmp(a,b,c) (memcmp((a),(b),(c))) +#define fast_memeq(a,b,c) (0==memcmp((a),(b),(c))) +#define fast_memneq(a,b,c) (0!=memcmp((a),(b),(c))) #endif diff --git a/src/common/torgzip.c b/src/common/torgzip.c index 51b29baca1..f5709aaf3c 100644 --- a/src/common/torgzip.c +++ b/src/common/torgzip.c @@ -356,7 +356,7 @@ tor_gzip_uncompress(char **out, size_t *out_len, compress_method_t detect_compression_method(const char *in, size_t in_len) { - if (in_len > 2 && tor_memeq(in, "\x1f\x8b", 2)) { + if (in_len > 2 && fast_memeq(in, "\x1f\x8b", 2)) { return GZIP_METHOD; } else if (in_len > 2 && (in[0] & 0x0f) == 8 && (ntohs(get_uint16(in)) % 31) == 0) { diff --git a/src/common/util.c b/src/common/util.c index cb2cfed64d..879a0e4bd3 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -459,7 +459,7 @@ strcmp_len(const char *s1, const char *s2, size_t s1_len) return -1; if (s1_len > s2_len) return 1; - return tor_memcmp(s1, s2, s2_len); + return fast_memcmp(s1, s2, s2_len); } /** Compares the first strlen(s2) characters of s1 with s2. Returns as for @@ -501,17 +501,17 @@ strcasecmpend(const char *s1, const char *s2) /** Compare the value of the string prefix with the start of the * memlen-byte memory chunk at mem. Return as for strcmp. * - * [As tor_memcmp(mem, prefix, strlen(prefix)) but returns -1 if memlen is less - * than strlen(prefix).] + * [As fast_memcmp(mem, prefix, strlen(prefix)) but returns -1 if memlen is + * less than strlen(prefix).] */ int -memcmpstart(const void *mem, size_t memlen, +fast_memcmpstart(const void *mem, size_t memlen, const char *prefix) { size_t plen = strlen(prefix); if (memlen < plen) return -1; - return tor_memcmp(mem, prefix, plen); + return fast_memcmp(mem, prefix, plen); } /** Return a pointer to the first char of s that is not whitespace and @@ -644,14 +644,16 @@ tor_mem_is_zero(const char *mem, size_t len) 0,0,0,0, 0,0,0,0, 0,0,0,0, 0,0,0,0, 0,0,0,0, 0,0,0,0, 0,0,0,0, 0,0,0,0, }; while (len >= sizeof(ZERO)) { - if (tor_memcmp(mem, ZERO, sizeof(ZERO))) + /* It's safe to use fast_memcmp here, since the very worst thing an + * attacker could learn is how many initial bytes of a secret were zero */ + if (fast_memcmp(mem, ZERO, sizeof(ZERO))) return 0; len -= sizeof(ZERO); mem += sizeof(ZERO); } /* Deal with leftover bytes. */ if (len) - return tor_memeq(mem, ZERO, len); + return fast_memeq(mem, ZERO, len); return 1; } @@ -660,7 +662,10 @@ tor_mem_is_zero(const char *mem, size_t len) int tor_digest_is_zero(const char *digest) { - return tor_mem_is_zero(digest, DIGEST_LEN); + static const uint8_t ZERO_DIGEST[] = { + 0,0,0,0, 0,0,0,0, 0,0,0,0, 0,0,0,0, 0,0,0,0 + }; + return tor_memeq(digest, ZERO_DIGEST, DIGEST_LEN); } /* Helper: common code to check whether the result of a strtol or strtoul or diff --git a/src/common/util.h b/src/common/util.h index 7bc5286a84..1012a111da 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -172,8 +172,8 @@ int strcasecmpstart(const char *s1, const char *s2) int strcmpend(const char *s1, const char *s2) ATTR_PURE ATTR_NONNULL((1,2)); int strcasecmpend(const char *s1, const char *s2) ATTR_PURE ATTR_NONNULL((1,2)); -int memcmpstart(const void *mem, size_t memlen, - const char *prefix) ATTR_PURE; +int fast_memcmpstart(const void *mem, size_t memlen, + const char *prefix) ATTR_PURE; void tor_strstrip(char *s, const char *strip) ATTR_NONNULL((1,2)); long tor_parse_long(const char *s, int base, long min, -- cgit v1.2.3-54-g00ecf