Not a bug TFA: Backup codes seem to be a security risk

[Kirby]

Affected version

2.2.12

When activating any two step authentication method, XenForo also generates a list of backups codes.

Those backup codes are stored as plaintext in the database and shown to the user when accessing account/two-step/backup/manage.

Storing those codes as plaintext in the database seems a security issue to me; if an attacker gets access to those codes, he can use them to log into accounts effectively bypassing stronger options (like TOTP) set up on accounts.

Therefore, backup codes IMHO should probably be treated like other secrets (=> password) as well and not be stored as plaintext; instead hashes should be stored.

Doing so would mean that the user can't view the backup codes after they have been generated, but that shouln't be a real issue:
There is no need for the user to view unused codes again after they have been generated; if he has lost access to the codes just generate new ones - used codes could still be displayed.

 

[Chris D]

While I agree, in principle, I think on balance hashing the values doesn't actually do much to add any additional protection.

Essentially backup codes are limited to 1 billion combinations. Computationally, if you have the hashes and a limited set of known combinations, it would probably take a matter of minutes or even seconds to brute force each one back to the original backup code.

So the question isn't really solely whether or not the codes should be hashed, it's whether they should be increased in length and/or complexity.

We could consider this in future, but we won't be taking that forward as a bug report, so a Suggestion should be made for future consideration.

 

[Kirby]

hashing the values doesn't actually do much to add any additional protection.

Essentially backup codes are limited to 1 billion combinations. Computationally, if you have the hashes and a limited set of known combinations, it would probably take a matter of minutes or even seconds to brute force each one back to the original backup code.

I am by no means a crypto expert, but plain text for sure requires zero effort to get all valid codes - bruteforcing a properly configured hash (preferably smth. like argon2id or scrypt) does require some effort to get one valid code, most likely more effort to get all valid codes.

Wether some effort to get one valid code vs. zero effort to get all valid codes is significant might be up to discussion though, at least to me this does sound significant.

So the question isn't really solely whether or not the codes should be hashed, it's whether they should be increased in length and/or complexity.

Ideally backup codes should

• Be more complex than ~ 30 bits of entropy
• Be stored as a proper hash
• Have an expiration date

We could consider this in future, but we won't be taking that forward as a bug report, so a Suggestion should be made for future consideration.

I'll file a suggestion, though I really feel smth. should be done here in a timely manner - at least by giving the admin an option to disable this insecure verification method.

 

[digitalpoint]

Storing those codes as plaintext in the database seems a security issue to me; if an attacker gets access to those codes, he can use them to log into accounts effectively bypassing stronger options (like TOTP) set up on accounts.

If an attacker has gained access to the database, there's a much bigger problem than leaked backups codes. Even if they were hashed, the attacker can simply change user's passwords and/or disable accounts to even need two-step authorization. Not the only reason, but the biggest reason you hash passwords is because users are stupid and like to use the same passwords on multiple sites (auto-generated backup codes don't have that issue). That being said, giving backup codes some options wouldn't hurt (being able to increase entropy/length as well an an option to completely disable them [and other two-step options, like your other suggestion]).

Personally, I'd love to be able to disable any two-step option, have a property designating each option as strong or not and as a bonus have xf_user_option.use_tfa be an integer instead of a boolean value (have it be the number of two-step options enabled). Then the permission where you force two-step to be used for certain user groups, have that be the minimum number of tfa's enabled. We already have this setting, but it's just a notice to the user rather than forced enforcement:



One of the biggest time sinks doing support for users is for users that have lost the one two-step option they setup. Forcing them to have multiple different options would be a time saver for me.

Oops, I got sidetracked.. haha