Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider removing the xxhash dependency #4547

Closed
seelabs opened this issue May 30, 2023 · 5 comments
Closed

Consider removing the xxhash dependency #4547

seelabs opened this issue May 30, 2023 · 5 comments
Labels
Feature Request Used to indicate requests to add new features

Comments

@seelabs
Copy link
Collaborator

seelabs commented May 30, 2023

Summary

We currently use xxhash to hash values that are themselves already a sha-256 hash. The code should be audited to see if we could use the sha-256 hash directly and remove the xxhash dependency.

Motivation

It removes a dependency.

Solution

Use a sha-256 hash directly instead of rehashing with xxhash.

@seelabs seelabs added the Feature Request Used to indicate requests to add new features label May 30, 2023
@MarkusTeufelberger
Copy link
Collaborator

I think xxhash is used in NuDB as a faster and cheaper hash algorithm than sha256.

@seelabs
Copy link
Collaborator Author

seelabs commented May 31, 2023

@MarkusTeufelberger I was considering cases where we using a sha-256 hash as a key in an hash table. So we're using

xxhash(sha256_hash(value));

I don't think that xxhash is doing anything useful and we could potentially remove it.

@intelliot
Copy link
Collaborator

@seelabs do you think this is a Good First Issue?

@JoelKatz
Copy link
Collaborator

JoelKatz commented Jul 8, 2023

This would leave us vulnerable to algorithmic complexity attacks. The point of xxhash is that it is unpredictable to an attacker. An attacker can predict the SHA256 hashes of transactions, accounts, and ledger objects they create permitting them to force numerous XRPL algorithms into their worst-case behaviors.

However, if the input is a SHA256 hash, then a simpler algorithm than xxhash could, in theory, be used because all you need is a defense against algorithmic complexity attacks and not any of the other properties xxhash gives us. For example, you could determine a "seed" at start time that chooses which bytes of the SHA256 hash are extracted and used as a key. Then it's just byte extraction.

@seelabs
Copy link
Collaborator Author

seelabs commented Jul 10, 2023

I did not understand the purpose of hashing something that was already a SHA256 hash. Thanks for clarifying. I'm going to close this issue.

@seelabs seelabs closed this as completed Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Used to indicate requests to add new features
Projects
None yet
Development

No branches or pull requests

4 participants