Fix 1623

[ATofighi]

#1623

[Sama34]

What is with this? :P

[JN-Jones]

Fixed some of the issues mentioned above (or not). This should be ready now

[Destroy666x]

What about failed login attempts? Haven't tested yet, but by looking at the code I don't think it works in this case.

[JN-Jones]

I've added the 2fa checks after all login/logout etc checks so it should work normally.

[JN-Jones]

I've renamed the directoy. I've also tested the lock/unlock feature and it works as expected.

The only thing to decide now is the recovery codes: IMHO it's okay if the same recovery code is generated twice.

[Sama34]

Well as long as it is not regenerated twice for the same user I suppose it is not a big deal.

[JN-Jones]

It may happen but it's very very unlikely. And no problem neither, that user has simply only 9 usable codes than (both occurrences will be deleted if the code is used).

[Destroy666x]

For the same user the fix is very simple though (so I don't see any reason to not include it), just change the for loop in the generate function to a while loop which checks the element count() in array and in the loop use !in_array().

Also, what if all codes were used someday and the user forgets to visit the page? Shouldn't the generate function be used after $ncodes becomes empty in do_fa action too (possibly with a success flash message saying that the user needs to visit the page with codes because they were regenerated - with a direct link)?

[JN-Jones]

It'd be useless to generate the codes and tell the user that he needs to visit the page where the codes are regenerated. But a normal message should be displayed, yes. Going to implement it later today.

About the same code: I can change the loop but IMHO it's so unlikely that the same code will be generated twice for the same user that we don't need to consider the case.

[JN-Jones]

Fixed both

[ATofighi]

I test with Authy and WinAuth and works good.

@JN-Jones I test lock out after X guesses for Authentication code but It doesn't work for me.
you should change https://github.com/JN-Jones/mybb/blob/fix-1623/admin/index.php#L213 and make some changes in https://github.com/JN-Jones/mybb/blob/fix-1623/admin/index.php#L538

[JN-Jones]

There isn't any limit of code guesses. It would require more changes as I'd also need to modify parts of the login handling. And tbh I don't think it's necessary.
If you guys think it should be implemented I'd rather implement a new counter instead of using the same counter as used for the login handing.

[ATofighi]

@JN-Jones I think this change can fix it: https://www.diffchecker.com/w2418f6b

[Sama34]

You can log any failing input too. Or send e-mails about it.

IMO 2fa and security codes should be treated just as passwords and blocked after X attempts, keep logging of those, and send notifications about it.

[JN-Jones]

@ATofighi you only need to select 2fasecret and from a quich look you'd need to do if(empty(... instead of !empty. Have you tested whether it works?

@Sama34 emailing when wrong codes are entered is pretty annoying imho and I've never seen a site doing that. Logging would be nice but I need to look how we're logging locked out users, probably it'd break that.

[ATofighi]

@ATofighi you only need to select 2fasecret and from a quich look you'd need to do if(empty(... instead of !empty. Have you tested whether it works?

you're right...

[JN-Jones]

I've modified your changes to work correctly. The lock out feature works now:

• Wrong passwords are counted
• Counter is reseted when password is correct and 2FA is disabled
• Counter increases when wrong 2FA code is used (3 wrong passwords and 2 wrong codes would result in 5 failed attempts)
• Main lock out code is copied
• Unlock works without 2FA (you need to login afterwards anyways)

[Sama34]

Some suggestions:

• There should be a regenerate button in the code generation page instead of regenerating on every page load. Browsers some time suck and it may be a situation where it reloads the page even when the user doesn't intend to do so. If this happens the user may not take note of the new codes.
• No logging for failing attempts were inserted, only the regular log that really show nothing.
• I think links to Google Authenticator and Authy should be included as well.

I used Google Authenticator for android BTW.

I'm now going to update my live board to test all the software.

[Sama34]

Does this asks for 2FA during the upgrade process BTW? I will check this later but it should.

[Destroy666x]

There should be a regenerate button in the code generation page instead of regenerating on every page load.

Definitely agreed.

Does this asks for 2FA during the upgrade process BTW?

IIrc the upgrade uses (well, not anymore, now only its parts) the front-end session/cookie system and there is no 2FA for front-end - we should still think about including it in 1.8.5.

[Sama34]

Didn't know it uses the front-end, interesting choice..

[JN-Jones]

There should be a regenerate button in the code generation page instead of regenerating on every page load. Browsers some time suck and it may be a situation where it reloads the page even when the user doesn't intend to do so. If this happens the user may not take note of the new codes.

I'm against this. I've only seen two things so far: either they're regenerated on every load or never. I've never seen a manual regenerate button (and tbh I doubt it has any use: who will ever regenerate the recovery codes?). And a browser shouldn't reload a page once it has been loaded completly.

No logging for failing attempts were inserted, only the regular log that really show nothing.

We don't log wrong passwords so we shouldn't log wrong codes.

I think links to Google Authenticator and Authy should be included as well.

Which one? Play Store (Android), iOS Store, Windows Store,....? The user should be smart enough to search in his app store. We can always include links in the docs and link to them. Would also allow us to maintain them easier.

[Sama34]

A link to a documentation page should suffice yes.

We don't log wrong passwords so we shouldn't log wrong codes.

But how do we diferenciate what was the incorrect input? Password/2FA

And a browser shouldn't reload a page once it has been loaded completly.

You sure? I have seen this on quite some sites already, and, "refresh to regenerate" is not user friendly at all.

[JN-Jones]

But how do we diferenciate what was the incorrect input? Password/2FA

Do we really need to know that? We could include the info probably in the locked out item but as said: logging wrong codes is useless imho.

You sure? I have seen this on quite some sites already,

Yep, sure. Otherwise all banking (or other things like that) would also screw up.

"refresh to regenerate" is not user friendly at all.

The whole 2FA thing isn't user friendly imho but it's security related.
Normally you should only access the codes page once (after activating 2FA), save the codes somewhere secure (best: print them) and never visit that page again until all codes are used. Probably even log whenever that page is accessed. But a manual reload button is a waste of time imho as nobody ever would click that.

A link to a documentation page should suffice yes.

Will add it later then.

[Starpaul20]

I've ran some tests and this works fine. My only suggestion is on the Recovery Codes page, there should be a 'Print This Page' link.

[Eldenroot]

@PaulBender - I agree with you! Or download into PC (.pdf)

[Stefan-MyBB]

@CU8ER Storing them on the PC is not the best idea in case you have malware on it.

[Eldenroot]

@stefan-st - yeah, you are right :) Anyway you can easily print in (or save it as pdf in print window). So just add "print this page" and it would be enough

[JN-Jones]

It's also pretty easy to get the file after it has been printed, most printer save them internally. Also I don't think a special link is necessary, you can easily print/save the codes without a special link.

[Starpaul20]

Looks good. If no one else fines any problems I'll merge this later today.

[ATofighi]

Looks good to me too.