From: Claudio Jeker Subject: Re: rpki-client: minimal .gbr vCard sanity check + safer printing To: Theo Buehler Cc: tech@openbsd.org Date: Tue, 1 Apr 2025 14:02:59 +0200 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 > #include > #include > +#include > > #include > > @@ -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 > #include > #include > +#include > > #include > #include > @@ -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