Some x509 branch review points

[Philip Martin <ph...@wandisco.com>]

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*

Re: Some x509 branch review points

[Ben Reser <be...@reser.org>]

On 10/15/14 3:15 AM, 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.

The 0 is there to say that the context specific value is 0.

ASN.1 has several forms of types.  Universal, meaning the types that are
defined by ASN.1 itself (e.g. the string types).  Application, meaning types
that are specific to certain applications (e.g. X.500 types).  Private, types
specific to a certain enterprise.  Then finally there are context specific
types which are used to distinguish between types within a structure where a
type has already been set.

Within X.509 the context specific type of 0 is the X.509 version number.

So this specific case is documentation in the form of code.  It's true it's a
no-op and the compiler will remove it.  But if you are familiar with X.509 and
ASN.1 it makes the code make more sense.  Note that every other use of
ASN1_CONTEXT_SPECIFIC has something like this, they're just usually not zero.

I've added a comment to help clear this up in r1635358.

> 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.

Agreed, removed in r1635322.

> 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.

Nice catch, I think in some of these cases it's actually safe, but I've
converted them to just set the tags explicitly in r1635357.

Re: Some x509 branch review points

[Stefan Sperling <st...@elego.de>]

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*