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

Mitigate bruteforce attempts #708

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Antoine-Gicquel
Copy link

Hi,
I have seen past issues about this topic, which have all been disregarded because "if an attacker has access to the SoftHSM library, he could also access the filesystem and loot the token file, to try and bruteforce locally".

However, as a QubesOS user, I use extensively SoftHSM in combination with socat, so that my SoftHSM server is on a virtual machine with all my certificates (a vault VM), and my clients (for instance a Firefox process) are all in other VMs, with no direct access to the vault's file system. In such cases, I feel it makes the most sense to implement a kind of rate limiting in SoftHSM as it would drastically reduce the bruteforce pace of an attacker in a client VM. The way I implemented it is very trivial, feel free to make it cleaner or to tell me what to do in order to make it so.
By default, when no configuration option is added to the config file, it does not change the behavior of SoftHSM (no delay on failed attempts). However, if you add the login.fail_delay (here again, the name might not be in line with the configuration options naming scheme, feel free to tell me about it) in your config file, the value of the option you provided (in seconds) will be waited on a failed attempt.

Feel free to reach out if you have any question, or to make any modification to the code of this PR.

Best regards,
Antoine

Add "login.fail_delay" config option
Add delay on failed attempt to mitigate bruteforce
@Antoine-Gicquel
Copy link
Author

Antoine-Gicquel commented Apr 20, 2023

I guess these checks are broken x) The project should build perfectly, and has been tested on Debian 11.

Also to add on my initial post : I think it doesn't cost much to merge this PR, and it can help raise the security level of SoftHSM in some use-cases (not all cases, I hear you).

@@ -51,6 +51,7 @@ const struct config Configuration::valid_config[] = {
{ "slots.removable", CONFIG_TYPE_BOOL },
{ "slots.mechanisms", CONFIG_TYPE_STRING },
{ "library.reset_on_fork", CONFIG_TYPE_BOOL },
{ "login.fail_delay", CONFIG_TYPE_INT },
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend to add a default in softhsm2.conf.in and softhsm2.conf.5.in to document this new feature as well, e.g. somewhere here:

library.reset_on_fork = false

.SH LIBRARY.RESET_ON_FORK

Also to indicate which units (e.g. seconds or milliseconds) these are supposed to be set to.

@@ -263,6 +270,7 @@ bool SecureDataManager::login(const ByteString& passphrase, const ByteString& en

if (!RFC4880::PBEDeriveKey(passphrase, salt, &pbeKey))
{
sleep(Configuration::i()->getInt("login.fail_delay", 0));
Copy link
Contributor

@ijsf ijsf Nov 29, 2024

Choose a reason for hiding this comment

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

On Windows I would expect this to be a capitalized Sleep (https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-sleep) which takes in milliseconds, instead of the POSIX sleep which takes seconds, so I'm not sure how this could work for both platforms.

(I guess a utility function elsewhere would help.)

@jschlyter jschlyter marked this pull request as draft November 29, 2024 16:26
@jschlyter
Copy link
Contributor

Please rebase on develop and mark as ready when ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants