subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Sperling <s...@elego.de>
Subject Re: Some x509 branch review points
Date Thu, 16 Oct 2014 14:27:33 GMT
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*

Mime
View raw message