From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: rpki-client: convert mft.c to opaque ASN1_STRING
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Mon, 1 Dec 2025 16:30:29 +0100

On Mon, Dec 01, 2025 at 04:09:37PM +0100, Theo Buehler wrote:
> First, more of the same. I'm not sure I like the data/length reuse
> in mft_parse_filehash(), but using fdata/flength and hdata/hlength
> or similar didn't seem to make things any better.
> 
> I added an XXX for a future check to avoid the possibility of someone
> fiddling with the unused bits octet if the hash ends in a few 0 bits.
> Tihs will be easier once we have API and I'll have audit the other bit
> strings anyway.
> 
> For the two comparison functions, we can use ASN1_STRING_cmp()
> for simplicity. If length and data compare equal, this adds a type check,
> which is fine since the template ensures that we compare two IA5 or
> two bit strings. The assert can be dropped since the uniequeness check
> comes after the length check in mft_parse_filehash().

This looks fine. I don't really want to understand the horror of the
unused bits octet but doesn't the current code have the same issue?

OK claudio@

 
> Index: mft.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
> diff -u -p -r1.133 mft.c
> --- mft.c	13 Nov 2025 15:18:53 -0000	1.133
> +++ mft.c	1 Dec 2025 14:55:13 -0000
> @@ -149,23 +149,31 @@ static int
>  mft_parse_filehash(const char *fn, struct mft *mft, const FileAndHash *fh,
>      int *found_crl)
>  {
> +	const unsigned char	*data;
>  	char			*file = NULL;
> -	int			 rc = 0;
> +	int			 length, rc = 0;
>  	struct mftfile		*fent;
>  	enum rtype		 type;
>  	size_t			 new_idx = 0;
>  
> -	if (!valid_mft_filename(fh->file->data, fh->file->length)) {
> +	data = ASN1_STRING_get0_data(fh->file);
> +	length = ASN1_STRING_length(fh->file);
> +
> +	if (!valid_mft_filename(data, length)) {
>  		warnx("%s: RFC 9286 section 4.2.2: bad filename", fn);
>  		goto out;
>  	}
> -	file = strndup(fh->file->data, fh->file->length);
> +	file = strndup(data, length);
>  	if (file == NULL)
>  		err(1, NULL);
>  
> -	if (fh->hash->length != SHA256_DIGEST_LENGTH) {
> +	/* XXX - malleability: ensure unused bits are 0. */
> +	data = ASN1_STRING_get0_data(fh->hash);
> +	length = ASN1_STRING_length(fh->hash);
> +
> +	if (length != SHA256_DIGEST_LENGTH) {
>  		warnx("%s: RFC 9286 section 4.2.1: hash: "
> -		    "invalid SHA256 length, have %d", fn, fh->hash->length);
> +		    "invalid SHA256 length, have %d", fn, length);
>  		goto out;
>  	}
>  
> @@ -181,7 +189,7 @@ mft_parse_filehash(const char *fn, struc
>  			goto out;
>  		}
>  		/* remember the filehash for the CRL in struct mft */
> -		memcpy(mft->crlhash, fh->hash->data, SHA256_DIGEST_LENGTH);
> +		memcpy(mft->crlhash, data, length);
>  		*found_crl = 1;
>  	}
>  
> @@ -197,7 +205,7 @@ mft_parse_filehash(const char *fn, struc
>  	fent->type = type;
>  	fent->file = file;
>  	file = NULL;
> -	memcpy(fent->hash, fh->hash->data, SHA256_DIGEST_LENGTH);
> +	memcpy(fent->hash, data, length);
>  
>  	rc = 1;
>   out:
> @@ -208,21 +216,13 @@ mft_parse_filehash(const char *fn, struc
>  static int
>  mft_fh_cmp_name(const FileAndHash *const *a, const FileAndHash *const *b)
>  {
> -	if ((*a)->file->length < (*b)->file->length)
> -		return -1;
> -	if ((*a)->file->length > (*b)->file->length)
> -		return 1;
> -
> -	return memcmp((*a)->file->data, (*b)->file->data, (*b)->file->length);
> +	return ASN1_STRING_cmp((*a)->file, (*b)->file);
>  }
>  
>  static int
>  mft_fh_cmp_hash(const FileAndHash *const *a, const FileAndHash *const *b)
>  {
> -	assert((*a)->hash->length == SHA256_DIGEST_LENGTH);
> -	assert((*b)->hash->length == SHA256_DIGEST_LENGTH);
> -
> -	return memcmp((*a)->hash->data, (*b)->hash->data, (*b)->hash->length);
> +	return ASN1_STRING_cmp((*a)->hash, (*b)->hash);
>  }
>  
>  /*
> @@ -247,7 +247,8 @@ mft_has_unique_names_and_hashes(const ch
>  
>  		if (mft_fh_cmp_name(&curr, &next) == 0) {
>  			warnx("%s: duplicate name: %.*s", fn,
> -			    curr->file->length, curr->file->data);
> +			    ASN1_STRING_length(curr->file),
> +			    ASN1_STRING_get0_data(curr->file));
>  			goto err;
>  		}
>  	}
> @@ -261,8 +262,10 @@ mft_has_unique_names_and_hashes(const ch
>  
>  		if (mft_fh_cmp_hash(&curr, &next) == 0) {
>  			warnx("%s: duplicate hash for %.*s and %.*s", fn,
> -			    curr->file->length, curr->file->data,
> -			    next->file->length, next->file->data);
> +			    ASN1_STRING_length(curr->file),
> +			    ASN1_STRING_get0_data(curr->file),
> +			    ASN1_STRING_length(next->file),
> +			    ASN1_STRING_get0_data(next->file));
>  			goto err;
>  		}
>  	}
> 

-- 
:wq Claudio