[PATCH] Add support for MS Catalog files

[Jeremy Allison]

A few more tiny comments inline. Nearly done :-).

On Fri, Jun 22, 2018 at 03:54:49PM +0200, Andreas Schneider wrote:
> +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) {
> +		if (len + 7 < len) {
> +			return -1;
> +		}
> +		len = (len + 7) / 8;
> +	}
> +
> +	if (len == 0) {
> +		*blob = data_blob_null;
> +		return 0;
> +	}
> +
> +	if (len + 1 < len) {
> +		return -1;
> +	}
> +	tmp = data_blob_talloc_zero(mem_ctx, len + 1);
> +	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) {
> +		len = (len + 7) / 8;
> +		if (len + 7 < len) {
> +			return -1;

The check needs to be before the use (as you've
done in the previous case above).

> +
> +static int mscat_ctl_cleanup(struct mscat_ctl *ctl)
> +{
> +	if (ctl->asn1_desc != ASN1_TYPE_EMPTY) {
> +		asn1_delete_structure(&ctl->asn1_desc);
> +	}
> +
> +	return 0;
> +}
> +
> +struct mscat_ctl *mscat_ctl_init(TALLOC_CTX *mem_ctx)
> +{
> +	char error_string[ASN1_MAX_ERROR_DESCRIPTION_SIZE] = {0};
> +	struct mscat_ctl *cat_ctl;
> +	int rc;
> +
> +	cat_ctl = talloc_zero(mem_ctx, struct mscat_ctl);
> +	if (cat_ctl == NULL) {
> +		return NULL;
> +	}
> +	talloc_set_destructor(cat_ctl, mscat_ctl_cleanup);
> +
> +	cat_ctl->asn1_desc = ASN1_TYPE_EMPTY;
> +	cat_ctl->tree_ctl = ASN1_TYPE_EMPTY;
> +
> +	rc = asn1_array2tree(mscat_asn1_tab,
> +			     &cat_ctl->asn1_desc,
> +			     error_string);
> +	if (rc != ASN1_SUCCESS) {
> +		talloc_free(cat_ctl);
> +		asn1_perror(rc);
> +		DBG_ERR("Failed to create parser tree: %s",
> +			error_string);
> +		return NULL;
> +	}
> +
> +	return cat_ctl;
> +}
> +
> +int mscat_ctl_import(struct mscat_ctl *ctl,
> +		     struct mscat_pkcs7 *pkcs7)
> +{
> +	char error_string[ASN1_MAX_ERROR_DESCRIPTION_SIZE] = {0};
> +	TALLOC_CTX *tmp_ctx = NULL;
> +	char *oid;
> +	bool ok;
> +	int rc;
> +
> +	rc = gnutls_pkcs7_get_embedded_data(pkcs7->c,
> +					    GNUTLS_PKCS7_EDATA_GET_RAW,
> +					    &ctl->raw_ctl);
> +	if (rc != GNUTLS_E_SUCCESS) {
> +		DBG_ERR("Failed to get embedded data from pkcs7: %s",
> +			gnutls_strerror(rc));
> +		return -1;
> +	}
> +
> +	rc = asn1_create_element(ctl->asn1_desc,
> +				 "CATALOG.CertTrustList",
> +				 &ctl->tree_ctl);
> +	if (rc != ASN1_SUCCESS) {
> +		asn1_perror(rc);
> +		DBG_ERR("Failed to create CertTrustList ASN.1 element");
> +		return -1;
> +	}
> +
> +	rc = asn1_der_decoding(&ctl->tree_ctl,
> +			       ctl->raw_ctl.data,
> +			       ctl->raw_ctl.size,
> +			       error_string);
> +	if (rc != ASN1_SUCCESS) {
> +		asn1_perror(rc);
> +		DBG_ERR("Failed to parse ASN.1 CertTrustList: %s",
> +			error_string);
> +		return -1;
> +	}
> +
> +	tmp_ctx = talloc_new(ctl);
> +	if (tmp_ctx == NULL) {
> +		return -1;
> +	}
> +
> +	oid = mscat_asn1_get_oid(tmp_ctx,
> +				 ctl->tree_ctl,
> +				 "catalogListId.oid");
> +	if (oid == NULL) {
> +		rc = -1;
> +		goto done;
> +	}
> +
> +	ok = mscat_asn1_oid_equal(oid, CATALOG_LIST_OBJOID);
> +	if (!ok) {
> +		DBG_ERR("Invalid oid (%s), expected CATALOG_LIST_OBJOID",
> +			oid);
> +		rc = -1;
> +		goto done;
> +	}
> +	talloc_free(oid);
> +
> +	oid = mscat_asn1_get_oid(tmp_ctx,
> +				 ctl->tree_ctl,
> +				 "catalogListMemberId.oid");
> +	if (oid == NULL) {
> +		rc = -1;
> +		goto done;
> +	}
> +
> +	ok = mscat_asn1_oid_equal(oid, CATALOG_LIST_MEMBER_V2_OBJOID);
> +	if (ok) {
> +		ctl->version = 2;
> +	} else {
> +		ok = mscat_asn1_oid_equal(oid, CATALOG_LIST_MEMBER_OBJOID);
> +		if (ok) {
> +			ctl->version = 1;
> +		} else {
> +			DBG_ERR("Invalid oid (%s), expected "
> +				"CATALOG_LIST_MEMBER_OBJOID",
> +				oid);
> +			rc = -1;
> +			goto done;
> +		}
> +	}
> +
> +	rc = 0;
> +done:
> +	talloc_free(tmp_ctx);
> +	return rc;
> +}
> +
> +static int ctl_get_member_checksum_string(struct mscat_ctl *ctl,
> +					  TALLOC_CTX *mem_ctx,
> +					  unsigned int idx,
> +					  const char **pchecksum,
> +					  size_t *pchecksum_size)
> +{
> +	TALLOC_CTX *tmp_ctx;
> +	DATA_BLOB chksum_ucs2;
> +	size_t converted_size = 0;
> +	char *checksum;
> +	char *element;
> +	int rc = -1;
> +	bool ok;
> +
> +	tmp_ctx = talloc_new(mem_ctx);
> +	if (tmp_ctx == NULL) {
> +		return -1;
> +	}
> +
> +	element = talloc_asprintf(tmp_ctx,
> +				  "members.?%u.checksum",
> +				  idx);
> +	if (element == NULL) {
> +		goto done;
> +	}
> +
> +	rc = mscat_asn1_read_value(tmp_ctx,
> +				   ctl->tree_ctl,
> +				   element,
> +				   &chksum_ucs2);
> +	talloc_free(element);
> +	if (rc != 0) {
> +		goto done;
> +	}
> +
> +	ok = convert_string_talloc(mem_ctx,
> +				   CH_UTF16LE,
> +				   CH_UNIX,
> +				   chksum_ucs2.data,
> +				   chksum_ucs2.length,
> +				   (void **)&checksum,
> +				   &converted_size);
> +	if (!ok) {
> +		rc = -1;
> +		goto done;
> +	}
> +
> +	*pchecksum = talloc_steal(mem_ctx, checksum);
> +	*pchecksum_size = strlen(checksum + 1);

Shouldn't the above be:

	*pchecksum_size = strlen(checksum) + 1;

> +				goto done;
> +			}
> +
> +			DBG_DEBUG("Parsed NameValue: name=%s, flags=%u, value=%s",
> +				  name,
> +				  flags,
> +				  value);
> +
> +			cmp = strcmp(name, "File");
> +			if (cmp == 0) {
> +				m->file.name = talloc_steal(m, value);
> +				m->file.flags = flags;
> +
> +				continue;
> +			}
> +
> +			cmp = strcmp(name, "OSAttr");
> +			if (cmp == 0) {
> +				m->osattr.value = talloc_steal(m, value);
> +				m->osattr.flags = flags;
> +
> +				continue;
> +			}
> +		}
> +
> +		ok = mscat_asn1_oid_equal(oid, CAT_MEMBERINFO_OBJID);
> +		if (ok) {
> +			char *name;

Initialize name to NULL please.

Really nice work Andreas, thanks.

Just one sylistic comment. Instead of using
talloc_steal(), can you use talloc_move()
instead ?

talloc_move() nulls out the moved pointer,
making it more likely to crash on erroneous
reuse rather than corrupt stuff (which is
preferable IMHO).

I can't see a place where it matters in this
code, so feel free to ignore my advice here
(I won't block the patch if you don't do this :-).

Cheers,

	Jeremy.