Skip to content
This repository was archived by the owner on Jan 19, 2021. It is now read-only.

bitbucket.org issue #17

Closed
532910 opened this issue Nov 28, 2019 · 28 comments · Fixed by browserpass/browserpass-extension#229
Closed

bitbucket.org issue #17

532910 opened this issue Nov 28, 2019 · 28 comments · Fixed by browserpass/browserpass-extension#229
Assignees

Comments

@532910
Copy link

532910 commented Nov 28, 2019

% pass otp IT/bugtrackers/bitbucket.org
231453

image

image

@erayd
Copy link
Collaborator

erayd commented Nov 28, 2019

Can you please paste the entire OTP line from that particular password file (obviously with the actual seed redacted)?

@erayd erayd self-assigned this Nov 28, 2019
@erayd
Copy link
Collaborator

erayd commented Nov 28, 2019

Never mind about posting the line - this appears to be the anti-phishing logic working as intended. The 2FA is on a completely different origin (bitbucket.org) than the actual login (id.atlassian.com).

Do you have any suggestions for how to proceed in this case? Dropping the token when the origin changes is a deliberate security measure.

I note also that the OTP extension does not clear the clipboard - so you can still paste the OTP code just fine.

/cc @maximbaz

@532910
Copy link
Author

532910 commented Jun 24, 2020

I loigin gitlab.gnome.org via github, but with OTP. And it also says no token available, looks like it's the same anti-phishing logic, isn't it?

@erayd
Copy link
Collaborator

erayd commented Jun 27, 2020

@532910 Can you clarify the workflow? I'm not sure where you're saying you are being prompted for OTP - as long as the password and TOTP entry are on the same origin, it should work. If they are on different origins (e.g. login on GitHub, but OTP on gitlab.gnome.org), then the phishing protection will discard the OTP seed, as that seed would belong to GitHub, not to gnome.org.

If you have a different OTP token available for the gnome.org origin, you can load it by performing any action with the gnome.org pass entry (e.g. copy username or password), after which the OTP token will be available until you navigate away from that origin.

@532910
Copy link
Author

532910 commented Jun 27, 2020

