[Opensc-devel] suggested small fix

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

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.

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?