On Thu, 25 Sep 2003, Geoff Thorpe wrote:
> On September 25, 2003 03:33 am, Nils Larsch wrote:
> > Otto Moerbeek wrote:
> > ....
> >
> > > OK, that would amount to the fixes below:
> > >
> > > - in BN_cmp, call bn_fix_top just before comparing the two tops.
> >
> > Not really necessary as the normal BN_* functions which change the
> > value of the bignum should always ensure that the top value is
> > correct (i.e. as small as possible), but of course adding bn_fix_top
> > does not really harm (besides wasting some cpu time :-) .
>
> I would go one step further and suggest that BN_cmp() and operations like
> it that should treat their inputs as "const" should not modify the
> BIGNUMs at all. Fixing BIGNUMs wherever we spot problems helps propogate
> the current mess, the correct fix (as Nils implied) is to trace the
> problem back to where it is caused and fix it there. No BIGNUM structure
> should ever be in an invalid state outside the scope of a BN_*** API
> function (or macro). So each function should, on exit, have correctly
> adjusted any data as necessary.
Well mmust say I agree, but until a more or less complete audit of the
BN code has been done, I like to bet on the safe side.
The moment one is sure that all BN ops result in valid BN's, the extra
bn_fix_top() calls can and should be removed. But since my work does
concentrate on using the NM lib, and _not_ on maintaining it, i went for
the safe, albeit ugly solution.
> Perhaps (any volunteers?) the best way to do this would be to define a
> validation function/macro and call it from every exit path for every
> BIGNUM API function and macro. This could sanity-check the in/out
> variables and abort() if there's something wrong so that the debugger can
> catch these problems where they occur rather than where we first notice
> them (later on in things like BN_cmp). IMHO this would be infinitely more
> useful than continually adding calls to bn_fix_top() everywhere it seems
> it could seal off another "case".
This is extra tricky. The particular problem I found was not dependent on
the exit path taken, but on the particular values supplied to
BN_add_word(). But having a test set that has a full return path coverage
_would_ be a good start.
> > Perhaps you should send a bug report to [EMAIL PROTECTED] containing the
> > patch and a link to this thread.
>
> Yup, this should certainly go into RT - but I think this would be the
> ideal opportunity to audit the BIGNUM code properly for this sort of
> thing. In particular, it would be nice if the result of this work would
> also allow us to better constify the BIGNUM API (in CVS HEAD of course,
> the stable branches would merely benefit from cleaner implementations;-).
In the meantime, the bug report has been submitted.
-Otto
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List [EMAIL PROTECTED]
Automated List Manager [EMAIL PROTECTED]
