Re: [Opensc-devel] suggested small fix

[Umberto R. a. U. <op...@se...>]

On 04/10/2014 02:57 PM, Umberto Rustichelli aka Ubi wrote:
>
> IMHO in opensc source, file src/tools/pkcs11-tool.c, function
>
> parse_certificate(struct x509cert_info *cert,
>                 unsigned char *data, int len)
>
> behaviour can potentially corrupt memory or lead to a segmentation fault.
>

Well, to be precise if it corrupts memory, there are two possibilities: 
the issue is trapped and the program exists with an error or there is a 
segmentation fault.
No way the code will proceed with a corruption in memory without 
stopping, which is of course a good thing.

Comments? Do I miss somenting?

  Umberto Rustichelli aka Ubi

> Three times, it writes into pre-allocated memory areas (here: 
> cert->subject, cert->issuer, cert->serialnum) where their dimension is 
> fixed (256, 256, 128) and the length (n) is checked when it is too late:
>
>        [...]
>        p = cert->subject;
>         n = i2d_X509_NAME(x->cert_info->subject, &p);
>         [...]
>         if (n > (int)sizeof (cert->subject))
>                 util_fatal("subject name too long");
>         [...]
>
>         p = cert->issuer;
>         n = i2d_X509_NAME(x->cert_info->issuer, &p);
>         [...]
>
>         p = cert->serialnum;
>         n = i2d_ASN1_INTEGER(x->cert_info->serialNumber, &p);
>         [...]
>
> Here is the host struct (cert is of this type):
>
> struct x509cert_info {
>         unsigned char   subject[256];
>         int             subject_len;
>         unsigned char   issuer[256];
>         int             issuer_len;
>         unsigned char   serialnum[128];
>         int             serialnum_len;
> };
>
> The correct behaviour should be to check length first (for all of the 
> three occurrences):
>
>         // check length first
>         n = i2d_X509_NAME(x->cert_info->subject, NULL);
>         if (n > (int)sizeof (cert->subject))
>                 util_fatal("subject name too long");
>         // green light, actually do it
>         p = cert->subject;
>         n = i2d_X509_NAME(x->cert_info->subject, &p);
>         [...]
>
> Do you agree?
>