magfest / ubersystem

MAGFest's Ubersystem - handles ticketing, staffing, analytics, volunteers, and tons more
http://magfest.org
GNU Affero General Public License v3.0
48 stars 55 forks source link

Insecure login failure message #1340

Open earl7399 opened 9 years ago

earl7399 commented 9 years ago

We need to change this error message - it tells hackers to move on to a different e-mail:

image

The less-of-a-security-risk version is usually something like "That ID/Password combination is invalid"

earl7399 commented 9 years ago

Lower priority, but it would be nice if these messages were less blue / more red, and possibly required acknowledgement before vanishing.

EliAndrewC commented 9 years ago

We do what you're describing where I work, but it's a security vs usability tradeoff. In the past I've deliberately erred on the side of usability in this particular case for the following reasons:

The system I described above works perfectly well, but seemed like enough of an extra hassle for our dept heads that I figured it wasn't worth the marginal extra security.

However, now that I'm no longer the person who generally creates new admin accounts for people, I care a lot less about this process and have no objection to someone implementing the extra security version :P

earl7399 commented 9 years ago

I'd rather not increase the chances that someone can break into the system in order to make life easier. I'm pushing a fix and figure someone smarter than me, or higher up the chain at least, can make the final decision.

binary1230 commented 9 years ago

I agree with Eli that I think for Magfest, we should err on the side of usability on this one.

Though, I could see doing one or all of these things here to mitigate brute-force attacks while still keeping helpful feedback:

i.e. if they try more than say 10 wrong usernames/passwords in a small period of time, we kill their IP address. that will effectively make any brute force attacks impossible because they likely would need to guess thousands of passwords quickly

kitsuta commented 9 years ago

I don't know, I think this is the wrong behavior to leave in to make up for the bigger problem of people's admin accounts not persisting. Not telling the user what account does or doesn't exist is a pretty standard obfuscation measure, I think we should add it in and brainstorm a better fix to the other problem.

For example, could we import admin accounts when we import staffers? Could we generate a report for department heads without admin accounts (that is, if Brent would be willing to get yet another email every week :))? If someone's account doesn't exist when they click 'forgot password,' could we email a specific address telling them that someone requested a password for a non-existent account? I think any or all of these things could fix the usability problem while allowing us to not let just anyone be able to sniff out who does and doesn't have an account.

bds002 commented 9 years ago

I already have a way of telling who doesn't have an admin account.. the DH checklist does that for me. https://classic.uber.magfest.org/uber/dept_checklist/overview

earl7399 commented 9 years ago

That makes me curious - is there a reason you don't add missing accounts now?

Robert Earl r.a.earl@gmail.com FAX: 1-440-919-5086

On Wed, Aug 5, 2015 at 2:55 PM, Brent Smart notifications@github.com wrote:

I already have a way of telling who doesn't have an admin account.. the DH checklist does that for me.

— Reply to this email directly or view it on GitHub https://github.com/magfest/ubersystem/issues/1340#issuecomment-128108648 .

bds002 commented 9 years ago

Because that just become live today. and not all the DH's come to Classic, so why make an account for someone that's not there?

bds002 commented 9 years ago

I use the function of a DH asking for a Admin account as a way to verify that they are coming and working on task. IE - have we heard from them or just assuming they are on board. I have to get someone to create my Admin account every time as well. ;)

EliAndrewC commented 9 years ago

I can't speak for Brent, but back when I was the one creating accounts, my logic was basically, "I'm not entirely sure who's running each of these departments this time around, so I'll just wait for people to ask for an account and then make them one after they ask." I would have been especially inclined to do that for an event like MagClassic, where who's heading departments might be different from MAGFest itself, and I don't read all of the mailing lists closely enough to know who's doing what.

bds002 commented 9 years ago

-- But back to your original point/question ... Changing the verbage to ""That ID/Password combination is invalid"" is a valid thought. and following it with a link to Email stops or someone to request an update.....

