New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NTLM tests fail on big-endian platforms (e.g. SPARC) #1315

Closed
mkauf opened this Issue Mar 7, 2017 · 16 comments

Comments

Projects
None yet
3 participants
@mkauf
Contributor

mkauf commented Mar 7, 2017

I did this

Looked at the autobuilds page. On Solaris SPARC, the NTLM tests fail. On Solaris i386, they pass.

When the test suite is run, curl always generates the same random data. But still the tests fail on SPARC. That's because Curl_rand() returns an array of unsigned integers, which is then converted into a char array by the NTLM code. The result depends on the endianness of the platform. The NTLM tests pass only on little-endian platforms.

Maybe Curl_rand() should provide a char array in the first place, or maybe the test suite should be extended to also accept the NTLM strings that are generated on a big-endian platform.

I expected the following

The NTLM tests pass on Solaris SPARC.

curl/libcurl version

curl master

operating system

(Solaris) SPARC

jay added a commit to jay/curl that referenced this issue Mar 8, 2017

rand: treat fake entropy the same regardless of endianness
When the random seed is purposely made predictable for testing purposes
by using the CURL_ENTROPY environment variable, process that data in an
endian agnostic way so the the initial random seed is the same
regardless of endianness.

Bug: curl#1315
Reported-by: Michael Kaufmann
@jay

This comment has been minimized.

Member

jay commented Mar 8, 2017

Try https://github.com/curl/curl/compare/master...jay:fix_fake_rand_for_big_endian?expand=1

Also I notice that in this case or if libcurl is not built against an SSL library that has a strong random libcurl will fallback on generating a weak random, and that it does that using static variables for the initialization which is not thread safe. Though we could improve that by initializing it in global_init there is still the possibility that the generator in subsequent calls could return the same pseudorandom at the same time for multiple threads.

diff --git a/lib/easy.c b/lib/easy.c
index bed94a4..1b77c9a 100644
--- a/lib/easy.c
+++ b/lib/easy.c
@@ -72,6 +72,7 @@
 #include "multiif.h"
 #include "sigpipe.h"
 #include "ssh.h"
+#include "rand.h"
 /* The last 3 #include files should be in this order */
 #include "curl_printf.h"
 #include "curl_memory.h"
@@ -260,6 +261,13 @@ static CURLcode global_init(long flags, bool memoryfuncs)
 
   Curl_version_init();
 
+  {
+    unsigned int x = 0;
+    CURLcode res = Curl_rand(NULL, &x, 1);
+    if(res != CURLE_OK)
+      return res;
+  }
+
   return CURLE_OK;
 }
 
@bagder

This comment has been minimized.

Member

bagder commented Mar 8, 2017

Try https://github.com/curl/curl/compare/master...jay:fix_fake_rand_for_big_endian?expand=1

I don't think this will work either. Isn't the problem that the random value is used as a number so the copy to the destination will be treated as different numbers depending on the endian of the machine? We can probably fix it by doing strtol() on the "force_entropy" and store that number as a plain unsigned int instead.

not thread safe

Right, but moving the seeding to the init is only improving things marginally. The use of a static variable is still not really thread-safe, although I think the real outcome of threading there is even weaker random.

@jay

This comment has been minimized.

Member

jay commented Mar 8, 2017

Well you have the problem right but bitshifting should give the same result regardless of endianness, so I fail to see how it isn't a good solution. strtol could also work if the test data is changed to reflect the new interpretation.

moving the seeding to the init is only improving things marginally

I agree. Since libcurl doesn't have thread synchronization I don't have a good way to handle this. What about putting it in data like data->seed

@bagder

This comment has been minimized.

Member

bagder commented Mar 8, 2017

Well you have the problem right but bitshifting should give the same result regardless of endianness

My suspicion is that's the problem, not the solution. The memcpy() also gives the same pattern no matter of endianess.

