Hi there, On September 25, 2003 02:29 pm, Otto Moerbeek wrote: > On Thu, 25 Sep 2003, Geoff Thorpe wrote: > > 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.
I understand that, and if someone else is prepared to verify and assure themselves that the patch is acceptable, I won't object to them committing it. However, I don't *like* us committing more hacks when there are already too many, and your bug-report and patch provided a suitable moment to discuss the issue. Unfortunately the "Right Thing"(tm) to do that this point would require someone to invest some time in an audit. I don't have that time, but have it on my list if the time turns up. If someone else has the time and an interest in cleaning up the bignum library, I'd gladly discuss the mechanics with them. > > 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. Well if you need to add bn_fix_top()s to BN_cmp(), then either; (a) you are modifying bignum structs directly, or (b) you are using something in the BIGNUM API that leaves structures in an invalid state. Of course, it is possible the reason is both (a)+(b) if you are using a macro declared in the BIGNUM API, but as a macro can have debugging added to it just as a function can that doesn't really change my point. In short, it *is* dependant on "the exit path taken", because at some point outside any API call, there are bignum structures in an invalid state. Ie. *that* is the bug, not the fact that BN_cmp() chokes on those values later on. Cheers, Geoff -- Geoff Thorpe [EMAIL PROTECTED] http://www.geoffthorpe.net/ ______________________________________________________________________ OpenSSL Project http://www.openssl.org Development Mailing List [EMAIL PROTECTED] Automated List Manager [EMAIL PROTECTED]