We are at a point where if someone request an Admin account, we can usually get it setup within 12-24 hours, at least let's where we are with Classic.

So waiting "forever" ;) shouldn't happen for a response.

earl7399 commented 9 years ago

Honestly, we shouldn't have to wait. I'm not sure what to do about that, since there isn't an automated system for creating accounts, and apparently, no real support for one, either.

So are we back to just obfuscating whether an account exists? If we do that, how will Joe DH know to ask for one?

Robert Earl r.a.earl@gmail.com FAX: 1-440-919-5086

On Wed, Aug 5, 2015 at 3:10 PM, Brent Smart notifications@github.com wrote:

-- But back to your original point/question ... Changing the verbage to ""That ID/Password combination is invalid"" is a valid thought. and following it with a link to Email stops or someone to request an update.....

We are at a point where if someone request an Admin account, we can usually get it setup within 12-24 hours, at least let's where we are with Classic.

So waiting "forever" ;) shouldn't happen for a response.

— Reply to this email directly or view it on GitHub https://github.com/magfest/ubersystem/issues/1340#issuecomment-128113384 .

bds002 commented 9 years ago

Joe DH will have to read their emails.... Stops sent an Email to the DH mailing list that says in part: "If you have an Admin account for the MAGClassic database, you should also be able to login in here: https://classic.uber.magfest.org/uber/accounts/login If you do not have an admin account yet, please email stops@magfest.org and we can get that setup, or contact Staff-Ops on the Slack channel. (Please note, you should have accepted your badge and completed the registration process first)."

bds002 commented 9 years ago

I would also like that verbage added to the Automate Email when we import Staff DH badges for MAGFest 2016 ( @EliAndrewC )

binary1230 commented 9 years ago

I did a little more looking in and it looks like an easy solution to fully defeat these attacks (rate limiting) is trivial to implement:

https://lincolnloop.com/blog/rate-limiting-nginx/

@earl7399 if you wanted to try and implement this, I think it could be done really easily in puppet by having it add a few lines to the nginx config.

try adding them to nginx.pp in magfest/ubersystem-puppet here:

location_cfg_append => {

something like: (you will have to modify the URL here)

location_cfg_append => {
...
'limit_req_zone' => '\$binary_remote_addr zone=login:10m rate=1r/s;'
}

once you do that, do a redeploy with './apply-node.sh localhost' and you should be able to test the limiting behavior locally and see it start rejecting your requests.

-Dom

earl7399 commented 9 years ago

I don't have a particular issue with protecting the website in general from brute force attacks, but setting up rate-limiting while telling an attacker to try a different e-mail address is, to be blunt, just plain stupid.

Rate-limiting should in fact be implemented IMO, but it should be part of a larger solution, not the only the we do.

That said, it makes sense to me to implement some of the basics in Staging. On Aug 5, 2015 3:25 PM, "Dominic Cerquetti" notifications@github.com wrote:

I did a little more looking in and it looks like an easy solution to fully defeat these attacks (rate limiting) is trivial to implement:

https://lincolnloop.com/blog/rate-limiting-nginx/

@earl7399 https://github.com/earl7399 if you wanted to try and implement this, I think it could be done really easily in puppet by having it add a few lines to the nginx config.

try adding them to nginx.pp in magfest/ubersystem-puppet here:

location_cfg_append => {

something like: (you will have to modify the URL here)

location_cfg_append => { ...'limit_req_zone' => '\$binary_remote_addr zone=login:10m rate=1r/s;' }

once you do that, do a redeploy with './apply-node.sh localhost' and you should be able to test the limiting behavior locally and see it start rejecting your requests.

-Dom

— Reply to this email directly or view it on GitHub https://github.com/magfest/ubersystem/issues/1340#issuecomment-128118698 .

binary1230 commented 9 years ago

I think adding the time delay removes any effectiveness that the attack could succeed at all.

One site saying "how long would it take to crack your 6 character password"

6 characters: 2.25 billion possible combinations
Cracking online using web app hitting a target site with one thousand guesses per second: 3.7 weeks

One thousand guesses per second = 3.7 weeks. Rate limited to ONE per second = 74 years to crack a single password. And you only get 10 tries before it stops. Brute forcing only works when either 1) you have a really really really bad password like 'password' 2) you can run a ton of them per second i.e. thousands or millions because you're guessing with a local file and not over a server, and 3) you can try forever with no consequences.

