On Wed, Oct 15, 2014 at 11:15:42AM +0100, Philip Martin wrote:
> 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.
Inherited from the original tropicssl code.
I think we can remove it.
>
> 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.
The code we imported from tropicssl used malloc/free. So this probably
happened during the conversion from malloc/free to APR pool.
I agree that the null check should just be removed.
>
> 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*
Received on 2014-10-16 16:28:05 CEST