Biometric prompt, Jailbreak / Root detection and screenshot prevention #8645
Conversation
…onfig type definition
enahum
added
2: Dev Review
enahum
requested review from
larkox,
jwilander,
yasserfaraazkhan and
harshilsharma63
| promises.push(promise); | ||
| } catch (error) { | ||
| logError('initializeSecurityManager', error); | ||
| continue; |
There was a problem hiding this comment.
Would this allow the app to bypass security restrictions because it faced some arbitrary issue in obtaining database or config?
There was a problem hiding this comment.
in theory, yes.. but here is the deal.. you only get to this point when the app is capable of showing the home screen, this means that the database is there and the config too, but the code is being defensive and capable of adding the error in the logs in case that happens.
There was a problem hiding this comment.
And in that case would I able to access the server?
There was a problem hiding this comment.
yes, cause by default there is no restriction
| logError(`Switch to Server with url ${serverUrl} not found`); | ||
| return; | ||
| } | ||
| if (server.lastActiveAt) { |
There was a problem hiding this comment.
Is this for allowing at least logging into the server from a jailbroken device? Can you please add a comment explaining the purpose of this check?
There was a problem hiding this comment.
server.lastActiveAt means that is an existing session on that server and is not a server that was logged out.
for example, if you have two servers in your app but logout from one of them and you want to connect to it again, it will return lastActiveAt as 0 meaning it does not hit this condition, if the server has an active session, then it will perform the necessary checks before allowing you to switch to it.
| /** | ||
| * Checks if the device is Jailbroken or Rooted. | ||
| */ | ||
| isDeviceJailbroken = async (server: string, siteName?: string) => { |
There was a problem hiding this comment.
All the usage of isDeviceJailbroken in this PR that I can find do not pass siteName. So, whats the purpose? Can you add docs explaining it and how it affects the result of this function?
There was a problem hiding this comment.
sorry, I'm fixing this.. this should have siteName set when connecting to a new server that has this setting enabled.
The reason to use siteName is to be able to go through the check without having to add the server security config into memory if you are unable to conform with the security policies before connecting. and the Site name is being used in the Alert box that is displayed in case of failure.
|
|
||
| const locale = DEFAULT_LOCALE; | ||
| const translations = getTranslations(locale); | ||
| if (config?.JailbreakProtection || siteName) { |
There was a problem hiding this comment.
I don't know the reason behind this, but if JailbreakProtection is disabled, why do we still check for it?
| return; | ||
| } | ||
|
|
||
| const isActive = appState === 'active'; |
There was a problem hiding this comment.
nit - create constants for these states.
There was a problem hiding this comment.
no need, this are protected by typescript set by AppStateStatus which is defined as type AppStateStatus = "active" | "background" | "inactive" | "unknown" | "extension"
| if (this.activeServer) { | ||
| const config = this.getServerConfig(this.activeServer); | ||
| if (config && config.Biometrics && isMainActivity()) { | ||
| const authExpired = this.backgroundSince > 0 && (Date.now() - this.backgroundSince) >= toMilliseconds({minutes: 5}); |
There was a problem hiding this comment.
Even if the app is in background for >5 mins, I can still see the push notifications, respond to messages via notification quick replies and potentially even accept incoming calls, all without opening Mattermost, right? I don't know what action sends the app into 'active' state so I might be wrong. Worth checking and documenting.
There was a problem hiding this comment.
well you could most definitely reply to notifications without opening the app and this PR does not cover the case when sharing content through the share extensions...
But we won't be covering those cases, at least not now.. for the notifications though, the user can set the device to not show the notifications content or actions if not authenticated, but this in fact a user setting on their own device and not controlled by us, not sure if we can prompt for any sort of biometric authentication before sending a reply directly from the notification.
Joining a call on the other hand, you do need to open the app.
The active state is set when the app is in the foreground.
There was a problem hiding this comment.
Got it, far enough. As long as we know and communicate the limitations, its fine. This will anyways go through security team as well.
| const messages = defineMessages({ | ||
| not_secured_vendor_ios: { | ||
| id: 'mobile.managed.not_secured.ios.vendor', | ||
| defaultMessage: 'his device must be secured with biometrics or passcode to use {vendor}.\n\nGo to Settings > Face ID & Passcode.', |
There was a problem hiding this comment.
Typo - 'his device must be -> 'This device must be
|
@enahum where can I grab an iOS build for this to try out locally? This is very exciting! |
you have two options
|
| @@ -269,6 +270,7 @@ const ChannelBookmarkAddOrEdit = ({ | |||
| edges={edges} | |||
| style={styles.content} | |||
| testID='channel_bookmark.screen' | |||
| nativeID={SecurityManager.getShieldScreenId(componentId)} | |||
There was a problem hiding this comment.
This seems something we will need to add for every screen we create from now on. Is there any way to enforce it (by linting rule, for example)?
Or what about to add a wrapper directly on the screen builder like we have withDatabase and so on?
There was a problem hiding this comment.
the thing is that not necessarily this is applied in the screens folder but it should be somewhere in the component tree of a screen, although the changes in the emm library have a fallback in case none is found.
Also, a HOC may not be what we want cause is not always the first "View" that has this id
There was a problem hiding this comment.
Other than what @larkox mentioned and communicating the known limitations with security team and customers, it all looks great! Hopefully we can have something similar for webapp when an admin access system console, like ask for a PIN or something.
yasserfaraazkhan
added
the
Build Apps for PR
yasserfaraazkhan
added
Build Apps for PR
|
Mattermost mobile is absolutely broken beyond belief and super slow and sluggish on a flagship Android phone and I come here to see you are investing resources that would be better spent fixing the app into making rooted users', such as myself, life harder. Words cannot describe how disappointed I am. |
amyblais
added
the
CherryPick/Approved
yasserfaraazkhan
added
Build Apps for PR
Summary
Added a security manager class to control the new security features introduced on a per-server level in addition to the app level managed by EMM providers.
This features are:
These features will work on a per-server basis only if not set with a EMM provider.
In order to prevent screenshots on iOS (the way it works is detailed in the library PR) we are adding a
nativeIDto views on screens and components whereshieldedmeans that it should be protected andskip.shieldedmeans that we won't be protecting this view (for screens like the ones in the login flow)Related PR's:
Server: mattermost/mattermost#30411
Library: mattermost/react-native-emm#18
Ticket Link
https://mattermost.atlassian.net/browse/MM-63362
https://mattermost.atlassian.net/browse/MM-63363
https://mattermost.atlassian.net/browse/MM-63364
Checklist
E2E iOS tests for PR.Device Information
This PR was tested on:
Android 14 & 15 emulators
iPhone 16 Pro and iPad 13 Pro iOS 18
Screenshots
Will add if requested
Release Note