metamask-mobile icon indicating copy to clipboard operation
metamask-mobile copied to clipboard

[FEAT] Screenshot Warning

Open gantunesr opened this issue 3 years ago • 8 comments

Description

Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions, 1. What is the reason for the change?

Improve the security around SRP reveal.

2. What is the improvement/solution?

iOS specific. This PR adds a warning when the user takes a screenshot of their SRP or Private Key.

Changes:

  • [ ] ~Introduction of react-native-detector v0.2.1 package~
  • [x] Refactor of RevealPrivateCredential and move styles to a different file
  • [x] Refactor of ManualBackupStep1 and move styles to a different file

it also refactors the component RevealPrivateCredential and ManualBackupStep1

Screenshots/Recordings

If applicable, add screenshots and/or recordings to visualize the before and after of your change

https://user-images.githubusercontent.com/17601467/178645859-b41bff01-b25b-4fa3-8d74-39a326c4894a.MP4

Note: You can observe a similar behaviour in the onboarding flow.

Issue

Progresses https://github.com/MetaMask/mobile-planning/issues/227

Checklist

  • [x] There is a related GitHub issue
  • [x] Tests are included if applicable
  • [ ] Any added code is fully documented

gantunesr avatar Jul 13 '22 03:07 gantunesr

Is the goal of this change to warn the user before they take a screenshot or simply to warn them whenever they take one? Would it even be possible to prevent a screenshot when the user takes one?

https://user-images.githubusercontent.com/22918444/180300751-2862a9b2-edfb-4bc7-aae0-227208c20eef.MOV

owencraston avatar Jul 21 '22 19:07 owencraston

@owencraston in response to https://github.com/MetaMask/metamask-mobile/pull/4670#issuecomment-1191861490

The goal is just to warn the user about their action and state explicitly that we don't recommend it. In iOS you can't prevent the screenshot.

gantunesr avatar Jul 24 '22 19:07 gantunesr

@gantunesr thanks for clarifying. I see how that it's not possible on iOS. I am curious how we went about disabling screenshots on Android? is it something we disable only on one screen or is it app wide? if we are disabling it on one screen then we should try and consolidate the logic since these packages allow us to disable screenshots on android.

owencraston avatar Jul 27 '22 21:07 owencraston

@owencraston regarding https://github.com/MetaMask/metamask-mobile/pull/4670#issuecomment-1197365885

I am curious how we went about disabling screenshots on Android?

We created a native module to disable it. You can check it here

Is it something we disable only on one screen or is it app wide?

Only SRP or Private Key related screens

if we are disabling it on one screen then we should try and consolidate the logic since these packages allow us to disable screenshots on android.

I support this decision, but it would be out of scope for this development

gantunesr avatar Jul 28 '22 12:07 gantunesr

if we are disabling it on one screen then we should try and consolidate the logic since these packages allow us to disable screenshots on android.

I support this decision, but it would be out of scope for this development

Looking at the expo package screen capture package, it seems like we could easily leverage this package to do both the screenshot prevention and the screenshot listener.

Also, slightly unrelated but it seems we can prevent screen recording on iOS for certain screens. Screen Shot 2022-07-28 at 11 14 51 AM

owencraston avatar Jul 28 '22 15:07 owencraston

@gantunesr I did a quick run through to see how this feature works and I received a [TypeError: undefined is not an object (evaluating '_this.props.passwordSet')] when I attempted to reveal my SRP.

cortisiko avatar Aug 12 '22 22:08 cortisiko

Socket Security Pull Request Report

👍 No dependency changes detected in pull request

Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

Powered by socket.dev

socket-security[bot] avatar Sep 05 '22 23:09 socket-security[bot]

According to socket.dev, this dependency was flagged as deprecated and has some high severity issues like install scripts. I have to take another look at this development.

Screen Shot 2022-09-16 at 13 08 00

Source

gantunesr avatar Sep 16 '22 16:09 gantunesr

QA Build 972 crashes on Android 13, 12 and 11 when I attempt to access SRP with a device that uses PIN authenticaton with NO biometric auth factors present on the device. I also noticed that when I attempt to reveal the private key, I get "Incorrect Password" even though I am using the correct password (not shown in video, but also easy to reproduce)

https://recordit.co/Uh75WoiAem

IOS 15.6.1 (edited to add iOS 16 shows this same error) using a device with biometrics present (but not used by MetaMask) I get this error on QA build 972 when I attempt to reveal SRP:

plasmacorral avatar Oct 04 '22 21:10 plasmacorral

Copy updated according to https://github.com/MetaMask/mobile-planning/issues/227#issuecomment-1307108805 in commit e951e16

gantunesr avatar Nov 08 '22 14:11 gantunesr

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

github-actions[bot] avatar Nov 24 '22 19:11 github-actions[bot]