Re: libcrypto: INTEGER_cmp vs. STRING_cmp
On Wed, Mar 06, 2019 at 11:16:07PM +0100, Holger Mikolon wrote:
>
> > Date: Wed, 6 Mar 2019 06:31:17
> > From: Theo Buehler
>
> (snip)
>
> > 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.
>
> (snip)
>
> I went through the files affected by said commit and focused on INTEGER
> vs. STRING mixup only (mostly related to serialNumber, once related to
> zone). Then I greped through the rest of libcrypto sources and found just
> x_crl.c to have a mixup.
Much better, thanks. Except for the first one, they match what BoringSSL
and OpenSSL are doing.
ok tb
> I did not touch asn1/a_strnid.c, where the serialNumber is listed as
> B_ASN1_PRINTABLESTRING. I don't know enough here, so I better leave
> this for the experts.
I think this should stay as it is.
>
> Holger
>
>
>
> Index: asn1/x_crl.c
> ===
> RCS file: /cvs/src/lib/libcrypto/asn1/x_crl.c,v
> retrieving revision 1.33
> diff -u -p -u -r1.33 x_crl.c
> --- asn1/x_crl.c 24 Aug 2018 19:55:58 - 1.33
> +++ asn1/x_crl.c 6 Mar 2019 21:46:52 -
> @@ -527,9 +527,7 @@ X509_CRL_dup(X509_CRL *x)
> static int
> X509_REVOKED_cmp(const X509_REVOKED * const *a, const X509_REVOKED * const
> *b)
> {
> - return(ASN1_STRING_cmp(
> - (ASN1_STRING *)(*a)->serialNumber,
> - (ASN1_STRING *)(*b)->serialNumber));
> + return(ASN1_INTEGER_cmp((*a)->serialNumber, (*b)->serialNumber));
> }
>
> int
> Index: pkcs7/pk7_doit.c
> ===
> RCS file: /cvs/src/lib/libcrypto/pkcs7/pk7_doit.c,v
> retrieving revision 1.42
> diff -u -p -u -r1.42 pk7_doit.c
> --- pkcs7/pk7_doit.c 2 May 2017 03:59:45 - 1.42
> +++ pkcs7/pk7_doit.c 6 Mar 2019 21:46:52 -
> @@ -410,7 +410,7 @@ pkcs7_cmp_ri(PKCS7_RECIP_INFO *ri, X509
> pcert->cert_info->issuer);
> if (ret)
> return ret;
> - return ASN1_STRING_cmp(pcert->cert_info->serialNumber,
> + return ASN1_INTEGER_cmp(pcert->cert_info->serialNumber,
> ri->issuer_and_serial->serial);
> }
>
> Index: pkcs7/pk7_lib.c
> ===
> RCS file: /cvs/src/lib/libcrypto/pkcs7/pk7_lib.c,v
> retrieving revision 1.19
> diff -u -p -u -r1.19 pk7_lib.c
> --- pkcs7/pk7_lib.c 29 Jan 2017 17:49:23 - 1.19
> +++ pkcs7/pk7_lib.c 6 Mar 2019 21:46:53 -
> @@ -374,7 +374,7 @@ PKCS7_SIGNER_INFO_set(PKCS7_SIGNER_INFO
>* things the ugly way. */
> ASN1_INTEGER_free(p7i->issuer_and_serial->serial);
> if (!(p7i->issuer_and_serial->serial =
> - ASN1_STRING_dup(X509_get_serialNumber(x509
> + ASN1_INTEGER_dup(X509_get_serialNumber(x509
> goto err;
>
> /* lets keep the pkey around for a while */
> @@ -534,7 +534,7 @@ PKCS7_RECIP_INFO_set(PKCS7_RECIP_INFO *p
>
> ASN1_INTEGER_free(p7i->issuer_and_serial->serial);
> if (!(p7i->issuer_and_serial->serial =
> - ASN1_STRING_dup(X509_get_serialNumber(x509
> + ASN1_INTEGER_dup(X509_get_serialNumber(x509
> return 0;
>
> pkey = X509_get_pubkey(x509);
> Index: x509/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/x509_cmp.c 24 Aug 2018 19:59:32 - 1.34
> +++ x509/x509_cmp.c 6 Mar 2019 21:46:53 -
> @@ -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));
> Index: x509v3/v3_sxnet.c
> ===
> RCS file: /cvs/src/lib/libcrypto/x509v3/v3_sxnet.c,v
> retrieving revision 1.21
> diff -u -p -u -r1.21 v3_sxnet.c
> --- x509v3/v3_sxnet.c 13 May 2018 15:03:01 - 1.21
> +++ x509v3/v3_sxnet.c 6 Mar 2019 21:46:53 -
> @@ -376,7 +376,7 @@ SXNET_get_id_INTEGER(SXNET *sx, ASN1_INT
>
> for (i = 0; i < sk_SXNETID_num(sx->ids); i++) {
> id = sk_SXNETID_value(sx->ids, i);
> - if (!ASN1_STRING_cmp(id->zone, zone))
> + if (!ASN1_INTEGER_cmp(id->zone, zone))
> return id->user;
> }
> return NULL;
Re: libcrypto: INTEGER_cmp vs. STRING_cmp
> Date: Wed, 6 Mar 2019 06:31:17
> From: Theo Buehler
(snip)
> 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.
(snip)
I went through the files affected by said commit and focused on INTEGER
vs. STRING mixup only (mostly related to serialNumber, once related to
zone). Then I greped through the rest of libcrypto sources and found just
x_crl.c to have a mixup.
I did not touch asn1/a_strnid.c, where the serialNumber is listed as
B_ASN1_PRINTABLESTRING. I don't know enough here, so I better leave
this for the experts.
Holger
Index: asn1/x_crl.c
===
RCS file: /cvs/src/lib/libcrypto/asn1/x_crl.c,v
retrieving revision 1.33
diff -u -p -u -r1.33 x_crl.c
--- asn1/x_crl.c24 Aug 2018 19:55:58 - 1.33
+++ asn1/x_crl.c6 Mar 2019 21:46:52 -
@@ -527,9 +527,7 @@ X509_CRL_dup(X509_CRL *x)
static int
X509_REVOKED_cmp(const X509_REVOKED * const *a, const X509_REVOKED * const *b)
{
- return(ASN1_STRING_cmp(
- (ASN1_STRING *)(*a)->serialNumber,
- (ASN1_STRING *)(*b)->serialNumber));
+ return(ASN1_INTEGER_cmp((*a)->serialNumber, (*b)->serialNumber));
}
int
Index: pkcs7/pk7_doit.c
===
RCS file: /cvs/src/lib/libcrypto/pkcs7/pk7_doit.c,v
retrieving revision 1.42
diff -u -p -u -r1.42 pk7_doit.c
--- pkcs7/pk7_doit.c2 May 2017 03:59:45 - 1.42
+++ pkcs7/pk7_doit.c6 Mar 2019 21:46:52 -
@@ -410,7 +410,7 @@ pkcs7_cmp_ri(PKCS7_RECIP_INFO *ri, X509
pcert->cert_info->issuer);
if (ret)
return ret;
- return ASN1_STRING_cmp(pcert->cert_info->serialNumber,
+ return ASN1_INTEGER_cmp(pcert->cert_info->serialNumber,
ri->issuer_and_serial->serial);
}
Index: pkcs7/pk7_lib.c
===
RCS file: /cvs/src/lib/libcrypto/pkcs7/pk7_lib.c,v
retrieving revision 1.19
diff -u -p -u -r1.19 pk7_lib.c
--- pkcs7/pk7_lib.c 29 Jan 2017 17:49:23 - 1.19
+++ pkcs7/pk7_lib.c 6 Mar 2019 21:46:53 -
@@ -374,7 +374,7 @@ PKCS7_SIGNER_INFO_set(PKCS7_SIGNER_INFO
* things the ugly way. */
ASN1_INTEGER_free(p7i->issuer_and_serial->serial);
if (!(p7i->issuer_and_serial->serial =
- ASN1_STRING_dup(X509_get_serialNumber(x509
+ ASN1_INTEGER_dup(X509_get_serialNumber(x509
goto err;
/* lets keep the pkey around for a while */
@@ -534,7 +534,7 @@ PKCS7_RECIP_INFO_set(PKCS7_RECIP_INFO *p
ASN1_INTEGER_free(p7i->issuer_and_serial->serial);
if (!(p7i->issuer_and_serial->serial =
- ASN1_STRING_dup(X509_get_serialNumber(x509
+ ASN1_INTEGER_dup(X509_get_serialNumber(x509
return 0;
pkey = X509_get_pubkey(x509);
Index: x509/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/x509_cmp.c 24 Aug 2018 19:59:32 - 1.34
+++ x509/x509_cmp.c 6 Mar 2019 21:46:53 -
@@ -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));
Index: x509v3/v3_sxnet.c
===
RCS file: /cvs/src/lib/libcrypto/x509v3/v3_sxnet.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 v3_sxnet.c
--- x509v3/v3_sxnet.c 13 May 2018 15:03:01 - 1.21
+++ x509v3/v3_sxnet.c 6 Mar 2019 21:46:53 -
@@ -376,7 +376,7 @@ SXNET_get_id_INTEGER(SXNET *sx, ASN1_INT
for (i = 0; i < sk_SXNETID_num(sx->ids); i++) {
id = sk_SXNETID_value(sx->ids, i);
- if (!ASN1_STRING_cmp(id->zone, zone))
+ if (!ASN1_INTEGER_cmp(id->zone, zone))
return id->user;
}
return NULL;
Re: libcrypto: INTEGER_cmp vs. STRING_cmp
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.c24 Aug 2018 19:59:32 - 1.34 > +++ x509_cmp.c5 Mar 2019 22:19:34 - > @@ -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)); >
libcrypto: INTEGER_cmp vs. STRING_cmp
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? 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 - 1.34 +++ x509_cmp.c 5 Mar 2019 22:19:34 - @@ -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));
