suggested small fix

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

suggested small fix

Umberto Rustichelli aka Ubi

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?


------------------------------------------------------------------------------
Put Bad Developers to Shame
Dominate Development with Jenkins Continuous Integration
Continuously Automate Build, Test & Deployment
Start a new project now. Try Jenkins in the cloud.
http://p.sf.net/sfu/13600_Cloudbees
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: suggested small fix

Umberto Rustichelli aka Ubi
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?
>


------------------------------------------------------------------------------
Put Bad Developers to Shame
Dominate Development with Jenkins Continuous Integration
Continuously Automate Build, Test & Deployment
Start a new project now. Try Jenkins in the cloud.
http://p.sf.net/sfu/13600_Cloudbees
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: suggested small fix

Ludovic Rousseau
2014-04-10 15:03 GMT+02:00 Umberto Rustichelli aka Ubi <[hidden email]>:

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

Memory corruption can have very bad effects.

I opened a pull request at https://github.com/OpenSC/OpenSC/pull/231
Thanks

--
 Dr. Ludovic Rousseau

------------------------------------------------------------------------------
Put Bad Developers to Shame
Dominate Development with Jenkins Continuous Integration
Continuously Automate Build, Test & Deployment
Start a new project now. Try Jenkins in the cloud.
http://p.sf.net/sfu/13600_Cloudbees
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: suggested small fix

Douglas E Engert
Why does the x509cert_info  have fixed length arrays? Why not pointers?
Its using OpenSSL routines that can handle this...

Why not:

struct x509cert_info {
         unsigned char   * subject;
         int             subject_len;
         unsigned char   * issuer;
         int             issuer_len;
         unsigned char   * serialnum;
         int             serialnum_len;
};

The patch to do this would be larger,  but would remove the length restrictions.



On 4/10/2014 9:46 AM, Ludovic Rousseau wrote:

> 2014-04-10 15:03 GMT+02:00 Umberto Rustichelli aka Ubi <[hidden email]>:
>> 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?
> Memory corruption can have very bad effects.
>
> I opened a pull request at https://github.com/OpenSC/OpenSC/pull/231
> Thanks
>

--

  Douglas E. Engert  <[hidden email]>
 


------------------------------------------------------------------------------
Put Bad Developers to Shame
Dominate Development with Jenkins Continuous Integration
Continuously Automate Build, Test & Deployment
Start a new project now. Try Jenkins in the cloud.
http://p.sf.net/sfu/13600_Cloudbees
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel