Commit d870740c authored by Geoff Thorpe's avatar Geoff Thorpe
Browse files

Put the first stage of my bignum debugging adventures into CVS. This code 

is itself experimental, and in addition may cause execution to break on
existing openssl "bugs" that previously were harmless or at least
invisible.
parent d8ec0dcf

CHANGES

+16 −0
Original line number Diff line number Diff line
@@ -4,6 +4,22 @@ 


 Changes between 0.9.7c and 0.9.8  [xx XXX xxxx]



  *) An audit of the BIGNUM code is underway, for which debugging code is

     enabled when BN_DEBUG is defined. This makes stricter enforcements on what

     is considered valid when processing BIGNUMs, and causes execution to

     assert() when a problem is discovered. If BN_DEBUG_RAND is defined,

     further steps are taken to deliberately pollute unused data in BIGNUM

     structures to try and expose faulty code further on. For now, openssl will

     (in its default mode of operation) continue to tolerate the inconsistent

     forms that it has tolerated in the past, but authors and packagers should

     consider trying openssl and their own applications when compiled with

     these debugging symbols defined. It will help highlight potential bugs in

     their own code, and will improve the test coverage for OpenSSL itself. At

     some point, these tighter rules will become openssl's default to improve

     maintainability, though the assert()s and other overheads will remain only

     in debugging configurations. See bn.h for more details.

     [Geoff Thorpe]



  *) BN_CTX_init() has been deprecated, as BN_CTX is an opaque structure

     that can only be obtained through BN_CTX_new() (which implicitly

     initialises it). The presence of this function only made it possible

crypto/bn/bn.h

+77 −1
Original line number Diff line number Diff line
@@ -611,7 +611,34 @@ const BIGNUM *BN_get0_nist_prime_521(void); 
BIGNUM *bn_expand2(BIGNUM *a, int words);

BIGNUM *bn_dup_expand(const BIGNUM *a, int words);



#define bn_fix_top(a) \

/* Bignum consistency macros

 * There is one "API" macro, bn_fix_top(), for stripping leading zeroes from

 * bignum data after direct manipulations on the data. There is also an

 * "internal" macro, bn_check_top(), for verifying that there are no leading

 * zeroes. Unfortunately, some auditing is required due to the fact that

 * bn_fix_top() has become an overabused duct-tape because bignum data is

 * occasionally passed around in an inconsistent state. So the following

 * changes have been made to sort this out;

 * - bn_fix_top()s implementation has been moved to bn_correct_top()

 * - if BN_DEBUG isn't defined, bn_fix_top() maps to bn_correct_top(), and

 *   bn_check_top() is as before.

 * - if BN_DEBUG *is* defined;

 *   - bn_check_top() tries to pollute unused words even if the bignum 'top' is

 *     consistent. (ed: only if BN_DEBUG_RAND is defined)

 *   - bn_fix_top() maps to bn_check_top() rather than "fixing" anything.

 * The idea is to have debug builds flag up inconsistent bignums when they

 * occur. If that occurs in a bn_fix_top(), we examine the code in question; if

 * the use of bn_fix_top() was appropriate (ie. it follows directly after code

 * that manipulates the bignum) it is converted to bn_correct_top(), and if it

 * was not appropriate, we convert it permanently to bn_check_top() and track

 * down the cause of the bug. Eventually, no internal code should be using the

 * bn_fix_top() macro. External applications and libraries should try this with

 * their own code too, both in terms of building against the openssl headers

 * with BN_DEBUG defined *and* linking with a version of OpenSSL built with it

 * defined. This not only improves external code, it provides more test

 * coverage for openssl's own code.

 */

#define bn_correct_top(a) \

        { \

        BN_ULONG *ftl; \

	if ((a)->top > 0) \
@@ -621,6 +648,55 @@ BIGNUM *bn_dup_expand(const BIGNUM *a, int words); 
		} \

	}



/* #define BN_DEBUG_RAND */



#ifdef BN_DEBUG



/* We only need assert() when debugging */

#include <assert.h>



#ifdef BN_DEBUG_RAND

/* To avoid "make update" cvs wars due to BN_DEBUG, use some tricks */

#ifndef RAND_pseudo_bytes

int RAND_pseudo_bytes(unsigned char *buf,int num);

#define BN_DEBUG_TRIX

#endif

#define bn_check_top(a) \

	do { \

		const BIGNUM *_tbignum = (a); \

		assert((_tbignum->top == 0) || \

				(_tbignum->d[_tbignum->top - 1] != 0)); \

		if(_tbignum->top < _tbignum->dmax) { \

			/* We cast away const without the compiler knowing, any \

			 * *genuinely* constant variables that aren't mutable \

			 * wouldn't be constructed with top!=dmax. */ \

			BN_ULONG *_not_const; \

			memcpy(&_not_const, &_tbignum->d, sizeof(BN_ULONG*)); \

			RAND_pseudo_bytes((unsigned char *)(_not_const + _tbignum->top), \

				(_tbignum->dmax - _tbignum->top) * sizeof(BN_ULONG)); \

		} \

	} while(0)

#ifdef BN_DEBUG_TRIX

#undef RAND_pseudo_bytes

#endif

#else /* !BN_DEBUG_RAND */

#define bn_check_top(a) \

	do { \

		const BIGNUM *_tbignum = (a); \

		assert((_tbignum->top == 0) || \

				(_tbignum->d[_tbignum->top - 1] != 0)); \

	} while(0)

#endif



#define bn_fix_top(a)		bn_check_top(a)



#else /* !BN_DEBUG */



#define bn_check_top(a)		do { ; } while(0)

#define bn_fix_top(a)		bn_correct_top(a)



#endif



BN_ULONG bn_mul_add_words(BN_ULONG *rp, const BN_ULONG *ap, int num, BN_ULONG w);

BN_ULONG bn_mul_words(BN_ULONG *rp, const BN_ULONG *ap, int num, BN_ULONG w);

void     bn_sqr_words(BN_ULONG *rp, const BN_ULONG *ap, int num);

crypto/bn/bn_add.c

+4 −1
Original line number Diff line number Diff line
@@ -100,6 +100,7 @@ int BN_add(BIGNUM *r, const BIGNUM *a, const BIGNUM *b) 
		r->neg=1;

	else

		r->neg=0;

	bn_check_top(r);

	return(1);

	}
@@ -161,6 +162,7 @@ int BN_uadd(BIGNUM *r, const BIGNUM *a, const BIGNUM *b) 
		}

	/* memcpy(rp,ap,sizeof(*ap)*(max-i));*/

	r->neg = 0;

	bn_check_top(r);

	return(1);

	}
@@ -253,7 +255,7 @@ int BN_usub(BIGNUM *r, const BIGNUM *a, const BIGNUM *b) 


	r->top=max;

	r->neg=0;

	bn_fix_top(r);

	bn_correct_top(r);

	return(1);

	}
@@ -304,6 +306,7 @@ int BN_sub(BIGNUM *r, const BIGNUM *a, const BIGNUM *b) 
		if (!BN_usub(r,a,b)) return(0);

		r->neg=0;

		}

	bn_check_top(r);

	return(1);

	}

crypto/bn/bn_blind.c

+1 −0
Original line number Diff line number Diff line
@@ -139,6 +139,7 @@ int BN_BLINDING_invert(BIGNUM *n, BN_BLINDING *b, BN_CTX *ctx) 
		if (!BN_BLINDING_update(b,ctx))

			return(0);

		}

	bn_check_top(n);

	return(ret);

	}

crypto/bn/bn_ctx.c

+4 −1
Original line number Diff line number Diff line
@@ -121,8 +121,10 @@ void BN_CTX_free(BN_CTX *ctx) 
	if (ctx == NULL) return;

	assert(ctx->depth == 0);



	for (i=0; i < BN_CTX_NUM; i++)

	for (i=0; i < BN_CTX_NUM; i++) {

		bn_check_top(&(ctx->bn[i]));

		BN_clear_free(&(ctx->bn[i]));

	}

	if (ctx->flags & BN_FLG_MALLOCED)

		OPENSSL_free(ctx);

	}
@@ -152,6 +154,7 @@ BIGNUM *BN_CTX_get(BN_CTX *ctx) 
			}

		return NULL;

		}

	bn_check_top(&(ctx->bn[ctx->tos]));

	return (&(ctx->bn[ctx->tos++]));

	}