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

Fix admin vulnerability to Brute Force #3492

Merged
merged 36 commits into from
Oct 10, 2024

Conversation

5e-Cleric
Copy link
Member

Added express-rate-limit package and implemented rate limiting for admin API login attempts

Have NOT tested this as of yet, feel free to do so. Should be a limit of 10 attempts per day.

@5e-Cleric 5e-Cleric added P0 - security or data loss Possible damage to server, users, or data 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed labels May 24, 2024
@5e-Cleric
Copy link
Member Author

5e-Cleric commented May 24, 2024

It works!

image

Copy link
Collaborator

@dbolack-ab dbolack-ab left a comment

Choose a reason for hiding this comment

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

Seems straightforward.

@G-Ambatte
Copy link
Collaborator

Here's a suggested tweak for admin.api.js:

// Define rate limiter options
const loginLimiter = rateLimit({
	timeWindow : 24 * 60 * 60 * 1000, // 24 hours window
	max        : 10, // limit each IP to 10 requests per timeWindow
	handler    : ()=>{throw { HBErrorCode: '100', code: 429, message: 'Too many requests' }; }
});

which results in:
image

I've just re-used HBErrorCode 100 on the proof of concept, we'd need to give this particular error its own code to use the Error Page. We might also want to do something similar for the Access Denied result.

@G-Ambatte
Copy link
Collaborator

On the actual error message, I think Too many failed login attempts, try again later is probably better than specifically stating that the ban is tied to their IP address.

@5e-Cleric
Copy link
Member Author

I'd rebuild the UI for this, it is not a brew that is being accessed, but the admin page. But yeah i can work with that, good point.

@5e-Cleric
Copy link
Member Author

image

This is better. I could use some advice on it anyway.

@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3492 June 16, 2024 15:06 Inactive
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3492 June 16, 2024 15:14 Inactive
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3492 June 16, 2024 15:22 Inactive
@5e-Cleric 5e-Cleric self-assigned this Aug 25, 2024
@Gazook89
Copy link
Collaborator

Gazook89 commented Sep 3, 2024

There are some spelling errors and awkward sentences that I think could be improved. I'm no wordsmith (I use 10 words where will 1 will do), but here is my take:

401

## Authorization Required
You need to provide correct administrator credentials to access this page.

:

If you should have access but are not able to log-in, please reach out via [reddit mod mail](https://old.reddit.com/message/compose/?to=/r/homebrewery) or in the [hb_issues channel of the Discord of Many Things server](https://discord.gg/domt).

403

## Access Denied
You need to provide correct administrator credentials to access this page.  *Too many unsuccessful attempts will lock you out for a period of time.*

:

If you should have access but are not able to log-in, please reach out via [reddit mod mail](https://old.reddit.com/message/compose/?to=/r/homebrewery) or in the [hb_issues channel of the Discord of Many Things server](https://discord.gg/domt).

470

## You have run out of attempts
You have failed to provide correct credentials to access the page too many times.  Please wait a bit before retrying again.

:

If you should have access but are not able to log-in, please reach out via [reddit mod mail](https://old.reddit.com/message/compose/?to=/r/homebrewery) or in the [hb_issues channel of the Discord of Many Things server](https://discord.gg/domt).

package.json Outdated
@@ -100,6 +100,7 @@
"expr-eval": "^2.0.2",
"express": "^4.21.0",
"express-async-handler": "^1.2.0",
"express-rate-limit": "^7.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

We can remove this package now if it's not being used.

Copy link
Member Author

Choose a reason for hiding this comment

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

You added it to the project for the 429 issues, right?

Copy link
Member

Choose a reason for hiding this comment

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

Took it back out because it didn't make a difference.

Copy link
Member

@calculuschild calculuschild Oct 10, 2024

Choose a reason for hiding this comment

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

I think this is the only thing. Remove that, fix any conflicts, and we can merge. I put my approval so you or I can merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will remove it as soon as i get home, but i won't be able to merge without another review..

Copy link
Collaborator

@Gazook89 Gazook89 left a comment

Choose a reason for hiding this comment

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

Just some remarks on messaging and a little bit of whitespace cleanup.

Marking 'approved' so as to not hold it up.

client/homebrew/pages/errorPage/errorPage.jsx Outdated Show resolved Hide resolved
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3492 October 10, 2024 21:01 Inactive
@5e-Cleric
Copy link
Member Author

Small PR, works great, thank you for the reviews and helpful comments! Specially @Gazook89, i know i am not the best at writing messages.

@5e-Cleric 5e-Cleric merged commit fcede54 into naturalcrit:master Oct 10, 2024
2 checks passed
@5e-Cleric 5e-Cleric deleted the fix-vulnerability-admin-pages branch October 22, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants