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

[SECURITY] Bitwarden browser extension leaks basic auth credentials #1124

Closed
h-town opened this issue Jan 26, 2020 · 20 comments
Closed

[SECURITY] Bitwarden browser extension leaks basic auth credentials #1124

h-town opened this issue Jan 26, 2020 · 20 comments

Comments

@h-town
Copy link

@h-town h-town commented Jan 26, 2020

While checking the NGINX logs on a machine hosting a site with HTTP Basic Auth I noticed a username which should not have been there. It was my own login but it was associated with the TLD site on hosted on a different machine, and here I was working with a subdomain.

After looking into it further I saw the extension--which was configured with default URL matching of "Base Domain"--was automatically volunteering credentials for the TLD, and doing so despite the fact that "Enable Auto-fill On Page Load" was disabled.

The Chrome and Firefox extensions were tested here and both exhibited the same behavior.

Although easing of HTTP Basic Auth in this manner may be viewed as a feature by some, it might feasibly compromise login data if targeted by bad actors. I'm sure there are several paths to exploit this but I've outlined 2 below:

  • User stores credentials for myhome.dynamicdns.tld and uses the default "Base Domain" configuration of BW browser extension. Nefarious Ned sets up badactor.dynamicdns.tld with a Basic Auth website, somehow coaxes User to visit, and BW attempts to login using credentials stored for myhome.dynamicdns.tld. User is not notified of this attempt and backs out of the login prompt thinking all is well.

  • User stores credentials for gmail.com and has the profile configured for "Begins With" matching. Vile Victor sets up gmail.com.badactor.tld with a Basic Auth website and somehow coaxes User to visit. Same scenario as above plays out here.

At the very least please discontinue automatic submission of credentials via Basic Auth if "auto-fill" is disabled. Ideally, and given many users may not understand the implications of this scenario, you'd remove the functionality outright.

@paudley
Copy link

@paudley paudley commented Jan 26, 2020

This is very useful functionality - please don't remove it. If needed, a config option for it maybe. I suspect most people expect their password manager to work in basic auth scenarios.

@h-town
Copy link
Author

@h-town h-town commented Jan 26, 2020

This is very useful functionality - please don't remove it. If needed, a config option for it maybe. I suspect most people expect their password manager to work in basic auth scenarios.

When properly deployed I don't disagree! In its current state I have zero doubt that credentials have been unintentionally submitted to the wrong parties before. Perhaps not for either of us but its definitely happened somewhere, and that's a problem. Has it been actively exploited? Who knows...

@paudley
Copy link

@paudley paudley commented Jan 26, 2020

@mritzmann
Copy link

@mritzmann mritzmann commented Sep 20, 2020

My suggestion:

  • Send the HTTP Authorization Header only if the URL including subdomain matches. For the automatic filling out of Basic Auth the default matching with "Base domain" should be overridden.
  • If auto-fill is switched off, this should not work.

@rugk
Copy link

@rugk rugk commented Sep 20, 2020

…or just do it like KeePassXC seems to do in it's recent versions: always open a popup asking whether to proceed.

@RobertMe
Copy link

@RobertMe RobertMe commented Sep 20, 2020

This might also be solved by changing "base domain" and using the public suffix list for this: https://publicsuffix.org/

This so that the extension knows that foo.dyndns.provider and bar.dyndns.provider are different domains under the same public suffix (dyndns.provider) and that credentials thus shouldn't be shared. This as this list for example is also used by browsers to determine if two domains are the same site to apply the SameSite cookie policy (so preventing cookies set on foo.dyndns.provider from being send to bar.dyndns.provider).

@rodalpho
Copy link

@rodalpho rodalpho commented Sep 20, 2020

No, the proper fix is to not send basic auth across subdomains at all. Don't match base domains, only full domains. Very simple. @mritzmann has it right.

Please do not restrict it to HTTPS only. I use basic auth in my LAN.

Also please don't open a popup. That's annoying.

@kspearrin
Copy link
Member

@kspearrin kspearrin commented Sep 20, 2020

If auto-fill is switched off, this should not work.

One problem here is that, due to limitations in the web extension APIs (see chrome.webRequest.onAuthRequired), filling credentials for HTTP Basic Auth prompts has to be non-interactive. I know of no other way to do it. Because of this, we do not respect the "auto-fill on page load" setting.

We could change the default match detection to Host on basic auth fills. That's not a bad idea.

@rugk
Copy link

@rugk rugk commented Sep 20, 2020

So whatever, there should of course be the possibility to disable that feature altogether. So any arguments like "That's annoying." or so don't count, because you can disable it.

No, the proper fix is to not send basic auth across subdomains at all. Don't match base domains, only full domains. Very simple. @mritzmann has it right.

Yeah, that's for a start may be a good idea, unless some users depend on that feature – you know.
Though even if that is restricted, you may not always like to automatically send this thing so hmm…

@rugk
Copy link

@rugk rugk commented Sep 20, 2020

credentials for HTTP Basic Auth prompts has to be non-interactive. I know of no other way to do it.

KeePassXC can do it, maybe look at their source https://github.com/keepassxreboot/keepassxc-browser/
I guess they may just cancel and re-request or do such tricks.

@rugk
Copy link

@rugk rugk commented Sep 20, 2020

Wait… your statement is just not true.
I just had a look on the docs https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/webRequest/onAuthRequired and it tells you many methods, which one is:

Provide credentials asynchronously

So technically that seems possible.

@kspearrin
Copy link
Member

@kspearrin kspearrin commented Sep 20, 2020

KeePassXC can do it, maybe look at their source https://github.com/keepassxreboot/keepassxc-browser/
I guess they may just cancel and re-request or do such tricks.

This seems to be because they have a native desktop app running that can open interactive UIs during the web request flow. Bitwarden works independently, only with available web extension APIs.

@kspearrin
Copy link
Member

@kspearrin kspearrin commented Sep 20, 2020

Changing the default match detection for basic auth fills has been addressed in #1397

@ottacom
Copy link

@ottacom ottacom commented Sep 21, 2020

I'm wondering if sounds reasonable from a UX perspective to disable the autofill and use the right click instead and one for all disable auto-login from the browser

@punkrokk
Copy link

@punkrokk punkrokk commented Sep 21, 2020

Yea this should be OPT IN

@LunNova
Copy link

@LunNova LunNova commented Sep 24, 2020

Having no way to disable this is unfortunate.
I have autofill off, credentials should never be automatically filled.
Is there still no way to turn this off?

Seeing collaborators appear to disregard this as a nonissue is very disappointing.

@rodalpho
Copy link

@rodalpho rodalpho commented Sep 25, 2020

They didn't ignore it, Kyle (BW's owner) addressed that here:

#1124 (comment)

Due to a technical issue, respecting the aut-fill setting for basic auth is not possible in an addon. Changing it to only match on full domains addresses the issue at hand.

@h-town
Copy link
Author

@h-town h-town commented Sep 25, 2020

They didn't ignore it, Kyle (BW's owner) addressed that here:

It was marked as an "enhancement" and still hasn't been addressed after 9 months and 11 releases...

@kspearrin
Copy link
Member

@kspearrin kspearrin commented Sep 25, 2020

resolved with #1397 for next release.

@LunNova
Copy link

@LunNova LunNova commented Sep 26, 2020

Due to a technical issue, respecting the aut-fill setting for basic auth is not possible in an addon.

It is possible - by not filling the password at all. Requiring me to manually copy/paste is better than autofilling that I never enabled.

Raised #1408

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants