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?
>
|
