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.
Received on 2014-10-30 01:50:34 CET