If we shift in data to the same 3 bytes out of 4, or actually just in the same order independent of machine, that buffer will be treated as two different numbers if the code is then treating those 4 bytes as a numeric integer and as a 4 bytes buffer - on big vs little endians.

This is for a debug situation only and we want the same number for big and little endian (so that it will generate the same NTLM gunk), not the same binary pattern.

@jay

This comment has been minimized.

Member

jay commented Mar 8, 2017

This is for a debug situation only and we want the same number for big and little endian

Yeah but what I'm telling you is I'm doing that and so the memory is not the same. I'm sure we can agree on this simple case:

randseed = (0xFF << 8);

value is the same little or big but on little it's stored in memory as 00 ff 00 00 and on big it's stored in memory as 00 00 ff 00. but the important part is value is the same, which afaict is the solution...

@bagder

This comment has been minimized.

Member

bagder commented Mar 8, 2017

Oh right, sorry for being slow.

But in order to simplify the code, I can imagine instead doing this: (although it will require some test cases to get updated accordingly too)

@@ -45,14 +45,11 @@ static CURLcode randit(struct Curl_easy *data, unsigned int *rnd)
 
 #ifdef CURLDEBUG
   char *force_entropy = getenv("CURL_ENTROPY");
   if(force_entropy) {
     if(!seeded) {
-      size_t elen = strlen(force_entropy);
-      size_t clen = sizeof(randseed);
-      size_t min = elen < clen ? elen : clen;
-      memcpy((char *)&randseed, force_entropy, min);
+      randseed = atoi(force_entropy);
       seeded = TRUE;
     }
     else
       randseed++;
     *rnd = randseed;
@jay

This comment has been minimized.

Member

jay commented Mar 8, 2017

Yes that will work assuming CURL_ENTROPY isn't used outside of curl's testing by a user who sets it to abcd or something

@bagder

This comment has been minimized.

Member

bagder commented Mar 8, 2017

True, but I don't think we have any obligations to the rest of the world when it comes to how our debug build assumes our debug-environment variables are treated so I don't consider that an actual problem.

@mkauf

This comment has been minimized.

Contributor

mkauf commented Mar 8, 2017

Thanks for the patches. I think some additional changes are needed.

For example, this code in ntlm.c generates 2 unsigned integers with Curl_rand(). But actually it wants 8 bytes, not 2 unsigned integers. The resulting MD5 sum depends on the platform.

    /* Need to create 8 bytes random data */
    result = Curl_rand(data, &entropy[0], 2);
    if(result)
      return result;

    /* 8 bytes random data as challenge in lmresp */
    memcpy(lmresp, entropy, 8);

    /* Pad with zeros */
    memset(lmresp + 8, 0, 0x10);

    /* Fill tmp with challenge(nonce?) + entropy */
    memcpy(tmp, &ntlm->nonce[0], 8);
    memcpy(tmp + 8, entropy, 8);

    result = Curl_ssl_md5sum(tmp, 16, md5sum, MD5_DIGEST_LENGTH);
@bagder

This comment has been minimized.

Member

bagder commented Mar 8, 2017

Yeah that struck me too when thinking about this problem further. The original version was actually better than the atoi() since it made almost the right pattern, I think the problem is instead rather the randseed++ operation which will cause the returned 32 bit value to look different on big vs little endian...

@jay

This comment has been minimized.

Member

jay commented Mar 9, 2017

It's just the same problem in two units. If we return an array of integers we want to return the same values for both little and big endian, since we allow fake seeds. Either of our patches accomplish that for rand. In the case of ntlm, Curl_rand will write some integers which have the same value but will be different memory depending on endianness. ntlm code (but not digest, for example) will then copy the returned array of integers as an array of bytes which creates the same problem.

I think Michael's initial suggestion to change the function to write to a char array is a good idea so I've implemented it in draft2.

I also added a Curl_rand_hexonly but at the moment it's wasteful because it calls Curl_rand and then basically converts each byte returned to a hex char (ie it uses only half the available bits each byte), and also it doesn't null terminate which could lead to bugs. But this will give you an idea. It could be given null termination and then renamed Curl_make_random_hex_string or something more explicit that implies that.

@bagder

This comment has been minimized.

Member

bagder commented Mar 9, 2017

Looks fine - I think it is a fine compromise to change the API in order to get the debug random thing to work easier, and I agree that it should probably zero terminate the hex string within the function, so the length argument should then probably be "buffer size" that would include the final zero.

Would be good to see @mkauf verify that the fix actually works on big endian too - I assume some test cases need update after this too ?

@jay

This comment has been minimized.

Member

jay commented Mar 11, 2017

Actually I wrote it so that it would continue to give the same results on little endian and only big endian would see the change. That way no test data needs to be modified. I can update for the hex string function but first I'd like to confirm from @mkauf that it works as expected on SPARC.

@mkauf

This comment has been minimized.

Contributor

mkauf commented Mar 11, 2017

With the patch, the NTLM tests pass on both x86 and SPARC. Very nice work, @jay !

The test data for the digest tests need to be adjusted though, but that's no big deal (they fail in the same way on x86 and SPARC): 823 869 907

And there's this compiler warning:

rand.c: In function 'Curl_rand_hexonly':
rand.c:162:9: warning: operation on 'rnd' may be undefined [-Wsequence-point]
     *rnd++ = hex[*rnd & 0xF];
         ^
@bagder

This comment has been minimized.

Member

bagder commented Mar 13, 2017

Clever compiler warning. Yes we can't safely increment the pointer on the left side and refer to it on the right side of the statement like that and be sure how the code will end up executed.

@bagder

This comment has been minimized.

Member

bagder commented Apr 18, 2017

@jay, can you provide your suggest change here as a PR ?

bagder added a commit that referenced this issue May 4, 2017

rand: treat fake entropy the same regardless of endianness
When the random seed is purposely made predictable for testing purposes
by using the CURL_ENTROPY environment variable, process that data in an
endian agnostic way so the the initial random seed is the same
regardless of endianness.

- Change Curl_rand to write to a char array instead of int array.

- Add Curl_rand_hexonly to write only random hex characters.

Fixes: #1315

Co-authored-by: Daniel Stenberg
Reported-by: Michael Kaufmann

bagder added a commit that referenced this issue May 5, 2017

rand: treat fake entropy the same regardless of endianness
When the random seed is purposely made predictable for testing purposes
by using the CURL_ENTROPY environment variable, process that data in an
endian agnostic way so the the initial random seed is the same
regardless of endianness.

- Change Curl_rand to write to a char array instead of int array.

- Add Curl_rand_hex to write random hex characters to a buffer.

Fixes: #1315

Co-authored-by: Daniel Stenberg
Reported-by: Michael Kaufmann

bagder added a commit that referenced this issue May 6, 2017

rand: treat fake entropy the same regardless of endianness
When the random seed is purposely made predictable for testing purposes
by using the CURL_ENTROPY environment variable, process that data in an
endian agnostic way so the the initial random seed is the same
regardless of endianness.

- Change Curl_rand to write to a char array instead of int array.

- Add Curl_rand_hex to write random hex characters to a buffer.

Fixes: #1315

Co-authored-by: Daniel Stenberg
Reported-by: Michael Kaufmann

bagder added a commit that referenced this issue May 8, 2017

rand: treat fake entropy the same regardless of endianness
When the random seed is purposely made predictable for testing purposes
by using the CURL_ENTROPY environment variable, process that data in an
endian agnostic way so the the initial random seed is the same
regardless of endianness.

- Change Curl_rand to write to a char array instead of int array.

- Add Curl_rand_hex to write random hex characters to a buffer.

Fixes: #1315

Co-authored-by: Daniel Stenberg
Reported-by: Michael Kaufmann

@bagder bagder closed this in 1cafede May 8, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.