[PATCH] Add support for MS Catalog files

[Jeremy Allison]

A few inline comments.

Jeremy.

On Thu, Jun 21, 2018 at 06:05:28PM +0200, Andreas Schneider via samba-technical wrote:

> +
> +static char *mscat_asn1_get_oid(TALLOC_CTX *mem_ctx,
> +				asn1_node root,
> +				const char *oid_name)
> +{
> +	char error_string[ASN1_MAX_ERROR_DESCRIPTION_SIZE] = {0};
> +	char oid_str[32] = {0};
> +	int oid_len = sizeof(oid_str);
> +	int rc;
> +
> +	rc = asn1_read_value(root,
> +			     oid_name,
> +			     oid_str,
> +			     &oid_len);

Should there be checks here that oid_len < sizeof(oid_str) ?

What about null termination ?

> +	if (rc != ASN1_SUCCESS) {
> +		asn1_perror(rc);
> +		fprintf(stderr,
> +			"Failed to read value '%s': %s\n",
> +			oid_name,
> +			error_string);
> +		return NULL;
> +	}
> +
> +	return talloc_strdup(mem_ctx, oid_str);
> +}
> +
> +static bool mscat_asn1_oid_equal(const char *o1, const char *o2)
> +{
> +	int cmp;
> +
> +	cmp = strcmp(o1, o2);
> +	if (cmp != 0) {
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static int mscat_asn1_read_value(TALLOC_CTX *mem_ctx,
> +				 asn1_node root,
> +				 const char *name,
> +				 DATA_BLOB *blob)
> +{
> +	DATA_BLOB tmp;
> +	unsigned int etype = ASN1_ETYPE_INVALID;
> +	int len = 0;
> +	int rc;
> +
> +	rc = asn1_read_value_type(root, name, NULL, &len, &etype);
> +	if (rc != ASN1_SUCCESS && len == 0) {
> +		return rc;
> +	}
> +
> +	if (etype == ASN1_ETYPE_BIT_STRING) {

Integer wrap on len ?

> +		len = (len + 7) / 8;
> +	}
> +
> +	if (len == 0) {
> +		*blob = data_blob_null;
> +		return 0;
> +	}
> +
> +	tmp = data_blob_talloc_zero(mem_ctx, len + 1);

Integer wrap on len ?

> +	if (tmp.data == NULL) {
> +		return -1;
> +	}
> +
> +	rc = asn1_read_value(root,
> +			     name,
> +			     tmp.data,
> +			     &len);
> +	if (rc != ASN1_SUCCESS) {
> +		data_blob_free(&tmp);
> +		return rc;
> +	}
> +
> +	if (etype == ASN1_ETYPE_BIT_STRING) {

Integer wrap on len ?

> +		len = (len + 7) / 8;
> +	}
> +	tmp.length = len;
> +
> +	*blob = tmp;
> +
> +	return 0;
> +}
> +

> +				      el1,
> +				      i + 1);
> +		if (el2 == NULL) {
> +			rc = -1;
> +			goto done;
> +		}
> +
> +		DBG_DEBUG("Decode element (startEnd)  %s",
> +			  el2);
> +
> +		rc = asn1_der_decoding_startEnd(ctl->tree_ctl,
> +						ctl->raw_ctl.data,
> +						ctl->raw_ctl.size,
> +						el2,
> +						&content_start,
> +						&content_end);
> +		if (rc != ASN1_SUCCESS) {
> +			goto done;
> +		}
> +		content_len = content_end - content_start + 1;

Arithmetic checks on values read from the file please.