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.

Sign up for GitHub

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

Jump to bottom

Wallet Security/PIN improvements #2574

Closed
nityas opened this issue Jan 28, 2020 · 9 comments · Fixed by #2648
Closed

Wallet Security/PIN improvements #2574

nityas opened this issue Jan 28, 2020 · 9 comments · Fixed by #2648
Assignees
@i1skn
Labels
feature Feature requests Priority: P1 Critical triaged label for issues that have been assigned a priority wallet

Comments

@nityas
Copy link
Contributor

nityas commented Jan 28, 2020

Expected Behavior

Prompt on App Open

Default Behavior: off

  • on opening the app (after setting a pin), the user should be prompted for their pin before being able to see home screen

  • add an item 'security' to the settings page with a toggle (similar to 'Analytics' section)
    "Require PIN on App Open"

Add to Safeguards

Additionally: Add a prompt for pin before viewing either safeguards or the backup key (this is not managed by the toggle)

Current Behavior

no prompt for pin on app open or safeguards
@nityas nityas added feature Feature requests Priority: P1 Critical wallet triaged label for issues that have been assigned a priority labels Jan 28, 2020
@i1skn i1skn self-assigned this Jan 29, 2020
@i1skn
Copy link
Contributor

i1skn commented Jan 29, 2020

Just to clarify a couple of things:

  1. PIN to unlock should be exactly the same as to send a transaction? If yes, do we want to cached it for next 5 min as we do in other cases?
  2. Because the initial behavior would be "off", do we want to somehow tell about this feature during/after the on-boarding?
  3. PIN prompt has been already added to "View backup key" and "View safeguards" some time ago. Does Additionally: Add a prompt for pin before viewing either safeguards or the backup key (this is not managed by the toggle) means to do something additional to that?

cc: @nityas. Thanks!

@nityas
Copy link
Contributor Author

nityas commented Jan 29, 2020 •
edited
Loading

@i1skn great questions!

PIN to unlock should be exactly the same as to send a transaction? If yes, do we want to cached it for next 5 min as we do in other cases?

Yes and Yes

Because the initial behavior would be "off", do we want to somehow tell about this feature during/after the on-boarding?

Good question -- I did a survey of other venmo-type apps and pins are disabled by default, in banking apps they tend to be enabled by default.

Thoughts on where we should fall? I'm not set on default off

PIN prompt has been already added to "View backup key" and "View safeguards" some time ago. Does Additionally: Add a prompt for pin before viewing either safeguards or the backup key (this is not managed by the toggle) means to do something additional to that?

I understood we punted it and I'm not seeing it in the latest build - are you prompted for your pin on the build running for you?

@i1skn
Copy link
Contributor

i1skn commented Jan 30, 2020

@nityas thanks for the detailed response!

Good question -- I did a survey of other venmo-type apps and pins are disabled by default, in banking apps they tend to be enabled by default.

Let's do it with "off" by default and iterate on that if we need to in the future.

I understood we punted it and I'm not seeing it in the latest build - are you prompted for your pin on the build running for you?

Yeah, that feature was merged into master on 4th Nov. See here: #1556

@i1skn
Copy link
Contributor

i1skn commented Feb 4, 2020

While I was working on this one I got some more questions to ask:

  1. When the app opens -> we ask for PIN, but what happens, if PIN is incorrect? Should we show the screen with a pin pad again? Do we need to set some timeout after a certain amount of wrong tries? @nityas
  2. If app was backgrounded, should we ask for PIN again when user foreground it? If yes - do we need to hide the screenshot of the app (which can have sensitive data on it) in app switcher? @nityas
  3. If user has chosen to use system PIN we can not enforce to ask for it again for the next N seconds (currently N=600), because we are setting this parameter during key the initialization. https://github.com/celo-org/react-native-confirm-device-credentials/blob/55c49dd99a2ecfe68d65ed6f2dec8db0054edb53/android/src/main/java/org/celo/devicecredentials/RNConfirmDeviceCredentialsModule.java#L115-L124. So, essentially we would either need to say no to caching of PIN if we would want to enforce user enter PIN every time app is open? @jmrossy please tell me what do you think, is this correct observation of mine?

@nityas
Copy link
Contributor Author

nityas commented Feb 4, 2020 •
edited
Loading

Great questions!

Yeah, that feature was merged into master on 4th Nov. See here: #1556

Interesting, could you confirm it works for you in the app itself? It's not for me on latest integration build, will file a bug.

When the app opens -> we ask for PIN, but what happens, if PIN is incorrect? Should we show the screen with a pin pad again?

Yes, let's go with this. I believe we already have an error message we display to the user if they enter the wrong PIN?

Do we need to set some timeout after a certain amount of wrong tries?

after 5 consecutive wrong tries can we set a timeout of 5m? I'd see this as something we make much more sophisticated over time, but overall we'd just want to make this challenging to brute force. Maybe in the future after ~20(should think through this exact number more) consecutive wrong tries we prompt for the seed phrase?

If this is something you're interested in, maybe we can start a design doc exploring our options here (probably not worth blocking on most of this but good to start thinking about it)

If app was backgrounded, should we ask for PIN again when user foreground it? If yes - do we need to hide the screenshot of the app (which can have sensitive data on it) in app switcher?

how hard is it to set up a timer? eg re-request if it's been > 5 mins. If this adds a lot of scope let's brainstorm alternatives -- better to re-request every time app is foregrounded again, the user can always disable this if they don't want it.

@i1skn
Copy link
Contributor

i1skn commented Feb 5, 2020

Interesting, could you confirm it works for you in the app itself? It's not for me on latest integration build, will file a bug.

Yes, it does work for me, but it has a couple of caveats:

  1. It does not work in forno mode
  2. When you are using Phone System Pin - it would only re-ask it once in 10 min

after 5 consecutive wrong tries can we set a timeout of 5m? I'd see this as something we make much more sophisticated over time, but overall we'd just want to make this challenging to brute force. Maybe in the future after ~20(should think through this exact number more) consecutive wrong tries we prompt for the seed phrase?

I would prefer to take this out of the scope of this issue and implement it later as a separate one.

how hard is it to set up a timer? eg re-request if it's been > 5 mins. If this adds a lot of scope let's brainstorm alternatives -- better to re-request every time app is foregrounded again, the user can always disable this if they don't want it.

I will hide the app last screen in the App Switcher, but it would only asks for a PIN again in 5-10 min after it was entered last time.

Let me know if that sounds good and I will go ahead!

@i1skn i1skn mentioned this issue Feb 5, 2020
Merged
@nityas
Copy link
Contributor Author

nityas commented Feb 5, 2020 •
edited
Loading

I would prefer to take this out of the scope of this issue and implement it later as a separate one.

Perfect- agreed on out of scope. Followup issue is here so we dont lose track: https://github.com/celo-org/celo-monorepo/issues/2649

I will hide the app last screen in the App Switcher, but it would only asks for a PIN again in 5-10 min after it was entered last time.

Sounds great 👍 Is there an exact timeout for the system PIN?

@annakaz
Copy link
Contributor

annakaz commented Feb 5, 2020

See also #2582

@i1skn
Copy link
Contributor

i1skn commented Feb 7, 2020 •
edited
Loading

Is there an exact timeout for the system PIN?

We are currently setting it to 10 min

@nityas nityas mentioned this issue Jul 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@i1skn i1skn

Labels
feature Feature requests Priority: P1 Critical triaged label for issues that have been assigned a priority wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Wallet] Lock the whole wallet with PIN celo-org/celo-monorepo
3 participants
@nityas @i1skn @annakaz