From 73758ba64ebc0911446e50273982f3e4b725b3f2 Mon Sep 17 00:00:00 2001 From: Colm MacCarthaigh Date: Fri, 21 Aug 2015 09:56:37 -0700 Subject: [PATCH 1/5] Avoid calling memset with 0 length arguments Per issue #161 , there are compiler optimizations that mean calling memset() with a NULL pointer, even when the length argument is 0, is a bad idea which may lead to undefined behavior. This change guards two calls to memset() in the s2n code base that may result in such a call. fixes #161 --- crypto/s2n_hmac.c | 2 +- stuffer/s2n_stuffer.c | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/crypto/s2n_hmac.c b/crypto/s2n_hmac.c index 7bfa1d7ef5e..65429803c78 100644 --- a/crypto/s2n_hmac.c +++ b/crypto/s2n_hmac.c @@ -149,7 +149,7 @@ int s2n_hmac_init(struct s2n_hmac_state *state, s2n_hmac_algorithm alg, const vo memcpy_check(state->xor_pad, state->digest_pad, state->digest_size); copied = state->digest_size; - } else { + } else if (klen) { memcpy_check(state->xor_pad, key, klen); } diff --git a/stuffer/s2n_stuffer.c b/stuffer/s2n_stuffer.c index 33f5c60974c..9037e0a99cf 100644 --- a/stuffer/s2n_stuffer.c +++ b/stuffer/s2n_stuffer.c @@ -121,8 +121,10 @@ int s2n_stuffer_wipe_n(struct s2n_stuffer *stuffer, const uint32_t size) } /* Use '0' instead of 0 precisely to prevent C string compatibility */ - memset(stuffer->blob.data + stuffer->write_cursor - n, '0', n); - stuffer->write_cursor -= n; + if (n) { + memset(stuffer->blob.data + stuffer->write_cursor - n, '0', n); + stuffer->write_cursor -= n; + } if (stuffer->write_cursor == 0) { stuffer->wiped = 1; From 3608f84ecf8a4977bc79c3897de8d688299c8a82 Mon Sep 17 00:00:00 2001 From: Colm MacCarthaigh Date: Fri, 28 Aug 2015 13:17:39 -0700 Subject: [PATCH 2/5] Make memset_check and memcpy_check more defensive This change modifies memset_check and memcpy_check to bail out if there is no work to be done. --- crypto/s2n_hmac.c | 2 +- stuffer/s2n_stuffer.c | 6 ++---- utils/s2n_safety.h | 4 ++-- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/crypto/s2n_hmac.c b/crypto/s2n_hmac.c index 65429803c78..7bfa1d7ef5e 100644 --- a/crypto/s2n_hmac.c +++ b/crypto/s2n_hmac.c @@ -149,7 +149,7 @@ int s2n_hmac_init(struct s2n_hmac_state *state, s2n_hmac_algorithm alg, const vo memcpy_check(state->xor_pad, state->digest_pad, state->digest_size); copied = state->digest_size; - } else if (klen) { + } else { memcpy_check(state->xor_pad, key, klen); } diff --git a/stuffer/s2n_stuffer.c b/stuffer/s2n_stuffer.c index 9037e0a99cf..6853ecceba1 100644 --- a/stuffer/s2n_stuffer.c +++ b/stuffer/s2n_stuffer.c @@ -121,10 +121,8 @@ int s2n_stuffer_wipe_n(struct s2n_stuffer *stuffer, const uint32_t size) } /* Use '0' instead of 0 precisely to prevent C string compatibility */ - if (n) { - memset(stuffer->blob.data + stuffer->write_cursor - n, '0', n); - stuffer->write_cursor -= n; - } + memset_check(stuffer->blob.data + stuffer->write_cursor - n, '0', n); + stuffer->write_cursor -= n; if (stuffer->write_cursor == 0) { stuffer->wiped = 1; diff --git a/utils/s2n_safety.h b/utils/s2n_safety.h index a2ac1748d3b..92669998bd4 100644 --- a/utils/s2n_safety.h +++ b/utils/s2n_safety.h @@ -27,8 +27,8 @@ /* Check memcpy's return, if it's not right (very unlikely!) bail, set an error * err and return -1; */ -#define memcpy_check( d, s, n ) do { notnull_check( (d) ); memcpy( (d), (s), (n)); } while(0) -#define memset_check( d, c, n ) do { notnull_check( (d) ); memset( (d), (c), (n)); } while(0) +#define memcpy_check( d, s, n ) do { notnull_check( (d) ); if ( (n) && (s) ) { memcpy( (d), (s), (n)); } } while(0) +#define memset_check( d, c, n ) do { notnull_check( (d) ); if ( (n) && (s) ) { memset( (d), (c), (n)); } } while(0) /* Range check a number */ #define gte_check(n, min) do { if ( (n) < min ) { S2N_ERROR(S2N_ERR_SAFETY); } } while(0) From a9652c1d9161bb40311577f3d2aefbc39b898023 Mon Sep 17 00:00:00 2001 From: Colm MacCarthaigh Date: Fri, 28 Aug 2015 13:38:38 -0700 Subject: [PATCH 3/5] (s) -> (c) variable typo fix --- utils/s2n_safety.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/s2n_safety.h b/utils/s2n_safety.h index 92669998bd4..704f56733ef 100644 --- a/utils/s2n_safety.h +++ b/utils/s2n_safety.h @@ -28,7 +28,7 @@ * err and return -1; */ #define memcpy_check( d, s, n ) do { notnull_check( (d) ); if ( (n) && (s) ) { memcpy( (d), (s), (n)); } } while(0) -#define memset_check( d, c, n ) do { notnull_check( (d) ); if ( (n) && (s) ) { memset( (d), (c), (n)); } } while(0) +#define memset_check( d, c, n ) do { notnull_check( (d) ); if ( (n) && (c) ) { memset( (d), (c), (n)); } } while(0) /* Range check a number */ #define gte_check(n, min) do { if ( (n) < min ) { S2N_ERROR(S2N_ERR_SAFETY); } } while(0) From 889ee817b2d0f812bdff775ffe9ed5add1f10807 Mon Sep 17 00:00:00 2001 From: Colm MacCarthaigh Date: Fri, 28 Aug 2015 13:43:44 -0700 Subject: [PATCH 4/5] Permit memset to literal zeroes --- utils/s2n_safety.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/utils/s2n_safety.h b/utils/s2n_safety.h index 704f56733ef..574829dfd1c 100644 --- a/utils/s2n_safety.h +++ b/utils/s2n_safety.h @@ -27,8 +27,8 @@ /* Check memcpy's return, if it's not right (very unlikely!) bail, set an error * err and return -1; */ -#define memcpy_check( d, s, n ) do { notnull_check( (d) ); if ( (n) && (s) ) { memcpy( (d), (s), (n)); } } while(0) -#define memset_check( d, c, n ) do { notnull_check( (d) ); if ( (n) && (c) ) { memset( (d), (c), (n)); } } while(0) +#define memcpy_check( d, s, n ) do { if ( (n) && (s) ) { notnull_check( (d) ); memcpy( (d), (s), (n)); } } while(0) +#define memset_check( d, c, n ) do { if ( (n) ) { notnull_check( (d) ); memset( (d), (c), (n)); } } while(0) /* Range check a number */ #define gte_check(n, min) do { if ( (n) < min ) { S2N_ERROR(S2N_ERR_SAFETY); } } while(0) From 73eef7607fa04a12f5d951163248f2c048a2282a Mon Sep 17 00:00:00 2001 From: Colm MacCarthaigh Date: Fri, 28 Aug 2015 13:49:28 -0700 Subject: [PATCH 5/5] Checking for a NULL source is unneccessary This change removes the NULL source pointer check from memcpy_check, the n == 0 guard is sufficient and avoids the address warnings. --- utils/s2n_safety.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/s2n_safety.h b/utils/s2n_safety.h index 574829dfd1c..951e1fcfebc 100644 --- a/utils/s2n_safety.h +++ b/utils/s2n_safety.h @@ -27,7 +27,7 @@ /* Check memcpy's return, if it's not right (very unlikely!) bail, set an error * err and return -1; */ -#define memcpy_check( d, s, n ) do { if ( (n) && (s) ) { notnull_check( (d) ); memcpy( (d), (s), (n)); } } while(0) +#define memcpy_check( d, s, n ) do { if ( (n) ) { notnull_check( (d) ); memcpy( (d), (s), (n)); } } while(0) #define memset_check( d, c, n ) do { if ( (n) ) { notnull_check( (d) ); memset( (d), (c), (n)); } } while(0) /* Range check a number */