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.

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

[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

[Bob Beck]

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

On Tue, Apr 22, 2014 at 7:05 PM, Michael W. Bombardieri  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;
>  }
>

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;
 }