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.c 30 Nov 2022 09:12:34 -0000 1.101
> +++ cert.c 21 Feb 2023 01:48:00 -0000
> @@ -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 -0000 1.26
> +++ cms.c 21 Feb 2023 01:45:37 -0000
> @@ -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 -0000 1.21
> +++ crl.c 21 Feb 2023 01:47:31 -0000
> @@ -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;
> }
>
>
--
:wq Claudio