rate limiting removes #2 and #3 while still keeping the usability. since usability here is an actual concern, and the security aspect isn't as much because we've eliminated this as an effective attack vector, the scale is way more in favor of keeping the usability.

thaeli commented 9 years ago

The proper medium/long term solution is changing over to two-factor Google accounts for Internet-accessible instances. Maybe even implement 2FA for at event if we really care to.

I would rather have the usability of telling them the account doesn't exist rather than obfuscating it. Rate limiting plus minimum password complexity solves the security issue fine in the short term, and it's a definite user confusion issue when we wont tell them if it's no account or no password. Besides, not like most of our logins are that hard to guess..

On Aug 5, 2015, at 5:34 PM, Dominic Cerquetti notifications@github.com wrote:

I think adding the time delay removes any effectiveness that the attack could succeed at all.

One site saying "how long would it take to crack your 6 character password"

6 characters: 2.25 billion possible combinations Cracking online using web app hitting a target site with one thousand guesses per second: 3.7 weeks One thousand guesses per second = 3.7 weeks. Rate limited to ONE per second = 74 years to crack a single password. And you only get 10 tries before it stops. Brute forcing only works when either 1) you have a really really really bad password like 'password' 2) you can run a ton of them per second i.e. thousands or millions because you're guessing with a local file and not over a server, and 3) you can try forever with no consequences.

rate limiting removes #2 and #3 while still keeping the usability. since usability here is an actual concern, and the security aspect isn't as much because we've eliminated this as an effective attack vector, the scale is way more in favor of keeping the usability.

— Reply to this email directly or view it on GitHub.

earl7399 commented 9 years ago

So if rate limiting worked, why doesn't Google do it? Or Stripe? Or any of a thousand sites I could name if I stayed up long enough who care about it? Because it is not a solution, it is a bandage. And not a great one at that.

However, I appear to be the only one in this group on my side of this, so I'll just wish you all well and back the fuck off.

Robert Earl r.a.earl@gmail.com FAX: 1-440-919-5086

On Wed, Aug 5, 2015 at 5:43 PM, thaeli notifications@github.com wrote:

The proper medium/long term solution is changing over to two-factor Google accounts for Internet-accessible instances. Maybe even implement 2FA for at event if we really care to.

I would rather have the usability of telling them the account doesn't exist rather than obfuscating it. Rate limiting plus minimum password complexity solves the security issue fine in the short term, and it's a definite user confusion issue when we wont tell them if it's no account or no password. Besides, not like most of our logins are that hard to guess..

On Aug 5, 2015, at 5:34 PM, Dominic Cerquetti notifications@github.com wrote:

I think adding the time delay removes any effectiveness that the attack could succeed at all.

One site saying "how long would it take to crack your 6 character password"

6 characters: 2.25 billion possible combinations Cracking online using web app hitting a target site with one thousand guesses per second: 3.7 weeks One thousand guesses per second = 3.7 weeks. Rate limited to ONE per second = 74 years to crack a single password. And you only get 10 tries before it stops. Brute forcing only works when either 1) you have a really really really bad password like 'password' 2) you can run a ton of them per second i.e. thousands or millions because you're guessing with a local file and not over a server, and 3) you can try forever with no consequences.

rate limiting removes #2 and #3 while still keeping the usability. since usability here is an actual concern, and the security aspect isn't as much because we've eliminated this as an effective attack vector, the scale is way more in favor of keeping the usability.

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/magfest/ubersystem/issues/1340#issuecomment-128158234 .