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

Attackers can crash the wallet via the gRPC service #5809

Closed
SWvheerden opened this issue Sep 26, 2023 · 1 comment · Fixed by #5936
Closed

Attackers can crash the wallet via the gRPC service #5809

SWvheerden opened this issue Sep 26, 2023 · 1 comment · Fixed by #5936
Labels
release-blocker Something that needs to be fixed before a release can be made

Comments

@SWvheerden
Copy link
Collaborator

SWvheerden commented Sep 26, 2023

The BasicAuthCredentials::validate method compares the username and
password for the service (specified in the service's configuration file) with the
BasicAuthCredentials parsed from the network. To do so it uses
Argon2::default().verify_password which takes a plain text password string and
a hash of the expected password. Unfortunately, the wallet implementation
inverts these parameters and instead of passing the client supplied password to
the function, it uses the expected password and performs PasswordHash::parse on
the attacker controlled input.

The way argon2 hashes are designed, allows users to store the hashing
parameters (salt, memory usage, CPU usage, etc) in the hash itself for future
updates and flexibility. This means that PasswordHash::parse does more than
parsing a hashed password; it sets the parameters (including algorithm, memory
used, and time spent) that will be used to verify the password, therefore the
parsed data should come from a trusted source, for instance a database or a local
configuration file, as is the case with the wallet.

Because the parameters are controlled by the attacker, it is possible for them to
crash the program or use an arbitrary amount of resources on the victim system.
This issue could also lead to complete authentication bypass due to the current
design: an attacker can supply a hash with parameters that are less secure than
the default ones defined by the argon2 crate. For example the argon2 specification
defines the minimum hash byte length to be 4 bytes, so an attacker could send a
hash with these parameters and they would have to only brute-force 2^32
passwords to find a collision with the actual password.
This situation is rendered several orders of magnitude harder due to the better
defaults that the argon2 crate defines for the minimum hash size, which is 10
bytes, making the attack unfeasible.
The likelihood of this issue is deemed low as it is expected that most
configurations will keep wallets and node running on the same host.

If the existing authentication method is retained, the client must provide a
password for the server to hash and compare with the stored hash of the expected
password.

@SWvheerden SWvheerden added the release-blocker Something that needs to be fixed before a release can be made label Oct 23, 2023
@SWvheerden
Copy link
Collaborator Author

This will be partly address for most use cases by: ##5930

SWvheerden pushed a commit that referenced this issue Nov 14, 2023
Description
---
Standardizes gRPC authentication by removing PHC strings from
configuration and preventing a client DoS.

Closes #5809. Closes #5927.

Motivation and Context
---
As noted in #5927, gRPC authentication is nonstandard. In the current
design, server credentials are processed against a client-supplied
username and PHC string for verification. This can lead to server DoS,
as described in #5809.

This PR fixes the DoS vector. When the gRPC server is started, it
applies `Argon2` to produce a PHC string that is kept in memory. When a
client supplies its (non-PHC) passphrase, it is processed against the
stored PHC string. This ensures that the server completely controls the
`Argon2` parameters that are used.

Note that this is still suboptimal, as the client and server passphrases
are still stored in plaintext configuration. Deciding how to handle
those is out of scope for this work.

How Has This Been Tested?
---
It should be tested manually.

What process can a PR reviewer use to test or verify this change?
---
It should be tested manually.

BREAKING CHANGE: Updates the public APIs for `BasicAuthCredentials` and
`ServerAuthenticationInterceptor` to accommodate the new behavior.
Additionally, existing configuration client/server credentials will stop
working, and client credentials will need to use plaintext passwords.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker Something that needs to be fixed before a release can be made
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant