Some x509 branch review points

1)

In x509parse.c:x509_get_version:

  err = asn1_get_tag(p, end, &len,
                     ASN1_CONTEXT_SPECIFIC | ASN1_CONSTRUCTED | 0);

Why the "| 0"? It doesn't do anything.

2)

In x509parse.c:x509_get_name:

  cur->next = apr_palloc(result_pool, sizeof(x509_name));

  if (cur->next == NULL)
    return SVN_NO_ERROR;

We generally assume apr_palloc will abort rather than return NULL,
that's certainly the assumption in other places in this file. If we
were to detect an allocation failure then returning no error is not
correct.

3)

In x509parse.c:x509_get_name:

  oid = &cur->oid;
  oid->tag = **p;

  err = asn1_get_tag(p, end, &oid->len, ASN1_OID);
  if (err)
    return svn_error_create(SVN_ERR_X509_CERT_INVALID_NAME, err, NULL);

The asn1_get_tag() call verifies both that *p has not reached end and
that the tag is ASN1_OID. This happens after assigning **p to oid->tag,
so if asn1_get_tag were to detect that *p had reached end it would
happen after we had dereferenced **p. This bug occurs in other places:
x509_get_sig, x509_get_alg, etc.

Fix by assigning ASN1_OID explicitly or by moving the assignment after
asn1_get_tag.

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*