I can't reproduce gnome's gitlab issue more (:

@erayd
Copy link
Collaborator

erayd commented Jun 27, 2020

OK. I will close this issue for now then, but if you can reproduce it, please let me know.

@erayd erayd closed this as completed Jun 27, 2020
@532910
Copy link
Author

532910 commented Jun 27, 2020

Nope! This issue is about bitbucket and anti-phishing.

@erayd
Copy link
Collaborator

erayd commented Jun 27, 2020

@532910 Are you requesting this issue be reopened, or saying it's OK that it be closed?

I think the bitbucket thing is working as intended, and nobody posted anything indicating there was any action needed as regards that feature.

@532910
Copy link
Author

532910 commented Jun 28, 2020

I'm requesting this issue be reopened.

I think the bitbucket thing is working as intended.

I believe the anti-phishing protection is not necessary, and it'd be nice to have a control to disable it.

  1. browserpass-otp doesn't fill nothing, it only shows OTP
  2. if one OTP will leak into an other channel than password you won't be compromised
  3. browserpass has no anti-phishing protection, you can fill (and automatically send) any password on any website

@erayd
Copy link
Collaborator

erayd commented Jun 28, 2020

Anti-phishing protection is definitely necessary. Phishing of OTP codes on alternate domains is one of the main ways by which OTP-protected accounts are compromised (the other being SMS hijacking). It happens often, and is a vector of concern. Completely disabling all anti-phishing protection won't be happening; that is not negotiable.

  1. Correct. It also copies the code to the clipboard.

  2. OTP leaks are in fact a big vector for account compromises (of OTP-protected accounts).

  3. Browserpass has significant anti-phishing protection. See here for more details. It's possible to override it, but by default it won't even offer you entries that aren't on a matching origin unless you have used them on that origin before.

Can you describe what changes you would like to see happen, and how those changes would still mitigate the phishing risk?

@erayd erayd reopened this Jun 28, 2020
@532910
Copy link
Author

532910 commented Jun 28, 2020

by default it won't even offer you entries that aren't on a matching origin

I meant you always can do it manually just typing into the search field, and browserpass will not stop you

what changes you would like to see happen

show OTPs for those websites that has an otpauth record it the passwordstore
For example, bitbucket remembers be and trying to log-in (https://bitbucket.org/account/signin/) it asks OTP directly, without login/password step. I see nothing wrong to show an OTP that is stored in bitbucket.org passwordstore's record for the pages on bitbucket.org domain.

@erayd
Copy link
Collaborator

erayd commented Jun 28, 2020

I meant you always can do it manually just typing into the search field, and browserpass will not stop you.

It does stop you. You can't find a non-matching entry using the search field without first disabling the anti-phishing protection, unless you have already used the entry on the current origin before.

If you have discovered any scenario where this is not the case, please log a new issue for it so that we can fix it (except for chrome:// and about: URL schemes).

Re OTP - how do you propose that logic should work? Can you describe the flow of how you see that working?

Please correct me if I'm wrong, but it sounds like you are suggesting that Browserpass should proactively select and decrypt credentials without being prompted by the user. That is something we currently never do; decryption currently occurs only after the user requests it.

@532910
Copy link
Author

532910 commented Jun 28, 2020

You can't find a non-matching entry using the search field

I can find any password staying on any webpage, just backspace for the first.

it sounds like you are suggesting that Browserpass should proactively select and decrypt

Yep, my fault. I understand the current logic.

  1. Is it possible to move OTP button into Browserpass menu (leaving it as as separate extension of course):
    image

a. This will save the toolbar space.
b. It will be clear visible which OTP is requested.
c. OTP will not be shown on the screen.
d. This issue will be solved, as you can search any password and press OTP button (that is a user request for decryption)
e. This will allow to store OTP's in a separate wallet.

  1. Another way is to add the same search field to the current OTP button, but I don't like this idea.

@erayd
Copy link
Collaborator

erayd commented Jun 28, 2020

...just backspace for the first.

Pressing backspace when there is no text in the search field disables the anti-phishing protection. That's how it's meant to work; I think we may have been talking about the same thing this whole time 🙂.

  1. Can't do that unfortunately; the WebExtension API doesn't allow it. It's also a bit problematic, in that it's impossible to know which entries to show such a button for without first decrypting them.

  2. Would prefer not to do that; it's way too much duplication.

Do you have any other suggestions for a workflow that would solve your issue?

@532910
Copy link
Author

532910 commented Jun 28, 2020

we may have been talking about the same thing this whole time 🙂.

(:

It's also a bit problematic, in that it's impossible to know which entries to show such a button for without first decrypting them.

Nothing wrong to show it for all records, the same as password and login button are always shown even the record has no login or password. Think about it as a pass otp command, you can always say pass otp <name> but it doesn't mean you'll receive a result.

Can't do that unfortunately; the WebExtension API doesn't allow it.

It's a pity ): Is it possible to make an OPT button a part of browserpass, but keep all logic in browserpass-otp?

Do you have any other suggestions for a workflow that would solve your issue?

It seems like no.

@erayd
Copy link
Collaborator

erayd commented Jun 28, 2020

@532910 We have discussed the OTP issue again, and the usability concerns. We have agreed to move that functionality into the main extension, which should hopefully resolve your issue.

@apiraino
Copy link
Contributor

apiraino commented Jul 1, 2020

@532910 We have discussed the OTP issue again, and the usability concerns. We have agreed to move that functionality into the main extension, which should hopefully resolve your issue.

@erayd sorry I may have missed the conversation, but I thought that after this previous discussion, the decision was final.

cc: @maximbaz

@532910
Copy link
Author

532910 commented Jul 1, 2020

Decisions reconsidering is a sign of mind flexibility.

@maximbaz
Copy link
Member

maximbaz commented Jul 1, 2020

@erayd and I discussed it in a private chat, so you didn't miss any conversation. My personal attitude to storing both OTP tokens and passwords in a single basket (and assisting with this workflow) hasn't changed at all, however considering the following factors merging codebases simply becomes the most rational choice:

  • Browserpass does support OTP today anyway (yes, via another extension, but still integration exists, and we do react on issues and feature requests)
  • There are limitations to what we can do in a two-extensions world
  • Some work is duplicated between extensions
  • Not only do we have the code for OTP functionality itself, we find ourselves writing even more glue code to link the two extensions

I have a great respect for everyone contributing to this project, and I don't want to irrationally stand in a way. That's why I decided to change my mind and agree to merging the codebases with the following conditions:

  • We still disapprove usage of OTP and keep the explanation of why in README
  • It is disabled by default
  • We don't grow OTP functionality and keep it at the very minimum, meaning no fancy stuff like injecting OTP code in the page

Maybe @erayd will want to add something. But what do you guys think?

@erayd
Copy link
Collaborator

erayd commented Jul 1, 2020

Maybe @erayd will want to add something.

I think you covered it pretty well @maximbaz 🙂.

One of the notable points IMO is the lack of complaints and feature requests regarding the OTP extension since its inception. If people have been happy with the limited way that functionality works for over a year now, then that's a fairly strong indicator that the feature doesn't need to grow.

The integration brings the same behavior and capability that browserpass-otp extension currently does, and nothing more. However, being integrated means that it can be searched for and triggered without needing to invoke another activity (such as autofilling login credentials) first, which IMO is a viable solution to the primary issue raised in this thread.

Another point in favour of including OTP in the main extension is that, due to some other (unrelated) work that was already planned, we now have a logical home for it in the UI.

To be very clear, I agree strongly with @maximbaz that using a password manager for managing both passwords and 2FA weakens user security, and neither of us wish to encourage that behavior. The inclusion of OTP within the main extension is not an indication that either of us have relaxed our opinion regarding that issue, and the feature will not be growing in scope. We've simply agreed to change the manner in which we are addressing significant user demand for the feature.

@apiraino
Copy link
Contributor

apiraino commented Jul 1, 2020

thanks both @maximbaz and @erayd for the clarifications, I completely understand the technical reasons. As I am clearly using browerpass-otp in some contexts I can't complain :-) although some may argue that not having it might not be the same as having it disabled (maybe plausible scenarios where the browser.storage.local might be accessible to flip a switch and reactivate it).

Anyway, not the right place to further discuss this (sorry @532910 for hijacking your issue) but I kind of feel that some users will be surprised.

@erayd
Copy link
Collaborator

erayd commented Jul 1, 2020

...maybe plausible scenarios where the browser.storage.local might be accessible to flip a switch and reactivate it.

Ideally it's better that people do not store OTP secrets in their password manager at all - if the secret doesn't exist, then no amount of compromising the password manager could possibly obtain it. The option to enable / disable OTP in Browserpass is just a switch for the UI and a bit of internal processing; turning it off doesn't magically make storing your OTP secrets there a good idea (because Browserpass still has access to the decrypted contents of your pass entries, plus it's a single point of failure for both authentication factors).

The storing of OTP secrets in the same place as passwords is the security-compromising behavior we are trying to discourage. The risk is the same, regardless of which tool they use.

Some may consider that the security tradeoff is acceptable to them, and that's their choice. But we want to make sure, as much as we can, that their decision is a properly informed one. Too many people just see the convenience side, and don't actually consider the security implications of that convenience.

@erayd
Copy link
Collaborator

erayd commented Jul 1, 2020

I kind of feel that some users will be surprised.

Are you able to expand on this point a bit? Surprising users is generally not a good thing; it often indicates that one has designed a workflow, process or UX poorly. I would prefer not to surprise our users in such a way.

@apiraino
Copy link
Contributor

apiraino commented Jul 2, 2020

Are you able to expand on this point a bit? Surprising users is generally not a good thing; it often indicates that one has designed a workflow, process or UX poorly. I would prefer not to surprise our users in such a way.

Oh sorry, I mean not being surprised about the UX/UI (which I'm sure will be fine, some prototypes around here were looking great), but about the decision to merge the OTP feature in the main extension.
The "userbase" of browerpass was asked whether they wanted this feature or not, a couple of users shared their point of view and the consensus was to keep it outside the main extension, now one the next major release will ship it anyway. The technical reasons are sound but it's a U-turn that I'm afraid nobody is aware of. I discovered it reading this issue by chance, for example :-)
No big deal, I guess, but sometimes users just show up when there's something they don't like :-)

@erayd
Copy link
Collaborator

erayd commented Jul 2, 2020

@apiraino

...that I'm afraid nobody is aware of.

There is an on-upgrade popup that will display advising of the change. And I will push a notice in the OTP plugin as well.

... next major release will ship it anyway.

Not quite; there have been plenty of releases in the interim - that discussion was well over a year ago now. V3.6.0 will ship it. This isn't a breaking change, and therefore doesn't warrant a major version bump.

...sometimes users just show up when there's something they don't like.

True. Are you suggesting that users might dislike this change?

@apiraino
Copy link
Contributor

apiraino commented Jul 2, 2020

Are you suggesting that users might dislike this change?

Everything's fine for me but yes, some users might complain about (or find surprising) a feature it was first discussed then externalized to a side-extension and finally decided to be merged into the main extension.
I said "surprised" because I guess most users just don't follow issues in this repo, just update browserpass (sometimes automatically) and tomorrow they'll find out of the blue a popup saying something about OTP.

Most will probably just think "nice, now browserpass also manages OTP". And possibly: "what's an OTP token by the way?"

But hey that's just my two cents on the matter, I hope my being transparent with you is not taken as a negative knee-jerk reaction :-) I've always appreciated you guys spending unpaid time on browserpass and will keep using it anyway. Hope my point is clear.

@erayd
Copy link
Collaborator

erayd commented Jul 2, 2020

@apiraino You make some good points there.

@maximbaz What do you think - better to put the announcement only on the OTP extension? That way existing users will still know about it, but we won't be collecting any new recruits for the OTP cause by displaying an update popup to users who don't already use that stuff.

@maximbaz
Copy link
Member

maximbaz commented Jul 2, 2020

Good points indeed! Yes let's make notification only in the otp extension!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants