From 797cceee728f53256e7696af4456493737a92932 Mon Sep 17 00:00:00 2001 From: tb Date: Tue, 21 Feb 2023 10:18:47 +0000 Subject: [PATCH] rpki-client: ensure there is no trailing garbage in signed objects The d2i functions are designed in such a way that the caller is responsible to check if the entire buffer was consumed. Add checks on deserializing a signed object to ensure the entire file has been consumed. Reject the file if it has trailing garbage. found by & ok job, ok claudio --- usr.sbin/rpki-client/cert.c | 22 ++++++++++++++-------- usr.sbin/rpki-client/cms.c | 12 +++++++++--- usr.sbin/rpki-client/crl.c | 14 ++++++++++---- 3 files changed, 33 insertions(+), 15 deletions(-) diff --git a/usr.sbin/rpki-client/cert.c b/usr.sbin/rpki-client/cert.c index 1f1348c16c3..1a749116abe 100644 --- a/usr.sbin/rpki-client/cert.c +++ b/usr.sbin/rpki-client/cert.c @@ -1,4 +1,4 @@ -/* $OpenBSD: cert.c,v 1.101 2022/11/30 09:12:34 job Exp $ */ +/* $OpenBSD: cert.c,v 1.102 2023/02/21 10:18:47 tb Exp $ */ /* * Copyright (c) 2022 Theo Buehler * Copyright (c) 2021 Job Snijders @@ -641,13 +641,14 @@ cert_parse_ee_cert(const char *fn, X509 *x) 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,10 +659,15 @@ cert_parse_pre(const char *fn, const unsigned char *der, size_t len) 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; + } /* Cache X509v3 extensions, see X509_check_ca(3). */ if (X509_check_purpose(x, -1, -1) <= 0) { diff --git a/usr.sbin/rpki-client/cms.c b/usr.sbin/rpki-client/cms.c index 1b2e6998885..79f6dbf91de 100644 --- a/usr.sbin/rpki-client/cms.c +++ b/usr.sbin/rpki-client/cms.c @@ -1,4 +1,4 @@ -/* $OpenBSD: cms.c,v 1.26 2022/12/28 21:30:18 jmc Exp $ */ +/* $OpenBSD: cms.c,v 1.27 2023/02/21 10:18:47 tb Exp $ */ /* * Copyright (c) 2019 Kristaps Dzonsons * @@ -64,9 +64,10 @@ cms_extract_econtent(const char *fn, CMS_ContentInfo *cms, unsigned char **res, 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,10 +90,15 @@ cms_parse_validate_internal(X509 **xp, const char *fn, const unsigned char *der, 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; + } /* * The CMS is self-signed with a signing certificate. diff --git a/usr.sbin/rpki-client/crl.c b/usr.sbin/rpki-client/crl.c index f5f7be669e3..da4eebfe7f7 100644 --- a/usr.sbin/rpki-client/crl.c +++ b/usr.sbin/rpki-client/crl.c @@ -1,4 +1,4 @@ -/* $OpenBSD: crl.c,v 1.21 2022/11/30 09:03:44 job Exp $ */ +/* $OpenBSD: crl.c,v 1.22 2023/02/21 10:18:47 tb Exp $ */ /* * Copyright (c) 2019 Kristaps Dzonsons * @@ -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,10 +37,15 @@ crl_parse(const char *fn, const unsigned char *der, size_t len) 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; + } if ((crl->aki = x509_crl_get_aki(crl->x509_crl, fn)) == NULL) { warnx("x509_crl_get_aki failed"); -- 2.20.1