On Tue, Mar 05, 2019 at 11:39:02PM +0100, Holger Mikolon wrote: > Hi, > > while debugging an unusual openssl use case, I tried reading and > understanding libcrypto x509 code and came across the comparison > of serialNumbers (of type ASN1_INTEGER*) with a string comparison > function. Below patch fixes the comparison to use ASN1_INTEGER_cmp. > > The man page (ASN1_STRING_cmp(3)) contains the following unambiguous > advice: > > "These functions should not be used to examine or modify ASN1_INTEGER > or ASN1_ENUMERATED types: the relevant INTEGER or ENUMERATED utility > functions should be used instead." > > Revision 1.26 introduced the use of ASN1_STRING_cmp for the serialNumber > with the commit message "Expand obsolete M_ASN1.*(cmp|dup|print|set) > macros ..." So it seems to have been an intentional change, even though > it contradicts the man page. > > Thoughts?
The change definitely was intentional, as it was a strictly mechanical macro expansion. While your patch is correct, I think it is incomplete. If you grep for serialNumber, you'll find a few more STRING vs. INTEGER mixups. I would prefer to address them all at the same time. If you're up for it, it would probably be a good idea to look at the changes introduced by the commit you mentioned and see what else looks suspicious and needs fixing. > > Best regards > Holger > > > > Index: x509_cmp.c > =================================================================== > RCS file: /cvs/src/lib/libcrypto/x509/x509_cmp.c,v > retrieving revision 1.34 > diff -u -p -u -r1.34 x509_cmp.c > --- x509_cmp.c 24 Aug 2018 19:59:32 -0000 1.34 > +++ x509_cmp.c 5 Mar 2019 22:19:34 -0000 > @@ -76,7 +76,7 @@ X509_issuer_and_serial_cmp(const X509 *a > > ai = a->cert_info; > bi = b->cert_info; > - i = ASN1_STRING_cmp(ai->serialNumber, bi->serialNumber); > + i = ASN1_INTEGER_cmp(ai->serialNumber, bi->serialNumber); > if (i) > return (i); > return (X509_NAME_cmp(ai->issuer, bi->issuer)); >
