Skip to content

Conversation

@1ma
Copy link

@1ma 1ma commented Jul 21, 2025

The DATUM gateway already depends on libsodium, and it has a constant-time comparison function. This other PR sounds like the sort of thing that we shouldn't be even thinking about in this repo.

Tested in my own DATUM deployment.

@wizkid057
Copy link
Member

If I recall the reasoning correctly, the libsodium function does not work correctly and as intended when comparing different length values, and no such function exists within libsodium.

@luke-jr luke-jr added the refactor Reorganizing code label Jul 22, 2025
@luke-jr
Copy link
Contributor

luke-jr commented Jul 22, 2025

Right, sodium_memcmp can only compare memory of equal length, which doesn't work for things like passwords where the strings have potentially different lengths.

@1ma
Copy link
Author

1ma commented Jul 22, 2025

Oof, good catch. Pretty rough that they don't mention this in their docs.

Comparing passwords in constant time is very common, but in DG there's a bit of friction because it has the secret in cleartext. Usually a backend would store hashes of passwords and hash the incoming password before comparing both.

Still, I think we can use their pwhash API. DG would simply store the result of crypto_pwhash_str() in the datum_config struct, then in datum_api.c we would call crypto_pwhash_str_verify.

Let me try this approach before giving up.

@1ma 1ma force-pushed the sodium-memcmp branch from e57f5eb to 5d5fdbf Compare July 22, 2025 22:21
}
if (!datum_secure_strequals(datum_config.api_csrf_token, sizeof(datum_config.api_csrf_token)-1, json_string_value(j_csrf))) {
const char * const csrf_s = json_string_value(j_csrf);
if ((strlen(csrf_s) == sizeof(datum_config.api_csrf_token)-1) && 0 != sodium_memcmp(datum_config.api_csrf_token, json_string_value(j_csrf), sizeof(datum_config.api_csrf_token)-1)) {
Copy link
Author

@1ma 1ma Jul 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This length check before calling sodium_memcmp should not leak any information because the length of CSRF tokens is already public (64 bytes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Reorganizing code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants