Switch crypto implementations to libsodium #1372
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jagerman
force-pushed
the
libsodium-primitives
branch
from
82f6662
to
5bb4b31
Compare
Both mlocker<T> and scrubbed<T> have ADL "unwrap" functions that simple cast to the `T` base type. This is idiotic and pointless because casting to a public base type is already implicitly done in C++. scrubbed_arr is idiotic in a different way: it just obfuscates the type without actually providing a useful abstraction.
This moves crypto::hash from crypto/hash.h into crypto/hash_type.h so that you can get *just* the hash type without the extra hashing code that hash.h pulls in. It also templates it as hash_t<SIZE> so that hash and hash8 are just different instantiations of hash_t<SIZE>. It also embeds a static `null` element so that hash::null and hash8::null can be used to get the null value rather than needing the namespace-polluting null_hash and null_hash8. Since this would be rather inconsistent with the other places we have null values, this applies the same change there: null_pkey -> public_key::null null_address -> account_public_address::null etc. This cleans up the code by decluttering the crypto and cryptonote namespaces a bit.
Avoid unintended conversions.
This caches one potentially nasty bug in particular where we are calling
vector<string>.push_back({string, crypto::null_hash}
This happened to compile because crypto::hash was previously implicitly
convertible to `bool', and so this ended up calling
std::string's (string, size_t pos) substring constructor by implicitly
converting the null_hash to a bool (which was false, since it's null)
and then from there casting to a size_t, and ending up calling
(string, 0). Since 0 is the start position of the whole string,
miraculously this didn't break anything.
It is clearer and easier to read. c.f. https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rt-using
This produced an alignment warning, but it turns out they simply aren't used anywhere.
Add generic point subclass comparisons and hashing without needing a bunch of dirty macros.
These wrappers just obfuscate what is being done without being a useful abstraction, so just eliminate them and replace with direct calls.
It is absolutely and completely pointless to build a wipeable_string that immedately gets copied into a std::string. Typical Monero idiocy.
Defer to std::string_view's hash function instead.
Most instances are replaced with either tools::hex_to_type, or lokimq::is_hex+lokimq::from_hex. Also cleans up/simplifies some hex converting code.
crypto::ec_scalar, ec_point, rct::key, crypto::hash, and related now
store std::byte arrays rather than the hodgepodge mix of `char`,
`unsigned char`, and `uint8_t` arrays.
C code interfaces are similarly unified to always use `unsigned char`
(where currently `char` was being used).
Additionally the C++ crypto structs all gain a `operator unsigned
char*()` implicit conversion operator so that they can be passed as-is
into C interface functions (including the embedded versions but also
libsodium), essentially allowing:
reinterpret_cast<unsigned char*>(whatever.data)
as an argument to be replaced with:
whatever
Included in this commit (because it is intricately tied to the above) is
a renaming, constexpr-ifying, and restructuring of `rct::key` constants.
`#pragma pack` can break things because it *also* sets the struct alignment to 1, which means accessing things in this struct are misaligned memory access. This is completely unnecessary here, though: the type will *already* be packed (and this adds a static assert verifying that).
Remove Monero NIH RNG with libsodium's well-tested, more robust version. Removes the terrible "thread-safe" random value generator which wasn't actually properly thread safe at all, but merely slapped a mutex around the random generator. Removes the ability to "add extra entropy". This seems like a well-intentioned but terribly dangerous option: for the vast majority of users this is likely to be no improvement (or make things worse), and for the small number of users who know better and are on systems where entropy is a problem, this is something better done with external tools like haveged. This also renames the random generation functions to have more descriptive names: - `crypto::rand<T>()` becomes `crypto::random_filled<T>()`. - `crypto::rand_idx(...)` and `rand_range` become `random_index` and `random_range`. - 'crypto::generate_random_bytes_thread_safe` becomes `crypto::fill_random` (and is now actually thread-safe via libsodium without needing a mutex on every call). - `crypto::random_scalar` is gone, replaced with a direct call to libsodium's `crypto_core_ed25519_scalar_random`
This code isn't currently used, but keep it around because it might be someday.
This functions are stupid, mostly unused, and where used are noticeably simplified by not using them.
Add constexpr crypto::rng so that we can use it with any random
algorithm (slightly simplifying needing to `crypto::random_device rng{}`
when invoking such a function).
Delete crypto::random_range - this is not saving anything over
std::uniform_int_distribution so just use it instead.
Move crypto::random_index to tools::random_index, where it belongs;
callers that want a cryptographically secure random index can now call
`tools::random_index(N, crypto::rng)`, or `tools::random_index(N)` if
they don't need cryptographic randomness.
This commit also added tools::random_element
This massive text file is pointless: the vast majority of tests here are randomly generated and are just testing the same thing over and over again in a massive file. Remove most of it, keeping 10 tests per randomly generated chunk rather than 256.
When you have:
operator bool() const { ... }
operator unsigned char*() { ... }
then, insanely, the pointer conversion is preferred to the const bool
conversion when used on a non-const object. Fix it by added a non-const
bool operator.
The v7 info doesn't need anything done, but it fails the assert() here for a debug build.
These were producing warnings or failing to compile because of recent changes in this branch.
Pass in a flat unsigned char* and walk through it via HASH_SIZE rather than a cursed `*[]`.
Replaces sc_reduce32 with:
scalar %= L;
for types like ec_scalar. Also supports
auto s2 = scalar % L;
to reduce-and-copy, and adds ed25519_scalar_reduce32 for the
implementation via libsodium, and for cases where we just have a pointer
rather than a struct.
The template deduction avoids implicit conversion except when `unsigned
char*` is the best conversion, but prevents things like
a == b
from triggering it (which they do with a plain implicit `unsigned char*`
conversion operator).
jagerman
force-pushed
the
libsodium-primitives
branch
from
46695b8
to
f37b690
Compare
It's stupid, and most of the places it is being used it is completely unnecessary.
Removes the profoundly stupid randXmrAmount() which generates a random ed25519 scalar in order to generate a uniform random uint64_t. Removes the profoundly broken populateFromBlockchain which isn't called anywhere, but also casts the randXmrAmount function pointer to a size_t rather than actually calling it. Cleans up a bunch of sloppy C code with too-large-scoped variables and eliminates some needless copying.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This (still very much draft work in progress!) PR is switching the crypto fundamentals to libsodium implementations, rather than needing to provide a full ed25519 implementation inside loki itself.
Aside from reducing size, cleaning up various related internals, this may have some effect at making us look less like a virus to Windows virus scanners; but even if it doesn't, it is a worthwhile undertaking as it reduces the amount of internalized code with a known-solid, well-regarded implementation.
Not all libsodium crypto functions can be directly used -- in particular, Monero's signature format is not standard EdDSA, though we can move closer to EdDSA with this branch.