Skip to content

Commit

Permalink
Merge bitcoin#29815: crypto: chacha20: always use our fallback timing…
Browse files Browse the repository at this point in the history
…safe_bcmp rather than libc's

2d18194 crypto: chacha20: always use our fallback timingsafe_bcmp rather than libc's (Cory Fields)

Pull request description:

  Looking at libc sources, apple and openbsd implementations match our naive fallback. Only FreeBSD (and only x86_64) seems to [implement an optimized version](https://github.com/freebsd/freebsd-src/blob/main/lib/libc/amd64/string/timingsafe_bcmp.S).

  It's not worth the hassle of using a platform-specific function for such little gain.

  Additionally, as mentioned below, this is the only case outside of sha2 that requires an autoconf check, and I have upcoming PRs to remove the sha2 ones.

  Apple's [impl is unoptimized](https://opensource.apple.com/source/Libc/Libc-1244.1.7/string/FreeBSD/timingsafe_bcmp.c.auto.html).

  As-is [OpenBSD's impl](https://github.com/openbsd/src/blob/master/lib/libc/string/timingsafe_bcmp.c).

  Relevant IRC conversation with sipa:

  > \<cfields\> sipa: chacha20poly1305.cpp uses libc's timingsafe_bcmp when possible. But looking around at apple/freebsd/openbsd, I don't see any impl that doesn't use the naive implementation that matches our fallback...
  > \<cfields\> is there any reason to belive there's an optimized impl somewhere that we're actually hitting?
  > \<cfields\> asking because after cleaning up sha2, timingsafe_bcmp is the last autoconf check that remains in all of crypto. It'd make life easy if we could just always use our internal one.
  > \<cfields\> *all of crypto/
  > \<sipa\> cfields: let's get rid of the dependency then
  > \<sipa\> it's a trivial function
  > \<sipa\> and if we need it for some platforms, no real reason not to use it on all

  After the above discusstion, I did end up finding the x86_64-optimized FreeBSD impl, but I don't think that's all that significant.

ACKs for top commit:
  sipa:
    utACK 2d18194
  fanquake:
    ACK 2d18194
  TheCharlatan:
    ACK 2d18194
  theStack:
    ACK 2d18194

Tree-SHA512: b9583e19ac2f77c5d572aa5b95bc4b53669d5717e5708babef930644980de7c5d06a9c7decd5c2b559d70b8597328ecfe513375e3d8c3ef523db80012dfe9266
  • Loading branch information
fanquake committed Apr 6, 2024
2 parents b5d2118 + 2d18194 commit 0f0e36d
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 13 deletions.
2 changes: 0 additions & 2 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -968,8 +968,6 @@ AC_CHECK_DECLS([setsid])

AC_CHECK_DECLS([pipe2])

AC_CHECK_FUNCS([timingsafe_bcmp])

AC_MSG_CHECKING([for __builtin_clzl])
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ ]], [[
(void) __builtin_clzl(0);
Expand Down
13 changes: 2 additions & 11 deletions src/crypto/chacha20poly1305.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,6 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif

#include <crypto/chacha20poly1305.h>

#include <crypto/common.h>
Expand All @@ -30,10 +26,7 @@ void AEADChaCha20Poly1305::SetKey(Span<const std::byte> key) noexcept

namespace {

#ifndef HAVE_TIMINGSAFE_BCMP
#define HAVE_TIMINGSAFE_BCMP

int timingsafe_bcmp(const unsigned char* b1, const unsigned char* b2, size_t n) noexcept
int timingsafe_bcmp_internal(const unsigned char* b1, const unsigned char* b2, size_t n) noexcept
{
const unsigned char *p1 = b1, *p2 = b2;
int ret = 0;
Expand All @@ -42,8 +35,6 @@ int timingsafe_bcmp(const unsigned char* b1, const unsigned char* b2, size_t n)
return (ret != 0);
}

#endif

/** Compute poly1305 tag. chacha20 must be set to the right nonce, block 0. Will be at block 1 after. */
void ComputeTag(ChaCha20& chacha20, Span<const std::byte> aad, Span<const std::byte> cipher, Span<std::byte> tag) noexcept
{
Expand Down Expand Up @@ -97,7 +88,7 @@ bool AEADChaCha20Poly1305::Decrypt(Span<const std::byte> cipher, Span<const std:
m_chacha20.Seek(nonce, 0);
std::byte expected_tag[EXPANSION];
ComputeTag(m_chacha20, aad, cipher.first(cipher.size() - EXPANSION), expected_tag);
if (timingsafe_bcmp(UCharCast(expected_tag), UCharCast(cipher.last(EXPANSION).data()), EXPANSION)) return false;
if (timingsafe_bcmp_internal(UCharCast(expected_tag), UCharCast(cipher.last(EXPANSION).data()), EXPANSION)) return false;

// Decrypt (starting at block 1).
m_chacha20.Crypt(cipher.first(plain1.size()), plain1);
Expand Down

0 comments on commit 0f0e36d

Please sign in to comment.