From: Theo Buehler <tb@theobuehler.org>
Subject: rpki-client: minimal .gbr vCard sanity check + safer printing
To: tech@openbsd.org
Date: Tue, 1 Apr 2025 13:12:18 +0200

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 {} }
        }
      }

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.

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).

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);
+	(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);
 		}
 	}