[Oisf-devel] [PATCH 1/3] Add ASN.1 parser for X509 certificates (in DER format)

[Victor Julien]

On 10/25/2011 02:10 PM, Pierre Chifflier wrote:
> +const Asn1Generic * Asn1DerGet(const Asn1Generic *top, const uint8_t *seq_index, const uint32_t seqsz)
> +{
> +    const Asn1Generic * Node;

Our style is to use all lowercase for variable names. Side note, we
should write a style guide :)

> +
> +#ifndef __DECODE_DER_GET_H__
> +#define __DECODE_DER_GET_H__

Minor, but can you update this to __UTIL_DECODE_DER_GET_H__ to fully
reflect the filename?

> +static Asn1Generic * DecodeAsn1DerPrintableString(const unsigned char *buffer, uint32_t max_size, uint8_t depth)
> +{
> +    const unsigned char *d_ptr = buffer;
> +    uint32_t length, numbytes;
> +    uint32_t i;
> +    Asn1Generic *a;
> +    unsigned char c;
> +
> +    d_ptr++;
> +
> +    /* total sequence length */
> +    c = d_ptr[0];
> +    if ((c & (1<<7))>>7 == 0) { /* short form 8.1.3.4 */
> +        length = c;
> +        d_ptr++;
> +    } else { /* long form 8.1.3.5 */
> +        numbytes = c & 0x7f;
> +        length = 0;
> +        d_ptr++;
> +        for (i=0; i<numbytes; i++) {
> +            length = length<<8 | d_ptr[0];
> +            d_ptr++;
> +        }
> +    }
> +    if (length > max_size) {
> +        SCLogWarning(SC_ERR_ALPARSER, "printable-string length too big for data at depth %d\n", depth);
> +        return NULL;
> +    }
> +
> +    a = Asn1GenericNew();
> +    if (a == NULL)
> +        return NULL;
> +    a->type = ASN1_PRINTSTRING;
> +    a->strlen = length;
> +    a->str = SCMalloc(length+1);
> +    if (a->str == NULL) {
> +        SCFree(a);
> +        return NULL;
> +    }
> +    strncpy(a->str, (const char*)d_ptr, length);

Please use strlcpy.

> +Asn1Generic * DecodeDer(const unsigned char *buffer, uint32_t size)
> +{
> +    const unsigned char *d_ptr = buffer;
> +    uint32_t d_length, numbytes;
> +    Asn1Generic *cert;
> +    uint8_t c;
> +    uint32_t i;
> +
> +    /* Check that buffer is an ASN.1 structure (basic checks) */
> +    if (d_ptr[0] != 0x30 && d_ptr[1] != 0x82) { /* Sequence */
> +        SCLogWarning(SC_ERR_ALPARSER, "Invalid ASN.1 structure: not a sequence\n");
> +        return NULL;
> +    }
> +    c = d_ptr[1];
> +    if ((c & (1<<7))>>7 != 1) {
> +        SCLogWarning(SC_ERR_ALPARSER, "Invalid ASN.1 structure: first byte of length: bit 8 is not 1 (See 8.1.3.5)\n");
> +        return NULL;
> +    }
> +    numbytes = c & 0x7f;
> +    d_length = 0;
> +    d_ptr += 2;
> +    for (i=0; i<numbytes; i++) {
> +        d_length = d_length<<8 | d_ptr[0];
> +        d_ptr++;
> +    }
> +    if (d_length+(d_ptr-buffer) != size) {
> +        SCLogWarning(SC_ERR_ALPARSER, "Invalid ASN.1 structure: size of top-level sequence does not match length\n");

Please don't issue errors/warnings based on malformed traffic. I know we
have a few of those currently but these are scheduled for removal as
well. Btw, no newline should be at the end of these macro calls.

Cheers,
Victor

-- 
---------------------------------------------
Victor Julien
http://www.inliniac.net/
PGP: http://www.inliniac.net/victorjulien.asc
---------------------------------------------