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

HackerOne TOTP is instead seen as password field #2332

Open
bwbroersma opened this issue Sep 7, 2024 · 5 comments · May be fixed by #2333
Open

HackerOne TOTP is instead seen as password field #2332

bwbroersma opened this issue Sep 7, 2024 · 5 comments · May be fixed by #2333
Labels

Comments

@bwbroersma
Copy link
Contributor

bwbroersma commented Sep 7, 2024

The current (far from ideal) HackerOne TOTP:

<input autocomplete="off" class="text-field__input" id="sign_in_totp_code" maxlength="6" name="user[totp_code]" type="password" value="">

Of course they should use autocomplete="one-time-code". However the /\btotp\b/ match in combination with maxlength=6, I think KeePassXC-browser should be enough hints to correctly detect TOTP here.

Expected Behavior

Detect TOTP field.

Current Behavior

The field is detected as password field.

Possible Solution

kpxcTOTPIcons.isAcceptedTOTPField(document.getElementById("sign_in_totp_code"))

is true, however because it's detected as a password field first, it seems the field is no longer detected as TOTP:

if (input.getLowerCaseAttribute('type') === 'password') {
const combination = {
username: (!usernameField || usernameField.size < 1) ? null : usernameField,
password: input,
passwordInputs: [ input ],
form: input.form
};
combinations.push(combination);
usernameField = null;
} else if (kpxcTOTPIcons.isValid(input)) {

So I tried adding an explicit !isAcceptedTOTPField on line 23:

-        if (input.getLowerCaseAttribute('type') === 'password') {
+        if (input.getLowerCaseAttribute('type') === 'password' && !kpxcTOTPIcons.isAcceptedTOTPField(input)) {

This results in two solutions:

  1. Remove password from ignoredTypes plus changing ignoreRegex to /(bank|coupon|postal|user|zip)((?!(\b|_)totp(\b|_)).)*code|comment|author|error/i (note: user.*code is probably to strict for negative, in this case it is user[totp_code] but I can also imagine user_mfa_code etc.).
  2. Adding an explicit allowRegex with strong indicators, e.g. /\b(totp|otp|2fa|mfa)\b/i, in which case other soft checks are not performed (e.g. ignoredTypes and ignoreRegex).

Steps to Reproduce (for bugs)

Enable 2FA on https://hackerone.com/ and sign in.

Debug info

KeePassXC - 2.7.9
KeePassXC-Browser - 1.9.3
Operating system: Linux x86_64
Browser: Mozilla Firefox 131.0

@bwbroersma
Copy link
Contributor Author

I've a PR for solution 1.

bwbroersma added a commit to bwbroersma/keepassxc-browser that referenced this issue Sep 7, 2024
@droidmonkey
Copy link
Member

droidmonkey commented Sep 7, 2024

That looks like a good solution to me. I would say, a field that obscures the text (ie a password field) should definitely be weighted more towards a password detection though. False positive hits for TOTP on password fields might be worse then the current state noted on this issue. You can also use custom fields for the website to overcome some auto detection faults.

Your change would need to be tested well on common sites to ensure no regression.

@bwbroersma
Copy link
Contributor Author

bwbroersma commented Sep 7, 2024

I agree with you both @droidmonkey and @varjolintu that this should be seriously tested.
I do see TOTP having the type=password more often nowadays, e.g. also on namesilo.com 2FA:

<label data-v-0a2585d0="" data-v-779d6dc5="" class=""><span data-v-0a2585d0="" data-v-779d6dc5="">2-Factor Code:</span><input data-v-0a2585d0="" data-v-779d6dc5="" type="password" maxlength="12" class="form-control" autocomplete="off"></label>

@droidmonkey
Copy link
Member

Geez and that one had a max length 12 too. So weird.

@varjolintu
Copy link
Member

I agree with you both @droidmonkey and @varjolintu that this should be seriously tested. I do see TOTP having the type=password more often nowadays, e.g. also on namesilo.com 2FA:

<label data-v-0a2585d0="" data-v-779d6dc5="" class=""><span data-v-0a2585d0="" data-v-779d6dc5="">2-Factor Code:</span><input data-v-0a2585d0="" data-v-779d6dc5="" type="password" maxlength="12" class="form-control" autocomplete="off"></label>

Usually type=password fields are masked and useless for TOTP input.

@varjolintu varjolintu removed the bug label Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants