keepassxc-browser icon indicating copy to clipboard operation
keepassxc-browser copied to clipboard

HackerOne TOTP is instead seen as password field

Open bwbroersma opened this issue 1 year ago • 5 comments

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: https://github.com/keepassxreboot/keepassxc-browser/blob/aa288ff797c0d9944734d7e08a6943b8715085d3/keepassxc-browser/content/fields.js#L23-L33

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

-        if (input.getLowerCaseAttribute('type') === 'password') {
+        if (input.getLowerCaseAttribute('type') === 'password' && !kpxcTOTPIcons.isAcceptedTOTPField(input)) {
  • However password was added as a negative check in #786 because of my complaining in #768.

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 avatar Sep 07 '24 11:09 bwbroersma

I've a PR for solution 1.

bwbroersma avatar Sep 07 '24 12:09 bwbroersma

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.

droidmonkey avatar Sep 07 '24 12:09 droidmonkey

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>

bwbroersma avatar Sep 07 '24 13:09 bwbroersma

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

droidmonkey avatar Sep 07 '24 13:09 droidmonkey

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 avatar Sep 07 '24 13:09 varjolintu