Use consttime_memequal instead of memcmp.

[antony-jr]

From the legacy source files , In lib/librcksum/rsum.c at https://github.com/AppImage/zsync2/blob/f93e10ab4d6204a8ec64ca5909fcc23417dbdcf7/lib/librcksum/rsum.c#L154 and https://github.com/AppImage/zsync2/blob/f93e10ab4d6204a8ec64ca5909fcc23417dbdcf7/lib/librcksum/rsum.c#L238, You use memcmp to compare security critical data(i.e cryptographic hash ) which is not recommended.

From memcmp manual page ($ man memcmp for more information) , The manual explicitly states not to use memcmp for comparing critical data.

Do not use memcmp() to compare security critical data, such as cryptographic secrets, because the required CPU time depends on the number of equal bytes. Instead, a function that performs comparisons in constant time is required. Some operating systems provide such a function (e.g., NetBSD's consttime_memequal()), but no such function is specified in POSIX. On Linux, it may be necessary to implement such a function oneself.

Thus I think it would be nice if zsync2 uses consttime_memequal , But it is only available in NetBSD , Therefore we have to write it ourselves , this is the function from NetBSD's source files.

int
consttime_memequal(const void *b1, const void *b2, size_t len)
{
 const char *c1 = b1, *c2 = b2;
 int res = 0;

 while (len --)
  res |= *c1++ ^ *c2++;

 /*
  * If the compiler for your favourite architecture generates a
  * conditional branch for `!res', it will be a data-dependent
  * branch, in which case this should be replaced by
  *
  * return (1 - (1 & ((res - 1) >> 8)));
  *
  * or rewritten in assembly.
  */
 return !res;
}

The above function can be declared as a static function in the source file(rsum.c) and can be used.

IMPORTANT: consttime_memequal() returns 0 if the given memory contents are distinct and 1 if its the same.

consttime_memequal() manual: https://www.daemon-systems.org/man/consttime_memequal.3.html

[TheAssassin]

@antony-jr you got this warning totally wrong. We do compare data in memory, yes. But it's no cryptographic secrets, which the manpage refers to.

When working with cryptographic data, in some cases, so-called side channel attacks may be performed to get information about said data. For example, by monitoring the execution time of a function, you might be able to estimate the length of the data. To prevent such attacks, in cryptography, functions are written so they take the same approximate time, independent from the input. The function you provided for example does not return immediately once a mismatch is found, but iterates over the whole input array even if it could return earlier. This way, an attacker cannot know any more whether the comparison failed (early return) or succeeded (normal runtime), and can therefore not try to estimate the result of the comparison.

All of this is only required when performing cryptographic work. We, however, don't use any cryptography. We use MD4 as a checksum function to verify the integrity of the data. In our scenario, we don't need any side channel attack preventing code, in fact it'd only increase the runtime for no reason.

Thanks for the suggestion anyway, and I hope you understand things a bit more now.

[antony-jr]

I know the difference between cryptographic secret and hash , I was just concerned about malicious data injection(And to make things worse , if an attacker manages to inject a machine code in the right place , It will be executed directly). Just to be safe , I thought using consttime_memequal would give us more security on such attacks. Sorry for the false alarm.