[openssl-users] CBC ciphers + TLS 1.0 protocol does not work in OpenSSL 1.0.2d

[Jayalakshmi bhat]

Hi Matt

Thanks for the patch. Unfortunately patch did not work. I continued
debugging and found that issue was in constant_time_msb.

static inline unsigned int constant_time_msb(unsigned int a)
{
-    *return 0 - (a >> (sizeof(a) * 8 - 1));*
+ return (((unsigned)((int)(a) >> (sizeof(int) * 8 - 1))));
}

Changed constant_time_msb implementation as shown above. All the tests
passed. I have attached the dis-assembly of the code for both successful
case and failure case.  This was requested by Jakob.

Please let me know your suggestions and inputs

Regards
Jaya

On Thu, Dec 10, 2015 at 2:48 AM, Matt Caswell <matt at openssl.org> wrote:

>
>
> On 09/12/15 23:13, Benjamin Kaduk wrote:
> > On 12/09/2015 05:04 PM, Matt Caswell wrote:
> >>
> >> On 09/12/15 11:44, Jayalakshmi bhat wrote:
> >>> Hi Matt,
> >>>
> >>> I could build and execute the constant_time_test. I have attached the
> .c
> >>> file and test results. 34 tests have failed. All failures are
> >>> around constant_time_eq_8. This is the function I had mentioned in the
> >>> earlier mails.
> >> Not quite all. There is also a failure right at the beginning of your
> >> log in constant_time_is_zero_8. Although it looks very similar to the
> >> constant_time_eq_8 failure.
> >>
> >> As to the failure it is very strange. This is the function doing the
> test:
> >>
> >>  int test_binary_op_8(unsigned
> >>                             char (*op) (unsigned int a, unsigned int b),
> >>                             const char *op_name, unsigned int a,
> >>                             unsigned int b, int is_true)
> >> {
> >>     unsigned char c = op(a, b);
> >>     if (is_true && c != CONSTTIME_TRUE_8) {
> >>         printf( "Test failed for %s(%du, %du): expected %u "
> >>                 "(TRUE), got %u at line %d\n", op_name, a, b,
> >> CONSTTIME_TRUE_8, c,__LINE__);
> >>         return 1;
> >>     } else if (!is_true && c != CONSTTIME_FALSE_8) {
> >>         printf( "Test failed for  %s(%du, %du): expected %u "
> >>                 "(FALSE), got %u at line %d\n", op_name, a, b,
> >> CONSTTIME_FALSE_8, c,__LINE__);
> >>         return 1;
> >>     }
> >>      printf( "Test passed for %s(%du, %du): expected %u got %u at line
> %d
> >> with %s\n", op_name, a, b, CONSTTIME_TRUE_8,
> >> c,__LINE__,is_true?"TRUE":"FALSE");
> >>     return 0;
> >> }
> >>
> >>
> >> and the output we see in the log file is:
> >>
> >> Test failed for constant_time_eq_8(0u, 0u): expected 255 (TRUE), got
> >> 4294967295 at line 85
> >>
> >> That big number in the output is actually 0x7FFFFFFF in hex. The
> >> variable that it is printing here is "c" which is declared as an
> >> "unsigned char".
> >>
> >> Please someone correct me if I'm wrong but doesn't the C spec guarantee
> >> that a "char" is 8 bits? In which case how can the value of "c" be
> >> greater than 255?????
> >
> > C does not make such a guarantee, though recent-ish POSIX does.  (This
> > system is a windows one, thought, right?)
> >
> > In any case, due to C's type promotion rules, it's very difficult to
> > actually use types narrower than 'int', since integers get auto-promoted
> > to int at integer conversion time.  This has extra-fun interactions with
> > varargs functions, depending on the platform ABI in use.  (Always cast
> > NULL to a pointer type when passing to a varargs function; this does
> > cause real bugs.)  Since c is unsigned, it is odd to see it get promoted
> > to (int)-1, since C type conversions are supposed to be
> > value-preserving, but it is certainly possible that the windows ABI is
> > doing something I don't expect.  Adjusting things so that the format
> > specifier and the type passed to printf match (whether by casting c to
> > int or qualifying the format specifier) might help.
>
> Thanks Ben.
>
> It's not 100% clear to me that we are dealing with a system where a char
> has more than 8 bits, but it certainly seems like a plausible
> explanation for what is going on. Especially when you look at the
> implementation of constant_time_eq_8:
>
> static inline unsigned char constant_time_eq_8(unsigned int a, unsigned
> int b)
> {
>     return (unsigned char)(constant_time_eq(a, b));
> }
>
> The function "constant_time_eq" here returns an "unsigned int". The
> whole purpose of "constant_time_eq_8" is to provide a convenience
> function to create an 8 bit mask. If the number of bits in an unsigned
> char > 8 then this code is going to fail!
>
> Jaya - please could you try the attached patch to see if that resolves
> the problem. Please try re-executing both your SSL/TLS tests and the
> constant_time test. Let me know how you get on.
>
> Thanks
>
> Matt
>
>
> _______________________________________________
> openssl-users mailing list
> To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-users
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mta.openssl.org/pipermail/openssl-users/attachments/20151210/5cfaa5de/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: changes.zip
Type: application/zip
Size: 21559 bytes
Desc: not available
URL: <http://mta.openssl.org/pipermail/openssl-users/attachments/20151210/5cfaa5de/attachment-0001.zip>