botan icon indicating copy to clipboard operation
botan copied to clipboard

Possible static init order fiasco in `Ed25519_PrivateKey::from_seed`

Open polarnis opened this issue 7 months ago • 2 comments

Hello! It seems that there might be preset static init order fiasco between initialization of this table: https://github.com/randombit/botan/blob/6a4f830b2ccc447c4325ea540266e48f10cc5de7/src/lib/pubkey/ed25519/ge.cpp#L595 (which is reachable from ed25519_gen_keypair) and initialization of statics in other translation units, like in this example:

#include <botan/ed25519.h>
#include <array>

constexpr std::array<uint8_t, 32> seed = {
    0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
    0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
    0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
    0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
};

static const auto static_global = Botan::Ed25519_PrivateKey::from_seed(seed);

int main()
{
    const auto local = Botan::Ed25519_PrivateKey::from_seed(seed);

    // returns 0
    return local.public_key_bits() == static_global.public_key_bits();
}

I observed that B_precomp is filled unexpectedly with zeros when constructing static_global, but properly initialized when constructing local. I was able to reproduce it with Clang on MacOS and Emscripten (I haven't checked other compilers/platforms).

One possible solution would be to make use of "construct on first use" idiom, but most likely adding constexpr to that array (and possibly to FE_25519 constructor) should be sufficient. Unfortunately I wasn't able to test either.

Edit: And of course usage of globals on it's own might be questionable (I discovered this issue when constructing helpers in unit tests), but I believe it's still worth fixing.

polarnis avatar Jun 06 '25 11:06 polarnis

Thanks for the report.

of course usage of globals on it's own might be questionable

Using objects from Botan as globals is worth avoiding if possible; historically we have had other bugs of a somewhat similar fashion. Certainly we do not check the behavior of this systematically, so there may be other issues elsewhere that have not been encountered and reported yet. That said any situation where the code silently misbehaves is something we certainly want to know about and will fix if possible.

randombit avatar Jun 06 '25 16:06 randombit

@polarnis I was not able to replicate the issue on Linux with GCC or Clang, so it's hard to create a fix. Can you see if applying #4907 resolves the issue for you?

randombit avatar Jun 08 '25 13:06 randombit

Hello @randombit and @polarnis,

To help investigate the error case here, I tested the error with Clang on Debian 12 as per the information provided. Proceeding as follows, I was able to get the error first, and when I switched to the current ‘master’ branch and checked again, I confirmed that the error did not occur.

Test Steps

  1. Checked out the problematic commit:
git checkout botan_master
git pull && git fetch --prune
git checkout 6a4f830  # Commit where issue was present
  1. I added a test with the name ed25519_static_order_fiasco to test the situation and provide control.:
// I add it above the `namespace botan test` field under the include contents.
// Global static seed and define
constexpr std::array<uint8_t, 32> seed = {
	0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
	0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
};
static const auto static_global = Botan::Ed25519_PrivateKey::from_seed(seed);

... // Same

class Ed25519_Static_Init_Order_Fiasco_Tests final : public Test {
	public:
	   std::vector<Test::Result> run() override {
	   Test::Result result("Ed25519 Static Order Fiasco");

	   const auto local = Botan::Ed25519_PrivateKey::from_seed(seed);

	   std::cout << "Local hex: " << Botan::hex_encode(local.public_key_bits()) << std::endl;
	   std::cout << "Static Global hex: " << Botan::hex_encode(static_global.public_key_bits()) << std::endl;

	   result.confirm("Bits same check", local.public_key_bits() == static_global.public_key_bits());
	   return std::vector<Test::Result>{result};
	 }
};

... // Same

BOTAN_REGISTER_TEST("pubkey", "ed25519_static_order_fiasco", Ed25519_Static_Init_Order_Fiasco_Tests);
  1. Build and test execution:
python3 src/scripts/dev_tools/run_clang_format.py --clang-format=clang-format-17 --src-dir=src && ninja clean && ./configure.py --without-documentation --cc=clang --compiler-cache=ccache --build-targets=static,cli,tests --build-tool=ninja && ninja
./botan-test ed25519_static_order_fiasco

Issue Confirmation - Commit 6a4f830

The test clearly demonstrated the static initialization order fiasco:

Testing Botan 3.9.0 (unreleased, revision git:6a4f830b2ccc447c4325ea540266e48f10cc5de7)
Properties:
CPU flags: sse2 ssse3 rdtsc adx bmi2 rdrand rdseed aesni clmul intel_sha
drbg_seed: 18593FB3ED8FB010
Local hex: AC7A4F4A227622FE731652EFFD24E818AED8232784CC53B48889FDF029D7EACB
Static Global hex: 0000000000000000000000000000000000000000000000000000000000000000
ed25519_static_order_fiasco:
Ed25519 Static Order Fiasco ran 1 tests 1 FAILED
Failure 1: Ed25519 Static Order Fiasco Bits same check produced unexpected result '0' expected '1' (at src/tests/test_ed25519.cpp:145)
Tests complete ran 1 tests in 79.81 msec 1 tests failed (in ed25519_static_order_fiasco)

Resolution Verification - Latest Commit 1f99875

After switching to the latest commit:

git checkout botan_master  # Latest commit 1f99875

Re-compile same command and Re-run the same test showed the issue was resolved:

Testing Botan 3.9.0 (unreleased, revision git:6b24db8518d17ffae51ef07c5a32e5748d3aa40c)
Properties:
  CPU flags: sse2 ssse3 rdtsc adx bmi2 rdrand rdseed aesni clmul intel_sha
  drbg_seed: 1859406CFE5B7587
ed25519_static_order_fiasco:
Local hex: AC7A4F4A227622FE731652EFFD24E818AED8232784CC53B48889FDF029D7EACB
Static Global hex: AC7A4F4A227622FE731652EFFD24E818AED8232784CC53B48889FDF029D7EACB
Ed25519 Static Order Fiasco ran 1 tests all ok
Tests complete ran 1 tests in 24.68 msec all tests ok

Both local and static global keys now produce identical, correct public keys.

Additional Test/Different Compiler: GCC

When I performed this test for the GCC compiler considering the following build command, I did not get the error condition. It seems to be related to Clang.

./configure.py && make clean && make -j$(nproc) && export LD_LIBRARY_PATH=/home/kagancansit/Desktop/Projects/botan:$LD_LIBRARY_PATH
./botan-test ed25519_static_order_fiasco # 6a4f830 commit fine here

Information Detail My Local Environment

Debian 12 | 6.1.0-37-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.1.140-1 (2025-05-22) x86_64 GNU/Linux

Debian clang version 14.0.6
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

gcc (Debian 12.2.0-14+deb12u1) 12.2.0
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

g++ (Debian 12.2.0-14+deb12u1) 12.2.0
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

I hope I have been able to help you. I think the problem here is solved and no additional development is needed.

Best regards.

KaganCanSit avatar Aug 06 '25 18:08 KaganCanSit

Thanks a lot @randombit for fixing this and @KaganCanSit for testing! I ran into the issue by accident and ended up avoiding reliance on static initialization order. Unfortunately, I didn't have the capacity to test it again myself.

Great to see it resolved nonetheless. Since we now have confirmation, I'll go ahead and close the issue.

polarnis avatar Aug 06 '25 19:08 polarnis