small patch: CRYPTO_memcmp

[Michael W. Bombardieri]

Hi tech@,

Sending this patch for comment...

CRYPTO_memcmp() is different to memcmp() because it can only check
for equality, not greater-than/less-than.
If we check the string in reverse order we can remove a variable
from the comparison loop.

Does this look ok?

- Michael



Index: cryptlib.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/cryptlib.c,v
retrieving revision 1.23
diff -u -r1.23 cryptlib.c
--- cryptlib.c  21 Apr 2014 11:19:28 -  1.23
+++ cryptlib.c  23 Apr 2014 01:19:39 -
@@ -727,15 +727,13 @@
 }
 
 int
-CRYPTO_memcmp(const void *in_a, const void *in_b, size_t len)
+CRYPTO_memcmp(const void *in_a, const void *in_b, size_t n)
 {
-   size_t i;
const unsigned char *a = in_a;
const unsigned char *b = in_b;
unsigned char x = 0;
 
-   for (i = 0; i  len; i++)
-   x |= a[i] ^ b[i];
-
+   while (n--  0)
+   x |= a[n] ^ b[n];
return x;
 }

Re: small patch: CRYPTO_memcmp

[Bob Beck]

Nope. One of those things is not like the other..

On Tue, Apr 22, 2014 at 7:05 PM, Michael W. Bombardieri m...@ii.net wrote:
 Hi tech@,

 Sending this patch for comment...

 CRYPTO_memcmp() is different to memcmp() because it can only check
 for equality, not greater-than/less-than.
 If we check the string in reverse order we can remove a variable
 from the comparison loop.

 Does this look ok?

 - Michael



 Index: cryptlib.c
 ===
 RCS file: /cvs/src/lib/libssl/src/crypto/cryptlib.c,v
 retrieving revision 1.23
 diff -u -r1.23 cryptlib.c
 --- cryptlib.c  21 Apr 2014 11:19:28 -  1.23
 +++ cryptlib.c  23 Apr 2014 01:19:39 -
 @@ -727,15 +727,13 @@
  }

  int
 -CRYPTO_memcmp(const void *in_a, const void *in_b, size_t len)
 +CRYPTO_memcmp(const void *in_a, const void *in_b, size_t n)
  {
 -   size_t i;
 const unsigned char *a = in_a;
 const unsigned char *b = in_b;
 unsigned char x = 0;

 -   for (i = 0; i  len; i++)
 -   x |= a[i] ^ b[i];
 -
 +   while (n--  0)
 +   x |= a[n] ^ b[n];
 return x;
  }

Re: small patch: CRYPTO_memcmp

[Ted Unangst]

On Wed, Apr 23, 2014 at 09:05, Michael W. Bombardieri wrote:

 CRYPTO_memcmp() is different to memcmp() because it can only check
 for equality, not greater-than/less-than.
 If we check the string in reverse order we can remove a variable
 from the comparison loop.
 
 Does this look ok?

Almost, but...

 + while (n--  0)
 + x |= a[n] ^ b[n];

Won't compare the bytes at [0]. I think switching this to be
timingsafe_bcmp would be better, then we only have copy.

Re: small patch: CRYPTO_memcmp

[Miod Vallat]

  +   while (n--  0)
  +   x |= a[n] ^ b[n];
 
 Won't compare the bytes at [0].

Uh? It will, n gets decremented after the test but before the x |=
statement.

 I think switching this to be
 timingsafe_bcmp would be better, then we only have copy.

Agreed.

Re: small patch: CRYPTO_memcmp

[Bob Beck]

On Wed, Apr 23, 2014 at 04:39:01AM +, Miod Vallat wrote:
   + while (n--  0)
   + x |= a[n] ^ b[n];
  
  Won't compare the bytes at [0].
 
 Uh? It will, n gets decremented after the test but before the x |=
 statement.

Heh. you're right.  And both Ted and I were dumbasses. I have
tied the Mr. Gumby hankerchief to my head.

Although probably means that hack is really not readable enough to be
worth saving 8 bytes of stack ;)

 
  I think switching this to be
  timingsafe_bcmp would be better, then we only have copy.
 
 Agreed.
 
Yup.