On Tue, Apr 01, 2025 at 01:12:18PM +0200, Theo Buehler wrote:
> Job noticed a while back that one of the two .gbr in the ecosystem is
> malformed. Worse, it makes filemode error out in json_do_string() when
> dumping JSON because its vCard is an empty octet string (which means it
> is the byte string 0x04 0x00, so it contains a control character):
> 
>       SEQUENCE {
>         # rpkiGhostbusters
>         OBJECT_IDENTIFIER { 1.2.840.113549.1.9.16.1.35 }
>         [0] {
>           OCTET_STRING { OCTET_STRING {} }
>         }
>       }

What? OCTET_STRING { OCTET_STRING {} } now that is 'cute'.
 
> A valid vCard contains a printable UTF-8 string and looks like this:
> 
>       SEQUENCE {
>         # rpkiGhostbusters
>         OBJECT_IDENTIFIER { 1.2.840.113549.1.9.16.1.35 }
>         [0] {
>           OCTET_STRING { "BEGIN:VCARD\r\nVERSION:4.0\r\n..." }
>         }
>       }
> 
> Now vCards are crazy complicated (despite all the simplifications in
> RFC 6893) because the format is overly flexible and they are defined
> to contain a subset of UTF-8 (for which no sane portable C API exists).
> 
> The problem really is that we dump the literal p->vcard as contained in
> the signed object to stdout/json.
> 
> Long story short: let's add a simple sanity check for the vCard and use
> strvisx() to encode the blob, so it is safe for printing. We can't use
> stravis() because the things we pass in aren't necessarily C strings.
> Portable provides strvisx() in vis.c.

Yes, we decided to not parse the vcard since that is madness. Instead we
just dump it out. I dislike the use of strvis in this case since we want
proper UTF-8 encoding to survive. But right now I think this is the right
fix for release. We may need some way to tweak the output to allow UTF-8
but no other non-printable chars.
 
> A similar problem exists for the comment in .tak objects. Also, there
> should not be an empty line between the comment and the URIs.
> 
> I have code that validates this stuff more thoroughly but that's a lot
> more complicated than what should land at this point in the cycle
> (perhaps it should not land at all).

I think you should commit these as 3 individual commits.
One bikeshed to consider below.
The diffs are OK claudio@
 
> Index: gbr.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/gbr.c,v
> diff -u -p -r1.30 gbr.c
> --- gbr.c	21 Feb 2024 09:17:06 -0000	1.30
> +++ gbr.c	1 Apr 2025 10:32:25 -0000
> @@ -19,6 +19,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include <vis.h>
>  
>  #include <openssl/x509.h>
>  
> @@ -26,6 +27,9 @@
>  
>  extern ASN1_OBJECT	*gbr_oid;
>  
> +#define VCARD_START	"BEGIN:VCARD\r\nVERSION:4.0\r\n"
> +#define VCARD_START_LEN	(sizeof(VCARD_START) - 1)
> +
>  /*
>   * Parse a full RFC 6493 file and signed by the certificate "cacert"
>   * (the latter is optional and may be passed as NULL to disable).
> @@ -48,9 +52,18 @@ gbr_parse(X509 **x509, const char *fn, i
>  	if ((gbr = calloc(1, sizeof(*gbr))) == NULL)
>  		err(1, NULL);
>  	gbr->signtime = signtime;
> -	if ((gbr->vcard = strndup(cms, cmsz)) == NULL)
> +
> +	if (cmsz < VCARD_START_LEN ||
> +	    strncmp(cms, VCARD_START, VCARD_START_LEN) != 0) {
> +		warnx("%s: Ghostbusters record with invalid vCard", fn);
> +		goto out;
> +	}
> +	if ((gbr->vcard = calloc(4, cmsz + 1)) == NULL)
>  		err(1, NULL);

Total bikeshed but wouldn't it be better to use:
	calloc(cmsz + 1, 4)
At least for my brain that makes more sense.

> +	(void)strvisx(gbr->vcard, cms, cmsz, VIS_SAFE);
> +
>  	free(cms);
> +	cms = NULL;
>  
>  	if (!x509_get_aia(*x509, fn, &gbr->aia))
>  		goto out;
> @@ -83,6 +96,7 @@ gbr_parse(X509 **x509, const char *fn, i
>  	return gbr;
>  
>   out:
> +	free(cms);
>  	gbr_free(gbr);
>  	X509_free(*x509);
>  	*x509 = NULL;
> Index: print.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/print.c,v
> diff -u -p -r1.58 print.c
> --- print.c	13 Nov 2024 12:51:04 -0000	1.58
> +++ print.c	1 Apr 2025 10:37:16 -0000
> @@ -795,8 +795,6 @@ takey_print(char *name, const struct tak
>  
>  		for (i = 0; i < t->num_comments; i++)
>  			printf("\t# %s\n", t->comments[i]);
> -		if (t->num_comments > 0)
> -			printf("\n");
>  		for (i = 0; i < t->num_uris; i++)
>  			printf("\t%s\n", t->uris[i]);
>  		printf("\n\t");
> Index: tak.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/tak.c,v
> diff -u -p -r1.21 tak.c
> --- tak.c	13 Nov 2024 12:51:04 -0000	1.21
> +++ tak.c	1 Apr 2025 10:36:50 -0000
> @@ -21,6 +21,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include <vis.h>
>  
>  #include <openssl/asn1.h>
>  #include <openssl/asn1t.h>
> @@ -102,10 +103,11 @@ parse_takey(const char *fn, const TAKey 
>  
>  		for (i = 0; i < res->num_comments; i++) {
>  			comment = sk_ASN1_UTF8STRING_value(takey->comments, i);
> -			res->comments[i] = strndup(comment->data,
> -			    comment->length);
> +			res->comments[i] = calloc(4, comment->length + 1);
>  			if (res->comments[i] == NULL)
>  				err(1, NULL);
> +			(void)strvisx(res->comments[i], comment->data,
> +			    comment->length, VIS_SAFE);
>  		}
>  	}
>  
> 

-- 
:wq Claudio