On 2/8/06, Nils Larsch wrote:
> David Hartman wrote:
> >>>Index: crypto/bio/b_print.c
> > >>>retrieving revision 1.1.1.1
> >>>diff -u -b -r1.1.1.1 b_print.c
> >>>--- crypto/bio/b_print.c 30 Aug 2005 19:33:35 -0000 1.1.1.1
> >>>+++ crypto/bio/b_print.c 29 Dec 2005 23:54:55 -0000
> >>>@@ -741,6 +741,7 @@
> >>> *buffer =3D OPENSSL_malloc(*maxlen);
> >>> if (*currlen > 0) {
> >>> assert(*sbuffer !=3D NULL);
> >>
> >>I guess this should be "assert(*buffer !=3D NULL)"

> >
> > [[DSH]]
> > The only problem with assertions is some groups may not compile with
> > assertions for production-level code.

>
> agree, returning an error would be better. I will look at it.


Robustness demands checking of everything passed into a function for
possible failures -- such as an inappropriate NULL pointer.

>
> >>>Index: crypto/ec/ec_lib.c
> >>>=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3 D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3 D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3 D=3D=3D
> >>>RCS file:
> >>

> > /local/cvs/master/pspOpenSSL/Current/source/openssl/crypto/ec/ec_lib.c,=

v
> >
> >>>retrieving revision 1.1.1.1
> >>>diff -u -b -r1.1.1.1 ec_lib.c
> >>>--- crypto/ec/ec_lib.c 30 Aug 2005 19:33:35 -0000 1.1.1.1
> >>>+++ crypto/ec/ec_lib.c 29 Dec 2005 23:55:02 -0000
> >>>@@ -124,7 +124,7 @@
> >>> {
> >>> if (!group) return;
> >>>
> >>>- if (group->meth->group_finish !=3D 0)
> >>>+ if ((group->meth !=3D NULL) && (group->meth->group_finish !=3D 0)=

)
> >>> group->meth->group_finish(group);
> >>
> >>group->meth should never be NULL

> >
> >
> > [[DSH]]
> > The reason this was flagged as a concern is a few lines down, the
> > original code was doing a check to see if group->meth was null. So
> > group->meth->group_clear_finished was being dereferenced, then the chec=

k
> > for a null group->meth was being made. See line 150 of the latest
> > snapshot. I also attached the Coverity output as ec_lib.txt.

>
> agree, the current code is not really consistent here. If we assume
> that EC_GROUP::meth cannot be NULL in a valid EC_GROUP object the
> check for "group->meth !=3D NULL" is superfluous and misleading and
> should be removed. Done.


The check for group->meth !=3D NULL should be above any of the other
code in the function. (Just because 'group' !=3D NULL doesn't mean that
group->meth !=3D NULL.)

>
> >
> >
> >>>Index: crypto/ec/ec_mult.c
> >>>=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3 D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3 D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3 D=3D=3D
> >>>RCS file:
> >>

> > /local/cvs/master/pspOpenSSL/Current/source/openssl/crypto/ec/ec_mult.c=

,
> > v
> >
> >>>retrieving revision 1.1.1.1
> >>>diff -u -b -r1.1.1.1 ec_mult.c
> >>>--- crypto/ec/ec_mult.c 30 Aug 2005 19:33:35 -0000 1.1.1.1
> >>>+++ crypto/ec/ec_mult.c 29 Dec 2005 23:55:02 -0000
> >>>@@ -436,7 +436,19 @@
> >>> {
> >>> size_t bits;
> >>>
> >>>- bits =3D i < num ? BN_num_bits(scalars[i]) :
> >>
> >>BN_num_bits(scalar);
> >>
> >>>+ if (i < num)
> >>>+ {
> >>>+ if (scalars[i] !=3D NULL)
> >>>+ bits =3D BN_num_bits(scalars[i]);
> >>>+ else goto err;
> >>>+ }
> >>
> >>unless the user supplies a NULL pointer for a BIGNUM this
> >>shouldn't be necessary

>
> note: as the user supplied arrary of BIGNUM pointers is not
> delimited by a NULL pointer a check for scalars[i] !=3D NULL
> isn't very useful


Even if the user supplies a NULL pointer for a BIGNUM it should return
an error, not SEGV.

> > Since num is a passed in value, it is possible for it to be a
> > non-zero number, so the for loop could be entered and scaler
> > dereferenced.

>
> however the "i >=3D num" branch could only be reached if "num_scalar !=3D=

0"
> and this could only happen if "scalar !=3D NULL" so the second check
> "scalar !=3D NULL" shouldn't be necessary as well.
>
> > Granted, this is unlikely and would only happen because
> > of the caller giving invalid inputs. I attached the coverity output as
> > ec_mult.txt.

>
> I've backported the changes I've made so far to the 0.9.8 branch,
> so please test a recent snapshot.


Document your assumptions as to what you're going to be passed, and
then verify those assumptions with tests. This is the way to avoid
all these "libcrypto causes segv"-type messages to the list.

-Kyle H
__________________________________________________ ____________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-dev@openssl.org
Automated List Manager majordomo@openssl.org