From 61d9e528a4d53a1dbd6e0a27694a920029bdcae7 Mon Sep 17 00:00:00 2001 From: Daniel Mendler Date: Sat, 11 May 2019 10:22:20 +0200 Subject: [PATCH] hardening: add MP_ZERO_BUFFER, MP_ZERO_DIGITS * (!) Always zero buffer before freeing if MP_NO_ZERO_ON_FREE is not defined * Add MP_NO_ZERO_ON_FREE to disable hardening * Add MP_ZERO_BUFFER, MP_ZERO_DIGITS, MP_FREE_BUFFFER, MP_FREE_DIGITS * Never use MP_FREE directly, use MP_FREE_DIGITS or MP_FREE_BUFFER * Add MP_USE_MEMSET to use memset instead of loop * Disable astyle backups which are annonying in the times of git --- astylerc | 3 +++ bn_mp_add_d.c | 4 +--- bn_mp_and.c | 4 +--- bn_mp_clear.c | 9 +------ bn_mp_copy.c | 4 +--- bn_mp_div_2.c | 5 +--- bn_mp_dr_reduce.c | 4 +--- bn_mp_fwrite.c | 6 ++--- bn_mp_grow.c | 4 +--- bn_mp_mod_2d.c | 6 ++--- bn_mp_mul_2.c | 5 +--- bn_mp_mul_d.c | 4 +--- bn_mp_prime_rand.c | 2 +- bn_mp_sub_d.c | 5 ++-- bn_mp_zero.c | 9 +------ bn_s_mp_add.c | 4 +--- bn_s_mp_montgomery_reduce_fast.c | 8 +++---- bn_s_mp_mul_digs_fast.c | 4 +--- bn_s_mp_mul_high_digs_fast.c | 4 +--- bn_s_mp_sqr_fast.c | 4 +--- bn_s_mp_sub.c | 4 +--- tommath_private.h | 40 ++++++++++++++++++++++++++++++-- 22 files changed, 68 insertions(+), 74 deletions(-) diff --git a/astylerc b/astylerc index 5d63f7a..c5ff779 100644 --- a/astylerc +++ b/astylerc @@ -4,6 +4,9 @@ # usage: # astyle --options=astylerc *.[ch] +# Do not create backup, annonying in the times of git +suffix=none + ## Bracket Style Options style=kr diff --git a/bn_mp_add_d.c b/bn_mp_add_d.c index 9b408c4..c02cb4e 100644 --- a/bn_mp_add_d.c +++ b/bn_mp_add_d.c @@ -85,9 +85,7 @@ int mp_add_d(const mp_int *a, mp_digit b, mp_int *c) c->sign = MP_ZPOS; /* now zero to oldused */ - while (ix++ < oldused) { - *tmpc++ = 0; - } + MP_ZERO_DIGITS(tmpc, oldused - ix); mp_clamp(c); return MP_OKAY; diff --git a/bn_mp_and.c b/bn_mp_and.c index 7f94d35..78e42d9 100644 --- a/bn_mp_and.c +++ b/bn_mp_and.c @@ -29,9 +29,7 @@ int mp_and(const mp_int *a, const mp_int *b, mp_int *c) } /* zero digits above the last from the smallest mp_int */ - for (; ix < t.used; ix++) { - t.dp[ix] = 0; - } + MP_ZERO_DIGITS(t.dp + ix, t.used - ix); mp_clamp(&t); mp_exch(c, &t); diff --git a/bn_mp_clear.c b/bn_mp_clear.c index cf52d15..ff78324 100644 --- a/bn_mp_clear.c +++ b/bn_mp_clear.c @@ -6,17 +6,10 @@ /* clear one (frees) */ void mp_clear(mp_int *a) { - int i; - /* only do anything if a hasn't been freed previously */ if (a->dp != NULL) { - /* first zero the digits */ - for (i = 0; i < a->used; i++) { - a->dp[i] = 0; - } - /* free ram */ - MP_FREE(a->dp, sizeof(mp_digit) * (size_t)a->alloc); + MP_FREE_DIGITS(a->dp, a->alloc); /* reset members to make debugging easier */ a->dp = NULL; diff --git a/bn_mp_copy.c b/bn_mp_copy.c index 85eb787..19fa9dc 100644 --- a/bn_mp_copy.c +++ b/bn_mp_copy.c @@ -38,9 +38,7 @@ int mp_copy(const mp_int *a, mp_int *b) } /* clear high digits */ - for (; n < b->used; n++) { - *tmpb++ = 0; - } + MP_ZERO_DIGITS(tmpb, b->used - n); } /* copy used count and sign */ diff --git a/bn_mp_div_2.c b/bn_mp_div_2.c index 1cf7c8d..7935235 100644 --- a/bn_mp_div_2.c +++ b/bn_mp_div_2.c @@ -40,10 +40,7 @@ int mp_div_2(const mp_int *a, mp_int *b) } /* zero excess digits */ - tmpb = b->dp + b->used; - for (x = b->used; x < oldused; x++) { - *tmpb++ = 0; - } + MP_ZERO_DIGITS(b->dp + b->used, oldused - b->used); } b->sign = a->sign; mp_clamp(b); diff --git a/bn_mp_dr_reduce.c b/bn_mp_dr_reduce.c index 83edbb6..b82b33d 100644 --- a/bn_mp_dr_reduce.c +++ b/bn_mp_dr_reduce.c @@ -58,9 +58,7 @@ top: *tmpx1++ = mu; /* zero words above m */ - for (i = m + 1; i < x->used; i++) { - *tmpx1++ = 0; - } + MP_ZERO_DIGITS(tmpx1, x->used - m - 1); /* clamp, sub and return */ mp_clamp(x); diff --git a/bn_mp_fwrite.c b/bn_mp_fwrite.c index d338604..1904237 100644 --- a/bn_mp_fwrite.c +++ b/bn_mp_fwrite.c @@ -19,18 +19,18 @@ int mp_fwrite(const mp_int *a, int radix, FILE *stream) } if ((err = mp_toradix(a, buf, radix)) != MP_OKAY) { - MP_FREE(buf, len); + MP_FREE_BUFFER(buf, (size_t)len); return err; } for (x = 0; x < len; x++) { if (fputc((int)buf[x], stream) == EOF) { - MP_FREE(buf, len); + MP_FREE_BUFFER(buf, (size_t)len); return MP_VAL; } } - MP_FREE(buf, len); + MP_FREE_BUFFER(buf, (size_t)len); return MP_OKAY; } #endif diff --git a/bn_mp_grow.c b/bn_mp_grow.c index 8011923..03ef6fa 100644 --- a/bn_mp_grow.c +++ b/bn_mp_grow.c @@ -34,9 +34,7 @@ int mp_grow(mp_int *a, int size) /* zero excess digits */ i = a->alloc; a->alloc = size; - for (; i < a->alloc; i++) { - a->dp[i] = 0; - } + MP_ZERO_DIGITS(a->dp + i, a->alloc - i); } return MP_OKAY; } diff --git a/bn_mp_mod_2d.c b/bn_mp_mod_2d.c index 0dbf55f..db58771 100644 --- a/bn_mp_mod_2d.c +++ b/bn_mp_mod_2d.c @@ -25,9 +25,9 @@ int mp_mod_2d(const mp_int *a, int b, mp_int *c) } /* zero digits above the last digit of the modulus */ - for (x = (b / MP_DIGIT_BIT) + (((b % MP_DIGIT_BIT) == 0) ? 0 : 1); x < c->used; x++) { - c->dp[x] = 0; - } + x = (b / MP_DIGIT_BIT) + (((b % MP_DIGIT_BIT) == 0) ? 0 : 1); + MP_ZERO_DIGITS(c->dp + x, c->used - x); + /* clear the digit that is not completely outside/inside the modulus */ c->dp[b / MP_DIGIT_BIT] &= ((mp_digit)1 << (mp_digit)(b % MP_DIGIT_BIT)) - (mp_digit)1; diff --git a/bn_mp_mul_2.c b/bn_mp_mul_2.c index cd87d71..75692d3 100644 --- a/bn_mp_mul_2.c +++ b/bn_mp_mul_2.c @@ -55,10 +55,7 @@ int mp_mul_2(const mp_int *a, mp_int *b) /* now zero any excess digits on the destination * that we didn't write to */ - tmpb = b->dp + b->used; - for (x = b->used; x < oldused; x++) { - *tmpb++ = 0; - } + MP_ZERO_DIGITS(b->dp + b->used, oldused - b->used); } b->sign = a->sign; return MP_OKAY; diff --git a/bn_mp_mul_d.c b/bn_mp_mul_d.c index 8419f72..74d69ce 100644 --- a/bn_mp_mul_d.c +++ b/bn_mp_mul_d.c @@ -49,9 +49,7 @@ int mp_mul_d(const mp_int *a, mp_digit b, mp_int *c) ++ix; /* now zero digits above the top */ - while (ix++ < olduse) { - *tmpc++ = 0; - } + MP_ZERO_DIGITS(tmpc, olduse - ix); /* set used count */ c->used = a->used + 1; diff --git a/bn_mp_prime_rand.c b/bn_mp_prime_rand.c index 975bde8..e4bc1a4 100644 --- a/bn_mp_prime_rand.c +++ b/bn_mp_prime_rand.c @@ -114,7 +114,7 @@ static int s_mp_prime_random_ex(mp_int *a, int t, int size, int flags, private_m err = MP_OKAY; error: - MP_FREE(tmp, bsize); + MP_FREE_BUFFER(tmp, (size_t)bsize); return err; } diff --git a/bn_mp_sub_d.c b/bn_mp_sub_d.c index 4e3ae02..fc45f97 100644 --- a/bn_mp_sub_d.c +++ b/bn_mp_sub_d.c @@ -67,9 +67,8 @@ int mp_sub_d(const mp_int *a, mp_digit b, mp_int *c) } /* zero excess digits */ - while (ix++ < oldused) { - *tmpc++ = 0; - } + MP_ZERO_DIGITS(tmpc, oldused - ix); + mp_clamp(c); return MP_OKAY; } diff --git a/bn_mp_zero.c b/bn_mp_zero.c index e2f7e50..72a255e 100644 --- a/bn_mp_zero.c +++ b/bn_mp_zero.c @@ -6,15 +6,8 @@ /* set to zero */ void mp_zero(mp_int *a) { - int n; - mp_digit *tmp; - a->sign = MP_ZPOS; a->used = 0; - - tmp = a->dp; - for (n = 0; n < a->alloc; n++) { - *tmp++ = 0; - } + MP_ZERO_DIGITS(a->dp, a->alloc); } #endif diff --git a/bn_s_mp_add.c b/bn_s_mp_add.c index 77b851e..e87dc9f 100644 --- a/bn_s_mp_add.c +++ b/bn_s_mp_add.c @@ -81,9 +81,7 @@ int s_mp_add(const mp_int *a, const mp_int *b, mp_int *c) *tmpc++ = u; /* clear digits above oldused */ - for (i = c->used; i < olduse; i++) { - *tmpc++ = 0; - } + MP_ZERO_DIGITS(tmpc, olduse - c->used); } mp_clamp(c); diff --git a/bn_s_mp_montgomery_reduce_fast.c b/bn_s_mp_montgomery_reduce_fast.c index a9a9a60..688049c 100644 --- a/bn_s_mp_montgomery_reduce_fast.c +++ b/bn_s_mp_montgomery_reduce_fast.c @@ -49,8 +49,8 @@ int s_mp_montgomery_reduce_fast(mp_int *x, const mp_int *n, mp_digit rho) } /* zero the high words of W[a->used..m->used*2] */ - for (; ix < ((n->used * 2) + 1); ix++) { - *_W++ = 0; + if (ix < ((n->used * 2) + 1)) { + MP_ZERO_BUFFER(_W, sizeof(mp_word) * (size_t)(((n->used * 2) + 1) - ix)); } } @@ -142,9 +142,7 @@ int s_mp_montgomery_reduce_fast(mp_int *x, const mp_int *n, mp_digit rho) /* zero oldused digits, if the input a was larger than * m->used+1 we'll have to clear the digits */ - for (; ix < olduse; ix++) { - *tmpx++ = 0; - } + MP_ZERO_DIGITS(tmpx, olduse - ix); } /* set the max used and clamp */ diff --git a/bn_s_mp_mul_digs_fast.c b/bn_s_mp_mul_digs_fast.c index e9af5ed..2361433 100644 --- a/bn_s_mp_mul_digs_fast.c +++ b/bn_s_mp_mul_digs_fast.c @@ -81,9 +81,7 @@ int s_mp_mul_digs_fast(const mp_int *a, const mp_int *b, mp_int *c, int digs) } /* clear unused digits [that existed in the old copy of c] */ - for (; ix < olduse; ix++) { - *tmpc++ = 0; - } + MP_ZERO_DIGITS(tmpc, olduse - ix); } mp_clamp(c); return MP_OKAY; diff --git a/bn_s_mp_mul_high_digs_fast.c b/bn_s_mp_mul_high_digs_fast.c index fe4bd8e..27242a1 100644 --- a/bn_s_mp_mul_high_digs_fast.c +++ b/bn_s_mp_mul_high_digs_fast.c @@ -72,9 +72,7 @@ int s_mp_mul_high_digs_fast(const mp_int *a, const mp_int *b, mp_int *c, int dig } /* clear unused digits [that existed in the old copy of c] */ - for (; ix < olduse; ix++) { - *tmpc++ = 0; - } + MP_ZERO_DIGITS(tmpc, olduse - ix); } mp_clamp(c); return MP_OKAY; diff --git a/bn_s_mp_sqr_fast.c b/bn_s_mp_sqr_fast.c index 42548c0..304500c 100644 --- a/bn_s_mp_sqr_fast.c +++ b/bn_s_mp_sqr_fast.c @@ -88,9 +88,7 @@ int s_mp_sqr_fast(const mp_int *a, mp_int *b) } /* clear unused digits [that existed in the old copy of c] */ - for (; ix < olduse; ix++) { - *tmpb++ = 0; - } + MP_ZERO_DIGITS(tmpb, olduse - ix); } mp_clamp(b); return MP_OKAY; diff --git a/bn_s_mp_sub.c b/bn_s_mp_sub.c index 2bf0679..ffd8272 100644 --- a/bn_s_mp_sub.c +++ b/bn_s_mp_sub.c @@ -60,9 +60,7 @@ int s_mp_sub(const mp_int *a, const mp_int *b, mp_int *c) } /* clear digits above used (since we may not have grown result above) */ - for (i = c->used; i < olduse; i++) { - *tmpc++ = 0; - } + MP_ZERO_DIGITS(tmpc, olduse - c->used); } mp_clamp(c); diff --git a/tommath_private.h b/tommath_private.h index de87d3c..466e727 100644 --- a/tommath_private.h +++ b/tommath_private.h @@ -10,6 +10,42 @@ extern "C" { #endif +/* Hardening libtommath + * -------------------- + * + * By default memory is zeroed before calling + * MP_FREE to avoid leaking data. This is good + * practice in cryptographical applications. + * + * Note however that memory allocators used + * in cryptographical applications can often + * be configured by itself to clear memory, + * rendering the clearing in tommath unnecessary. + * See for example https://github.com/GrapheneOS/hardened_malloc + * and the option CONFIG_ZERO_ON_FREE. + * + * Furthermore there are applications which + * value performance more and want this + * feature to be disabled. For such applications + * define MP_NO_ZERO_ON_FREE during compilation. + */ +#ifdef MP_NO_ZERO_ON_FREE +# define MP_FREE_BUFFER(mem, size) MP_FREE((mem), (size)) +# define MP_FREE_DIGITS(mem, digits) MP_FREE((mem), sizeof (mp_digit) * (digits)) +#else +# define MP_FREE_BUFFER(mem, size) do { size_t fs_ = (size); void* fm_ = (mem); if (fm_) { MP_ZERO_BUFFER(fm_, fs_); MP_FREE(fm_, fs_); } } while (0) +# define MP_FREE_DIGITS(mem, digits) do { int fd_ = (digits); void* fm_ = (mem); if (fm_) { MP_ZERO_BUFFER(fm_, sizeof (mp_digit) * (size_t)fd_); MP_FREE(fm_, sizeof (mp_digit) * (size_t)fd_); } } while (0) +#endif + +#ifdef MP_USE_MEMSET +# include +# define MP_ZERO_BUFFER(mem, size) memset((mem), 0, (size)) +# define MP_ZERO_DIGITS(mem, digits) do { int zd_ = (digits); if (zd_ > 0) { memset((mem), 0, sizeof (mp_digit) * (size_t)zd_); } } while (0) +#else +# define MP_ZERO_BUFFER(mem, size) do { size_t zs_ = (size); char* zm_ = (char*)(mem); while (zs_-- > 0) { *zm_++ = 0; } } while (0) +# define MP_ZERO_DIGITS(mem, digits) do { int zd_ = (digits); mp_digit* zm_ = (mem); while (zd_-- > 0) { *zm_++ = 0; } } while (0) +#endif + /* Tunable cutoffs * --------------- * @@ -43,8 +79,8 @@ extern "C" { /* default to libc stuff */ # include # define MP_MALLOC(size) malloc(size) -# define MP_REALLOC(mem, oldsize, newsize) realloc(mem, newsize) -# define MP_CALLOC(nmemb, size) calloc(nmemb, size) +# define MP_REALLOC(mem, oldsize, newsize) realloc((mem), (newsize)) +# define MP_CALLOC(nmemb, size) calloc((nmemb), (size)) # define MP_FREE(mem, size) free(mem) #else /* prototypes for our heap functions */