terra-money / station-mobile

🛰️ Station wallet mobile
https://apps.apple.com/it/app/terra-station/id1548434735
34 stars 61 forks source link

Account for Root Check False Positives #11

Closed gramx closed 2 years ago

gramx commented 2 years ago

I made this its own issue as it has to do with how the root check works and not about it being optional.

Please see the documentation for RootBeer under False Positives: https://github.com/scottyab/rootbeer#false-positives

Manufacturers often leave the busybox binary in production builds and this doesn't always mean that a device is root. We have removed the busybox check we used to include as standard in the isRooted() method to avoid these false positives. The following devices are known the have the busybox binary present on the stock rom:

  • All OnePlus Devices
  • Moto E
  • OPPO R9m (ColorOS 3.0,Android 5.1,Android security patch January 5, 2018 )

As far as I understand it, it looks like the implementation is not using the recommended practice.

https://github.com/terra-money/station-mobile/blob/4dfb12a28a5b029fa2f91cae9703fce1c9d3b7fc/android/app/src/main/java/money/terra/station/UtilLib/RootChecker.kt

file: station-mobile/android/app/src/main/java/money/terra/station/UtilLib/RootChecker.kt (line 21)

RootBeer(context).let {
                if (it.isRootedWithBusyBoxCheck) {
                    promise.resolve(true)
                }
            }

I am reporting this because I have a OnePlus phone that is not Rooted and it is not allowing me to use Terra Station. See attached screenshots. There are also some negative app reviews of people who say they are not rooted but get this error.

Screenshot_20220114-175735 Screenshot_20220114-042502

I would like to say, this looks like a great app! Keep up the good work. 👍

gramx commented 2 years ago

It should be noted that making this check optional would allow a workaround, though would not truly be a fix.

Another possible fix would be to make this check optional and also change the text to say "The device may be rooted. ...."

gramx commented 2 years ago

I made a pull request that should correct this issue, I would love some 👀s. #12

gramx commented 2 years ago

Thank you this issue is resolved as per the PR merge.