<div dir="ltr">Hi Matt <div><br></div><div>Thanks for the patch. Unfortunately patch did not work. I continued debugging and found that issue was in constant_time_msb.</div><div><br></div><div>static inline unsigned int constant_time_msb(unsigned int a)<br></div><div>{</div><div>- <b>return 0 - (a >> (sizeof(a) * 8 - 1));</b></div><div><span style="white-space:pre">+ </span>return (((unsigned)((int)(a) >> (sizeof(int) * 8 - 1))));</div><div>} </div><div><br></div><div>Changed constant_time_msb implementation as shown above. All the tests passed. I have attached the <span style="font-size:12.8px">dis-assembly of the</span> code for both successful case and failure case. This was requested by Jakob. </div><div><br></div><div>Please let me know your suggestions and inputs</div><div><br></div><div>Regards</div><div>Jaya</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Dec 10, 2015 at 2:48 AM, Matt Caswell <span dir="ltr"><<a href="mailto:matt@openssl.org" target="_blank">matt@openssl.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
<br>
On 09/12/15 23:13, Benjamin Kaduk wrote:<br>
> On 12/09/2015 05:04 PM, Matt Caswell wrote:<br>
>><br>
>> On 09/12/15 11:44, Jayalakshmi bhat wrote:<br>
>>> Hi Matt,<br>
>>><br>
>>> I could build and execute the constant_time_test. I have attached the .c<br>
>>> file and test results. 34 tests have failed. All failures are<br>
>>> around constant_time_eq_8. This is the function I had mentioned in the<br>
>>> earlier mails.<br>
>> Not quite all. There is also a failure right at the beginning of your<br>
>> log in constant_time_is_zero_8. Although it looks very similar to the<br>
>> constant_time_eq_8 failure.<br>
>><br>
>> As to the failure it is very strange. This is the function doing the test:<br>
>><br>
>> int test_binary_op_8(unsigned<br>
>> char (*op) (unsigned int a, unsigned int b),<br>
>> const char *op_name, unsigned int a,<br>
>> unsigned int b, int is_true)<br>
>> {<br>
>> unsigned char c = op(a, b);<br>
>> if (is_true && c != CONSTTIME_TRUE_8) {<br>
>> printf( "Test failed for %s(%du, %du): expected %u "<br>
>> "(TRUE), got %u at line %d\n", op_name, a, b,<br>
>> CONSTTIME_TRUE_8, c,__LINE__);<br>
>> return 1;<br>
>> } else if (!is_true && c != CONSTTIME_FALSE_8) {<br>
>> printf( "Test failed for %s(%du, %du): expected %u "<br>
>> "(FALSE), got %u at line %d\n", op_name, a, b,<br>
>> CONSTTIME_FALSE_8, c,__LINE__);<br>
>> return 1;<br>
>> }<br>
>> printf( "Test passed for %s(%du, %du): expected %u got %u at line %d<br>
>> with %s\n", op_name, a, b, CONSTTIME_TRUE_8,<br>
>> c,__LINE__,is_true?"TRUE":"FALSE");<br>
>> return 0;<br>
>> }<br>
>><br>
>><br>
>> and the output we see in the log file is:<br>
>><br>
>> Test failed for constant_time_eq_8(0u, 0u): expected 255 (TRUE), got<br>
>> 4294967295 at line 85<br>
>><br>
>> That big number in the output is actually 0x7FFFFFFF in hex. The<br>
>> variable that it is printing here is "c" which is declared as an<br>
>> "unsigned char".<br>
>><br>
>> Please someone correct me if I'm wrong but doesn't the C spec guarantee<br>
>> that a "char" is 8 bits? In which case how can the value of "c" be<br>
>> greater than 255?????<br>
><br>
> C does not make such a guarantee, though recent-ish POSIX does. (This<br>
> system is a windows one, thought, right?)<br>
><br>
> In any case, due to C's type promotion rules, it's very difficult to<br>
> actually use types narrower than 'int', since integers get auto-promoted<br>
> to int at integer conversion time. This has extra-fun interactions with<br>
> varargs functions, depending on the platform ABI in use. (Always cast<br>
> NULL to a pointer type when passing to a varargs function; this does<br>
> cause real bugs.) Since c is unsigned, it is odd to see it get promoted<br>
> to (int)-1, since C type conversions are supposed to be<br>
> value-preserving, but it is certainly possible that the windows ABI is<br>
> doing something I don't expect. Adjusting things so that the format<br>
> specifier and the type passed to printf match (whether by casting c to<br>
> int or qualifying the format specifier) might help.<br>
<br>
</div></div>Thanks Ben.<br>
<br>
It's not 100% clear to me that we are dealing with a system where a char<br>
has more than 8 bits, but it certainly seems like a plausible<br>
explanation for what is going on. Especially when you look at the<br>
implementation of constant_time_eq_8:<br>
<span class=""><br>
static inline unsigned char constant_time_eq_8(unsigned int a, unsigned<br>
int b)<br>
{<br>
return (unsigned char)(constant_time_eq(a, b));<br>
}<br>
<br>
</span>The function "constant_time_eq" here returns an "unsigned int". The<br>
whole purpose of "constant_time_eq_8" is to provide a convenience<br>
function to create an 8 bit mask. If the number of bits in an unsigned<br>
char > 8 then this code is going to fail!<br>
<br>
Jaya - please could you try the attached patch to see if that resolves<br>
the problem. Please try re-executing both your SSL/TLS tests and the<br>
constant_time test. Let me know how you get on.<br>
<br>
Thanks<br>
<span class="HOEnZb"><font color="#888888"><br>
Matt<br>
<br>
</font></span><br>_______________________________________________<br>
openssl-users mailing list<br>
To unsubscribe: <a href="https://mta.openssl.org/mailman/listinfo/openssl-users" rel="noreferrer" target="_blank">https://mta.openssl.org/mailman/listinfo/openssl-users</a><br>
<br></blockquote></div><br></div>