On Thu, May 12, 2022 at 07:44:51PM +0200, Theo Buehler wrote:
> ip_addr_parse() sticks its fingers into undocumented API surface of
> libcrypto. What is true is that the unused bit count is in the lower
> three bits of p->flags, provided that ASN1_STRING_FLAG_BITS_LEFT is set.
> This is "documented" in asn1.h
>
> #define ASN1_STRING_FLAG_BITS_LEFT 0x08 /* Set if 0x07 has bits left value */
>
> What the flags beyond 0x08 mean is mostly unspecified. They a priori
> don't have anything to do with the bit string's unused bits, so the
> checks and errors make no sense.
>
> Maybe we should add a check of the form
>
> if (p->flags & ~(ASN1_STRING_FLAG_BITS_LEFT | 0x07)) {
> warnx("%s: unexpected flags set in address bit string", fn);
> return 0;
> }
>
> since equivalent checks have been working so far, but I'm not sure.
> This complains about an implementation detail, not on anything directly
> related to the (mis)parsing of a cert, roa, or ...
Yes, this seems reasonable. Diff is OK claudio@
Not sure about the extra paranoia check. Since unused is now masked with
0x07 extra bits wont hurt anymore.
> Index: ip.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/ip.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 ip.c
> --- ip.c 11 May 2022 18:48:35 -0000 1.22
> +++ ip.c 12 May 2022 11:20:52 -0000
> @@ -189,17 +189,9 @@ ip_addr_parse(const ASN1_BIT_STRING *p,
> /* Weird OpenSSL-ism to get unused bit count. */
>
> if ((p->flags & ASN1_STRING_FLAG_BITS_LEFT))
> - unused = p->flags & ~ASN1_STRING_FLAG_BITS_LEFT;
> + unused = p->flags & 0x07;
>
> - if (unused < 0) {
> - warnx("%s: RFC 3779 section 2.2.3.8: "
> - "unused bit count must be non-negative", fn);
> - return 0;
> - } else if (unused >= 8) {
> - warnx("%s: RFC 3779 section 2.2.3.8: "
> - "unused bit count must mask an unsigned char", fn);
> - return 0;
> - } else if (p->length == 0 && unused != 0) {
> + if (p->length == 0 && unused != 0) {
> warnx("%s: RFC 3779 section 2.2.3.8: "
> "unused bit count must be zero if length is zero", fn);
> return 0;
>
--
:wq Claudio
