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*
