subject:"rpki\-client\: disallow trailing garbage in signed objects"

Re: rpki-client: disallow trailing garbage in signed objects

2023-02-21 Thread Claudio Jeker

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, , 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, , derlen)) == NULL) {
> + oder = der;
> + if ((cms = d2i_CMS_ContentInfo(NULL, , 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, , 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

2023-02-21 Thread Theo Buehler

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

2023-02-20 Thread Job Snijders

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?

rpki-client: disallow trailing garbage in signed objects

2023-02-20 Thread Theo Buehler

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, , 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, , derlen)) == NULL) {
+   oder = der;
+   if ((cms = d2i_CMS_ContentInfo(NULL, , 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, , 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

Re: rpki-client: disallow trailing garbage in signed objects

Re: rpki-client: disallow trailing garbage in signed objects

rpki-client: disallow trailing garbage in signed objects

4 matches

Site Navigation

Mail list logo

Footer information