Re: [PATCH v3 02/10] dpp-util: add URI parsing

* Re: [PATCH v3 02/10] dpp-util: add URI parsing
@ 2022-02-14 22:52 Denis Kenzior
  0 siblings, 0 replies; 4+ messages in thread
From: Denis Kenzior @ 2022-02-14 22:52 UTC (permalink / raw)
  To: iwd

[-- Attachment #1: Type: text/plain, Size: 6473 bytes --]

Hi James,

On 2/14/22 15:48, James Prestwood wrote:
> Parses K (key), M (mac), C (class/channels), and V (version) tokens
> into a new structure dpp_uri_info. H/I are not parsed since there
> currently isn't any use for them.
> ---
>   src/dpp-util.c | 217 +++++++++++++++++++++++++++++++++++++++++++++++++
>   src/dpp-util.h |  13 +++
>   2 files changed, 230 insertions(+)
> 
> diff --git a/src/dpp-util.c b/src/dpp-util.c
> index 5199d0ae..ce2e1740 100644
> --- a/src/dpp-util.c
> +++ b/src/dpp-util.c
> @@ -866,3 +866,220 @@ struct l_ecc_point *dpp_point_from_asn1(const uint8_t *asn1, size_t len)
>   	return l_ecc_point_from_data(curve, key_data[1],
>   					key_data + 2, elen - 2);
>   }
> +
> +/*
> + * Advances 'p' to the next character 'sep' plus one. strchr can be trusted to
> + * find the next character, but we do need to check that the next character + 1
> + * isn't the NULL terminator, i.e. that data actually exists past this point.
> + */
> +#define TOKEN_NEXT(p, sep) \
> +({ \
> +	const char *_next = strchr((p), (sep)); \
> +	if (_next) { \
> +		if (*(_next + 1) == '\0') \
> +			_next = NULL; \
> +		else \
> +			_next++; \
> +	} \
> +	_next; \
> +})
> +
> +/*
> + * Finds the length of the current token (characters until next 'sep'). If no
> + * 'sep' is found zero is returned.
> + */
> +#define TOKEN_LEN(p, sep) \
> +({ \
> +	const char *_next = strchr((p), (sep)); \
> +	if (!_next) \
> +		_next = (p); \
> +	(_next - (p)); \
> +})
> +
> +/*
> + * Ensures 'p' points to something resembling a single character followed by
> + * ':' followed by at least one non-null byte of data. This allows the parse
> + * loop to safely advance the pointer to each tokens data (pos + 2)
> + */
> +#define TOKEN_OK(p) \
> +	((p) && (p)[0] != '\0' && (p)[1] == ':' && (p)[2] != '\0') \
> +
> +static struct scan_freq_set *dpp_parse_class_and_channel(const char *token)
> +{
> +	const char *pos = token;
> +	char *end;
> +	struct scan_freq_set *freqs = scan_freq_set_new();
> +
> +	/* Checking for <operclass>/<channel>,<operclass>/<channel>,... */
> +	for (; pos; pos = TOKEN_NEXT(pos, ',')) {
> +		uint8_t channel;
> +		uint8_t oper_class = strtol(pos, &end, 10);

strtoul?  Also, might want to check for the minus sign and make sure that strtol 
isn't returning something larger > 255.

> +
> +		if (!end || end == pos || (end && *end != '/'))
> +			goto free_set;
> +
> +		pos = end + 1;
> +		channel = strtol(pos, &end, 10);
> +
> +		/* Either another pair (,) or end of this token (;) */
> +		if (!end || end == pos || (*end != ',' && *end != ';'))
> +			goto free_set;
> +
> +		scan_freq_set_add(freqs, oci_to_frequency(oper_class, channel));
> +	}
> +
> +	if (scan_freq_set_isempty(freqs)) {
> +free_set:
> +		scan_freq_set_free(freqs);
> +		return NULL;
> +	}
> +
> +	return freqs;
> +}
> +
> +static int dpp_parse_mac(const char *str, uint8_t *mac_out)
> +{
> +	uint8_t mac[6];
> +	unsigned int i;
> +
> +	for (i = 0; i < 12; i += 2) {
> +		if (!l_ascii_isxdigit(str[i]))
> +			return -EINVAL;
> +
> +		if (!l_ascii_isxdigit(str[i + 1]))
> +			return -EINVAL;
> +	}
> +
> +	if (sscanf(str, "%2hhx%2hhx%2hhx%2hhx%2hhx%2hhx",
> +			&mac[0], &mac[1], &mac[2],
> +			&mac[3], &mac[4], &mac[5]) != 6)
> +		return -EINVAL;
> +
> +	if (!util_is_valid_sta_address(mac))
> +		return -EINVAL;
> +
> +	memcpy(mac_out, mac, 6);
> +
> +	return 0;
> +}
> +
> +static int dpp_parse_version(const char *str, uint8_t *version_out)
> +{
> +	char *end;
> +	uint8_t version;
> +	size_t len = TOKEN_LEN(str, ';');
> +
> +	if (len != 1)
> +		return -EINVAL;
> +
> +	version = strtol(str, &end, 10);
> +
> +	if (version != 1 && version != 2)
> +		return -EINVAL;
> +
> +	*version_out = version;
> +
> +	return 0;
> +}
> +
> +static struct l_ecc_point *dpp_parse_key(const char *str)
> +{
> +	_auto_(l_free) uint8_t *decoded = NULL;
> +	size_t decoded_len;
> +	size_t len = TOKEN_LEN(str, ';');
> +
> +	if (!len)
> +		return NULL;
> +
> +	decoded = l_base64_decode(str, len, &decoded_len);
> +	if (!decoded)
> +		return NULL;
> +
> +	return dpp_point_from_asn1(decoded, decoded_len);
> +}
> +
> +/*
> + * Parse a bootstrapping URI. This parses the tokens defined in the Easy Connect
> + * spec, and verifies they are the correct syntax. Some values have extra
> + * verification:
> + *  - The bootstrapping key is base64 decoded and converted to an l_ecc_point
> + *  - The operating class and channels are checked against the OCI table.
> + *  - The version is checked to be either 1 or 2, as defined by the spec.
> + *  - The MAC is verified to be a valid station address.
> + */
> +struct dpp_uri_info *dpp_parse_uri(const char *uri)
> +{
> +	struct dpp_uri_info *info;
> +	const char *pos = uri;
> +	const char *end = uri + strlen(uri) - 1;
> +	int ret = 0;
> +
> +	if (strncmp(pos, "DPP:", 4))
> +		return NULL;

strncmp?  If the string has already been validated to be a valid string, then 
strcmp is probably just fine.

> +
> +	info = l_new(struct dpp_uri_info, 1);
> +
> +	pos += 4;
> +
> +	/* EasyConnect 5.2.1 - Bootstrapping information format */
> +	for (; TOKEN_OK(pos); pos = TOKEN_NEXT(pos, ';')) {

You don't seem to be validating that the token is terminated by ';'?

> +		switch (*pos) {
> +		case 'C':
> +			info->freqs = dpp_parse_class_and_channel(pos + 2);
> +			if (!info->freqs)
> +				goto free_info;
> +			break;
> +		case 'M':
> +			ret = dpp_parse_mac(pos + 2, info->mac);
> +			if (ret < 0)
> +				goto free_info;

This doesn't seem to be checking that parse_mac consumes the entire token?

> +			break;
> +		case 'V':
> +			ret = dpp_parse_version(pos + 2, &info->version);
> +			if (ret < 0)
> +				goto free_info;

This does...

> +			break;
> +		case 'K':
> +			info->boot_public = dpp_parse_key(pos + 2);
> +			if (!info->boot_public)
> +				goto free_info;

Unclear here?

> +			break;
> +		case 'H':
> +		case 'I':
> +			break;
> +		default:
> +			goto free_info;
> +		}
> +	}
> +
> +	/* Extra data found after last token */
> +	if (pos != end)
> +		goto free_info;
> +
> +	/* The public bootstrapping key is the only required token */
> +	if (!info->boot_public)
> +		goto free_info;
> +
> +	return info;
> +
> +free_info:
> +	dpp_free_uri_info(info);
> +	return NULL;
> +}
> +

Regards,
-Denis

^ permalink raw reply	[flat|nested] 4+ messages in thread