On Mon, Mar 29, 2021 at 01:22:20PM +0200, Claudio Jeker wrote:
> On Mon, Mar 29, 2021 at 01:19:21PM +0200, Claudio Jeker wrote:
> > On Mon, Mar 29, 2021 at 12:42:02PM +0200, Theo Buehler wrote:
> > > On Mon, Mar 29, 2021 at 10:38:54AM +0200, Claudio Jeker wrote:
> > > > Replace a super strange way to translate some binary blob into a hex
> > > > string.
> > > > The code drops the : from the string but this is fine, the : is just
> > > > visual fluff. I used the same function in the not yet finished RRDP
> > > > codebase and there I don't want the extra ':'.
> > >
> > > Yes, that's much better.
> > >
> > > Another detail that changes is that the hexadecimal digits will now be
> > > lower case. Trading one byte overallocation for a free overflow check
> > > is reasonable. The explicit NUL termination out[i * 2] = '\0' is not
> > > really needed since we calloc, but it's probably fine to be explicit.
> >
> > Yeah, I decided to overallocate by one but have calloc() do the overflow
> > check. Also I flipped to uppercase hex since both Job and you mentioned
> > it. I just prefer the explicit NUL termination, if someone copy pastes the
> > code somewhere else where no calloc() happens then such an implicit
> > side-effect could introduce a bug.
> >
> > > ok tb
> > >
> > > for the diff as it is (also for moving the function to x509.c)
> >
> > I also renamed the function to hex_encode() so it fits with
> > base64_decode() which sits in tal.c. I consider to move those functions
> > plus a soon needed hex_decode() into a new file (encoding.c).
> >
Sounds good.
> And here is the diff I wanted to attach but forgot.
ok
>
> --
> :wq Claudio
>
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.58
> diff -u -p -r1.58 extern.h
> --- extern.h 29 Mar 2021 06:50:44 -0000 1.58
> +++ extern.h 29 Mar 2021 11:09:57 -0000
> @@ -433,6 +433,7 @@ int io_recvfd(int, void *, size_t);
>
> /* X509 helpers. */
>
> +char *hex_encode(const unsigned char *, size_t);
> char *x509_get_aia(X509 *, const char *);
> char *x509_get_aki(X509 *, int, const char *);
> char *x509_get_ski(X509 *, const char *);
> Index: x509.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 x509.c
> --- x509.c 29 Mar 2021 06:50:44 -0000 1.19
> +++ x509.c 29 Mar 2021 11:09:40 -0000
> @@ -30,9 +30,32 @@
> #include "extern.h"
>
> /*
> + * Convert binary buffer of size dsz into an upper-case hex-string.
> + * Returns pointer to the newly allocated string. Function can't fail.
> + */
> +char *
> +hex_encode(const unsigned char *in, size_t insz)
> +{
> + const char hex[] = "0123456789ABCDEF";
> + size_t i;
> + char *out;
> +
> + if ((out = calloc(2, insz + 1)) == NULL)
> + err(1, NULL);
> +
> + for (i = 0; i < insz; i++) {
> + out[i * 2] = hex[in[i] >> 4];
> + out[i * 2 + 1] = hex[in[i] & 0xf];
> + }
> + out[i * 2] = '\0';
> +
> + return out;
> +}
> +
> +/*
> * Parse X509v3 authority key identifier (AKI), RFC 6487 sec. 4.8.3.
> * Returns the AKI or NULL if it could not be parsed.
> - * The AKI is formatted as aa:bb:cc:dd, with each being a hex value.
> + * The AKI is formatted as a hex string.
> */
> char *
> x509_get_aki(X509 *x, int ta, const char *fn)
> @@ -40,8 +63,7 @@ x509_get_aki(X509 *x, int ta, const char
> const unsigned char *d;
> AUTHORITY_KEYID *akid;
> ASN1_OCTET_STRING *os;
> - int i, dsz, crit;
> - char buf[4];
> + int dsz, crit;
> char *res = NULL;
>
> akid = X509_get_ext_d2i(x, NID_authority_key_identifier, &crit, NULL);
> @@ -80,16 +102,7 @@ x509_get_aki(X509 *x, int ta, const char
> goto out;
> }
>
> - /* Make room for [hex1, hex2, ":"]*, NUL. */
> -
> - if ((res = calloc(dsz * 3 + 1, 1)) == NULL)
> - err(1, NULL);
> -
> - for (i = 0; i < dsz; i++) {
> - snprintf(buf, sizeof(buf), "%02X:", d[i]);
> - strlcat(res, buf, dsz * 3 + 1);
> - }
> - res[dsz * 3 - 1] = '\0';
> + res = hex_encode(d, dsz);
> out:
> AUTHORITY_KEYID_free(akid);
> return res;
> @@ -98,15 +111,14 @@ out:
> /*
> * Parse X509v3 subject key identifier (SKI), RFC 6487 sec. 4.8.2.
> * Returns the SKI or NULL if it could not be parsed.
> - * The SKI is formatted as aa:bb:cc:dd, with each being a hex value.
> + * The SKI is formatted as a hex string.
> */
> char *
> x509_get_ski(X509 *x, const char *fn)
> {
> const unsigned char *d;
> ASN1_OCTET_STRING *os;
> - int i, dsz, crit;
> - char buf[4];
> + int dsz, crit;
> char *res = NULL;
>
> os = X509_get_ext_d2i(x, NID_subject_key_identifier, &crit, NULL);
> @@ -130,16 +142,7 @@ x509_get_ski(X509 *x, const char *fn)
> goto out;
> }
>
> - /* Make room for [hex1, hex2, ":"]*, NUL. */
> -
> - if ((res = calloc(dsz * 3 + 1, 1)) == NULL)
> - err(1, NULL);
> -
> - for (i = 0; i < dsz; i++) {
> - snprintf(buf, sizeof(buf), "%02X:", d[i]);
> - strlcat(res, buf, dsz * 3 + 1);
> - }
> - res[dsz * 3 - 1] = '\0';
> + res = hex_encode(d, dsz);
> out:
> ASN1_OCTET_STRING_free(os);
> return res;
> @@ -270,14 +273,19 @@ out:
> return crl;
> }
>
> +/*
> + * Parse X509v3 authority key identifier (AKI) from the CRL.
> + * This is matched against the string from x509_get_ski() above.
> + * Returns the AKI or NULL if it could not be parsed.
> + * The AKI is formatted as a hex string.
> + */
> char *
> x509_crl_get_aki(X509_CRL *crl, const char *fn)
> {
> const unsigned char *d;
> AUTHORITY_KEYID *akid;
> ASN1_OCTET_STRING *os;
> - int i, dsz, crit;
> - char buf[4];
> + int dsz, crit;
> char *res = NULL;
>
> akid = X509_CRL_get_ext_d2i(crl, NID_authority_key_identifier, &crit,
> @@ -315,16 +323,7 @@ x509_crl_get_aki(X509_CRL *crl, const ch
> goto out;
> }
>
> - /* Make room for [hex1, hex2, ":"]*, NUL. */
> -
> - if ((res = calloc(dsz * 3 + 1, 1)) == NULL)
> - err(1, NULL);
> -
> - for (i = 0; i < dsz; i++) {
> - snprintf(buf, sizeof(buf), "%02X:", d[i]);
> - strlcat(res, buf, dsz * 3 + 1);
> - }
> - res[dsz * 3 - 1] = '\0';
> + res = hex_encode(d, dsz);
> out:
> AUTHORITY_KEYID_free(akid);
> return res;
