rpki-client: disallow trailing garbage in signed objects
By design of d2i, it's the caller's responsibility to check a DER object has been fully consumed. We read files from the disk, check hashes, parse and validate the DER we encounter, but we do not make sure that nothing follows the DER blob we parsed. As Job noticed, it is possible to append data to a CRL and still have a manifest display "Validation: OK" in file mode. This is partly possible due to the fact that filemode has a rather lax notion of validity (since it is an inspection tool), but also due to these missing checks. The diff below checks for !=. Barring bugs in ASN1_item_d2i() (unheard of!), only the < case should be possible, but it seems better to allow for > as well. I guess we could assert <=. Index: cert.c === RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v retrieving revision 1.101 diff -u -p -r1.101 cert.c --- cert.c 30 Nov 2022 09:12:34 - 1.101 +++ cert.c 21 Feb 2023 01:48:00 - @@ -641,13 +641,14 @@ cert_parse_ee_cert(const char *fn, X509 struct cert * cert_parse_pre(const char *fn, const unsigned char *der, size_t len) { - int extsz; - int sia_present = 0; - size_t i; - X509*x = NULL; - X509_EXTENSION *ext = NULL; - ASN1_OBJECT *obj; - struct parse p; + const unsigned char *oder; + int extsz; + int sia_present = 0; + size_t i; + X509*x = NULL; + X509_EXTENSION *ext = NULL; + ASN1_OBJECT *obj; + struct parse p; /* just fail for empty buffers, the warning was printed elsewhere */ if (der == NULL) @@ -658,8 +659,13 @@ cert_parse_pre(const char *fn, const uns if ((p.res = calloc(1, sizeof(struct cert))) == NULL) err(1, NULL); + oder = der; if ((x = d2i_X509(NULL, &der, len)) == NULL) { cryptowarnx("%s: d2i_X509", p.fn); + goto out; + } + if (der != oder + len) { + warnx("%s: %td bytes trailing garbage", fn, oder + len - der); goto out; } Index: cms.c === RCS file: /cvs/src/usr.sbin/rpki-client/cms.c,v retrieving revision 1.26 diff -u -p -r1.26 cms.c --- cms.c 28 Dec 2022 21:30:18 - 1.26 +++ cms.c 21 Feb 2023 01:45:37 - @@ -64,9 +64,10 @@ cms_extract_econtent(const char *fn, CMS static int cms_parse_validate_internal(X509 **xp, const char *fn, const unsigned char *der, -size_t derlen, const ASN1_OBJECT *oid, BIO *bio, unsigned char **res, +size_t len, const ASN1_OBJECT *oid, BIO *bio, unsigned char **res, size_t *rsz) { + const unsigned char *oder; char buf[128], obuf[128]; const ASN1_OBJECT *obj, *octype; ASN1_OCTET_STRING *kid = NULL; @@ -89,8 +90,13 @@ cms_parse_validate_internal(X509 **xp, c if (der == NULL) return 0; - if ((cms = d2i_CMS_ContentInfo(NULL, &der, derlen)) == NULL) { + oder = der; + if ((cms = d2i_CMS_ContentInfo(NULL, &der, len)) == NULL) { cryptowarnx("%s: RFC 6488: failed CMS parse", fn); + goto out; + } + if (der != oder + len) { + warnx("%s: %td bytes trailing garbage", fn, oder + len - der); goto out; } Index: crl.c === RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v retrieving revision 1.21 diff -u -p -r1.21 crl.c --- crl.c 30 Nov 2022 09:03:44 - 1.21 +++ crl.c 21 Feb 2023 01:47:31 - @@ -25,9 +25,10 @@ struct crl * crl_parse(const char *fn, const unsigned char *der, size_t len) { - struct crl *crl; - const ASN1_TIME *at; - int rc = 0; + const unsigned char *oder; + struct crl *crl; + const ASN1_TIME *at; + int rc = 0; /* just fail for empty buffers, the warning was printed elsewhere */ if (der == NULL) @@ -36,8 +37,13 @@ crl_parse(const char *fn, const unsigned if ((crl = calloc(1, sizeof(*crl))) == NULL) err(1, NULL); + oder = der; if ((crl->x509_crl = d2i_X509_CRL(NULL, &der, len)) == NULL) { cryptowarnx("%s: d2i_X509_CRL", fn); + goto out; + } + if (der != oder + len) { + warnx("%s: %td bytes trailing garbage", fn, oder + len - der); goto out; }
Re: rpki-client: disallow trailing garbage in signed objects
On Tue, Feb 21, 2023 at 03:07:00AM +0100, Theo Buehler wrote: > By design of d2i, it's the caller's responsibility to check a DER object > has been fully consumed. We read files from the disk, check hashes, > parse and validate the DER we encounter, but we do not make sure that > nothing follows the DER blob we parsed. > > As Job noticed, it is possible to append data to a CRL and still have > a manifest display "Validation: OK" in file mode. This is partly > possible due to the fact that filemode has a rather lax notion of > validity (since it is an inspection tool), but also due to these > missing checks. > > The diff below checks for !=. Barring bugs in ASN1_item_d2i() (unheard > of!), only the < case should be possible, but it seems better to allow > for > as well. I guess we could assert <=. OK job@ ps. If there are 'bytes trailing garbage' on an *.mft discovered in the DIR_VALID storage area, would a more pristine version of the MFT in DIR_TEMP be ignored?
Re: rpki-client: disallow trailing garbage in signed objects
On Tue, Feb 21, 2023 at 02:51:09AM +, Job Snijders wrote: > ps. If there are 'bytes trailing garbage' on an *.mft discovered in the > DIR_VALID storage area, would a more pristine version of the MFT in > DIR_TEMP be ignored? Yes. The whole point of the complicated dance in proc_parser_mft() is to try to fish a valid mft out of either DIR_VALID or DIR_TEMP.
Re: rpki-client: disallow trailing garbage in signed objects
On Tue, Feb 21, 2023 at 03:07:00AM +0100, Theo Buehler wrote: > By design of d2i, it's the caller's responsibility to check a DER object > has been fully consumed. We read files from the disk, check hashes, > parse and validate the DER we encounter, but we do not make sure that > nothing follows the DER blob we parsed. > > As Job noticed, it is possible to append data to a CRL and still have a > manifest display "Validation: OK" in file mode. This is partly possible > due to the fact that filemode has a rather lax notion of validity (since > it is an inspection tool), but also due to these missing checks. > > The diff below checks for !=. Barring bugs in ASN1_item_d2i() (unheard > of!), only the < case should be possible, but it seems better to allow > for > as well. I guess we could assert <=. OK claudio@ > Index: cert.c > === > RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v > retrieving revision 1.101 > diff -u -p -r1.101 cert.c > --- cert.c30 Nov 2022 09:12:34 - 1.101 > +++ cert.c21 Feb 2023 01:48:00 - > @@ -641,13 +641,14 @@ cert_parse_ee_cert(const char *fn, X509 > struct cert * > cert_parse_pre(const char *fn, const unsigned char *der, size_t len) > { > - int extsz; > - int sia_present = 0; > - size_t i; > - X509*x = NULL; > - X509_EXTENSION *ext = NULL; > - ASN1_OBJECT *obj; > - struct parse p; > + const unsigned char *oder; > + int extsz; > + int sia_present = 0; > + size_t i; > + X509*x = NULL; > + X509_EXTENSION *ext = NULL; > + ASN1_OBJECT *obj; > + struct parse p; > > /* just fail for empty buffers, the warning was printed elsewhere */ > if (der == NULL) > @@ -658,8 +659,13 @@ cert_parse_pre(const char *fn, const uns > if ((p.res = calloc(1, sizeof(struct cert))) == NULL) > err(1, NULL); > > + oder = der; > if ((x = d2i_X509(NULL, &der, len)) == NULL) { > cryptowarnx("%s: d2i_X509", p.fn); > + goto out; > + } > + if (der != oder + len) { > + warnx("%s: %td bytes trailing garbage", fn, oder + len - der); > goto out; > } > > Index: cms.c > === > RCS file: /cvs/src/usr.sbin/rpki-client/cms.c,v > retrieving revision 1.26 > diff -u -p -r1.26 cms.c > --- cms.c 28 Dec 2022 21:30:18 - 1.26 > +++ cms.c 21 Feb 2023 01:45:37 - > @@ -64,9 +64,10 @@ cms_extract_econtent(const char *fn, CMS > > static int > cms_parse_validate_internal(X509 **xp, const char *fn, const unsigned char > *der, > -size_t derlen, const ASN1_OBJECT *oid, BIO *bio, unsigned char **res, > +size_t len, const ASN1_OBJECT *oid, BIO *bio, unsigned char **res, > size_t *rsz) > { > + const unsigned char *oder; > char buf[128], obuf[128]; > const ASN1_OBJECT *obj, *octype; > ASN1_OCTET_STRING *kid = NULL; > @@ -89,8 +90,13 @@ cms_parse_validate_internal(X509 **xp, c > if (der == NULL) > return 0; > > - if ((cms = d2i_CMS_ContentInfo(NULL, &der, derlen)) == NULL) { > + oder = der; > + if ((cms = d2i_CMS_ContentInfo(NULL, &der, len)) == NULL) { > cryptowarnx("%s: RFC 6488: failed CMS parse", fn); > + goto out; > + } > + if (der != oder + len) { > + warnx("%s: %td bytes trailing garbage", fn, oder + len - der); > goto out; > } > > Index: crl.c > === > RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v > retrieving revision 1.21 > diff -u -p -r1.21 crl.c > --- crl.c 30 Nov 2022 09:03:44 - 1.21 > +++ crl.c 21 Feb 2023 01:47:31 - > @@ -25,9 +25,10 @@ > struct crl * > crl_parse(const char *fn, const unsigned char *der, size_t len) > { > - struct crl *crl; > - const ASN1_TIME *at; > - int rc = 0; > + const unsigned char *oder; > + struct crl *crl; > + const ASN1_TIME *at; > + int rc = 0; > > /* just fail for empty buffers, the warning was printed elsewhere */ > if (der == NULL) > @@ -36,8 +37,13 @@ crl_parse(const char *fn, const unsigned > if ((crl = calloc(1, sizeof(*crl))) == NULL) > err(1, NULL); > > + oder = der; > if ((crl->x509_crl = d2i_X509_CRL(NULL, &der, len)) == NULL) { > cryptowarnx("%s: d2i_X509_CRL", fn); > + goto out; > + } > + if (der != oder + len) { > + warnx("%s: %td bytes trailing garbage", fn, oder + len - der); > goto out;