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

[Pierre Chifflier]

Hi,

On 11/01/2011 09:34 AM, Victor Julien wrote:
> 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?

Indeed, I changed that.

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

Done.

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

Ok, all warnings from the parser were removed. This makes it more silent
in the case the stream is malformed, but there is no other consequence.

Additionally, I have changed the license to BSD, as discussed.

Regards,
Pierre