Status-react: Sensitive data is shown in TF logs
Description
Type: Bug
Summary: we had an issue to hide user sensitive data from test fairy service logs https://github.com/status-im/status-react/issues/3876. Now this issue is still in place.
Expected behavior
Hide user sensitive data. Please see https://github.com/status-im/status-react/issues/3876 for more details.
Actual behavior
NOTES
probably it is introduced here https://github.com/status-im/status-react/pull/5521 but i am not sure. this PR is one of most recent i remember related to TF.
doesnt happen in https://status-im.slack.com/archives/C8RNGLKRT/p1534736586000100
Reproduction
- Open Status
- Recover account
- Check TF logs
asemiankevich
All 28 comments
yes, it was probably introduced by #5521. @rasom could you check?
lukaszfryc
on 21 Aug 2018
@lukaszfryc that commit upgraded react-native-testfairy to 2.14.1, the issue should have been fixed in2.10.0 https://github.com/testfairy/react-native-testfairy/issues/18#issuecomment-383931094
rasom
on 6 Sep 2018
@lukaszfryc @asemiankevich is this still an issue?
chadyj
on 15 Oct 2018
@chadyj i dont think so but worth checking. @antdanchenko this is covered with e2e right?
asemiankevich
on 15 Oct 2018
@asemiankevich we can't easily check TF in e2e. We would need to have a proxy to intercept http trafic
lukaszfryc
on 15 Oct 2018
@chadyj @asemiankevich I will check it today or tomorrow morning
lukaszfryc
on 15 Oct 2018
@yenda Just verified that passphrase are visible in TestFairy.
We need to fix this asap.
@rasom Can you please take a look at it?
chadyj
on 15 Oct 2018
If TestFairy is causing the mnemonic to be leaked is it about time we got rid of this? Instabug too? Let's reduce the surface area for issues like this.
This means that anyone with TestFairy access can see any nightly users mnemonic.
cc @oskarth @corpetty @arnetheduck @mandrigin @annadanchenko
chadyj
on 15 Oct 2018
@corpetty this is pretty big we should delete all the testfairy sessions and warn users that they'd better create a new account if they used a nightly build
yenda
on 15 Oct 2018
yeah. any information from testfairy we have should be deleted, and we should let people know that if they used a nightly they should create a new account and transfer whatever funds they had.
corpetty
on 15 Oct 2018
I'd like to know:
- who has access to this information within status?
- how long has this been a problem?
- what information are we capturing with testfairy (seed, passphrase, both )
- is "deleting sessions" within testfairy actually getting rid of it on their side too?
- why do we need testfairy in the first place?
- can we control within testfairy what information is captured?
corpetty
on 15 Oct 2018
As these are nightlies... it is unlikely that people are storing large amounts of value within the app. that being said, it is our job to be transparent and clear about what we have and the course of action to move whatever funds are there to a private account. If we continue to use services like this, then a modal should warn users that this is a nightly build, and any serious funds shouldn't be stored within the app.
corpetty
on 15 Oct 2018
I believe this is has been a known and deliberate feature for quite some time, not sure why the sudden panic and reversal. It really shouldn't be news to anybody by now that a product that records screens and shows a complete list of interactions also occasionally will record passwords and other unintended details, such as private conversations.
It used to be that nightlies showed a big fat warning, can't remember if it was on install or somewhere else.. is this no longer the case?
Though I personally disagree, previous discussions on the subject have always landed in this being a reasonable trade-off to reach a more polished product faster.
I'm generally uncomfortable with having this kind of code present in the build system at all, because, as noted, it increases attack surface and the possibility that something goes wrong, even in non-nightlies. As the classic goes, anything that can go wrong, also will go wrong, and developers will inevitably keep forgetting to add sensitive stuff to the blacklist - the default of including every event encourages mistakes. We're also a boolean away from this being accidentally added to a production build.
Our dependency list is massive - I'm not sure addressing testfairy in isolation is the right way to go, rather I would think that a general inventory and review of dependencies would be better. Likewise, we regularly perform unaudited/random upgrades to these dependencies that cause general breakage. Fortunately, our application is not wide spread enough to make it a target for slightly more sophisticated attacks, the kind that for instance would introduce a key-logger in a point-upgrade to dependency number 327 and go unnoticed past all code reviews and multiple-signatories-of-the-official-release-schemes. One could argue like Visa and say that this is a reasonable thing to do, on the assumption that paying damages is cheaper than addressing the problem.
who has access to this information within status?
everyone, and the rest of the internet also, as soon as someone publishes a link like this while discussing the bug: https://app.testfairy.com/projects/4803590-status/builds/8112310/sessions/12/?accessToken=sjjlm6BzJwjorBeKojPlAkystK4
is "deleting sessions" within testfairy actually getting rid of it on their side too?
what does history teach you?
arnetheduck
on 16 Oct 2018
I'd personally like to see both TestFairy and Instabug removed completely, as they are not essential to the functioning of the app and opens up attack surface etc. The reasoning is the same as Mixpanel. Last time this was brought up there was some pushback, but perhaps people are coming around now?
There are other such dependencies that require some more careful thought, for example Firebase which is necessary to receive push notifications. We explicitly (hackily) removed the Analytics part of that library back when.
oskarth
on 16 Oct 2018
On the transparency front, as we've been pushing people to install nightly with TF on by default, I think it would be a good idea to mention this in next release notes / once we address this issue.
oskarth
on 16 Oct 2018
general inventory and review of dependencies would be better.
Agree, this has come up a few times. Adding to wall of shame, should be explicit goal and in our face.
https://discuss.status.im/t/principles-seminar-session-4-security/557/2
Status.im
general inventory and review of dependencies would be better. Wall of shame: lack of clear understanding and rationale of dependencies, leading to a big and haphazard surface area See https://github.com/status-im/status-react/issues/5567 and 3rd party HTTP services
oskarth
on 16 Oct 2018
Thanks for the response. Here are a few additional items for clarification:
I believe this is has been a known and deliberate feature for quite some time, not sure why the sudden panic and reversal
It was known that TF records video. It was unknown that the seed phrase was recorded.
The severity is that all users of nightly builds have had their seed phrase recorded and uploaded to Testfairy's servers. These recordings are made available to those at Status and potentially others. This was previously not the case, and not the intention. Furthermore, this has been happening for several weeks or longer.
It used to be that nightlies showed a big fat warning, can't remember if it was on install or somewhere else.. is this no longer the case?
Here is the warning:
You are using an app installed from a nightly build. If you're connected to WiFi, your interactions with the app will be saved as video and logs. **These recordings do not save your passwords**. They are used by our development team to investigate possible issues and only occur if the app is install from a nightly build.
We explicitly tell users that their passwords are not saved in this disclaimer.
We also inform users about TestFairy in our privacy policy, but again there is no mention of the seed phrase being recorded.
We are lying to our users.
The bottom line is that users of the nightlies, including most of Status, have had their seed phrase exposed.
chadyj
on 16 Oct 2018
Immediate action points:
- Remove TestFairy. https://github.com/status-im/status-react/issues/6330 - PR
- Create blog post informing users. I've pinged Jonny to help coordinate
- Delete TF sessions. @annadanchenko ~can you help with this?~ Done
- Delete TF account @annadanchenko to do
chadyj
on 16 Oct 2018
@chadyj all TF sessions are deleted
annadanchenko
on 16 Oct 2018
@chadyj all TF sessions are deleted
Thanks @annadanchenko
Although I see there are 40 new sessions. Can we delete the account as a permanent fix?
chadyj
on 16 Oct 2018
Or maybe there is an option to disable/disconnect the API key somewhere in account settings that we could do first. And then ask TF team if they can pernamently remove all the data (including backups).
lukaszfryc
on 16 Oct 2018
@chadyj @lukaszfryc I'm checking with TF support as regenerating aPI key does not help. ofc, new sessions are recorded as we didn't disable TF in the old builds and can't do this.
annadanchenko
on 16 Oct 2018
state for today: app token for TF is regenerated and TF is removed from the app, so no new sessions are created. I'm not deleting TF account yet as I want to get confirmation from TF CustomerSupport about backups being deleted.
annadanchenko
on 17 Oct 2018
I'm curious - with testfairy gone, how was our dependency graph affected? was testfairy a single-library dependency or did the removal take any other libraries with it? @rasom ? did the app become any smaller as a result?
arnetheduck
on 18 Oct 2018
@arnetheduck still 1868 packages
yenda
on 18 Oct 2018
Removed Testfairy from the privacy policy.
https://www.iubenda.com/privacy-policy/45710059
Instabug removal PR is complete and being reviewed
https://github.com/status-im/status-react/pull/6359
A post-mortem is being done for this incident.
https://discuss.status.im/t/post-mortems-learning-from-our-mistakes/581
Closing this issue since TF is now removed.
chadyj
on 18 Oct 2018
@arnetheduck well, from what I see apk size is pretty much the same, around 43MB
rasom
on 18 Oct 2018
@arnetheduck well, from what I see apk size is pretty much the same, around 43MB
either we're doing something wrong or testfairy is doing something right :) interesting
arnetheduck
on 22 Oct 2018
Related issues
annadanchenko
路
3Comments
jarradh
路
4Comments
flexsurfer
路
3Comments
oskarth
路
4Comments
yevh-berdnyk
路
4Comments
Most helpful comment
I believe this is has been a known and deliberate feature for quite some time, not sure why the sudden panic and reversal. It really shouldn't be news to anybody by now that a product that records screens and shows a complete list of interactions also occasionally will record passwords and other unintended details, such as private conversations.
It used to be that nightlies showed a big fat warning, can't remember if it was on install or somewhere else.. is this no longer the case?
Though I personally disagree, previous discussions on the subject have always landed in this being a reasonable trade-off to reach a more polished product faster.
I'm generally uncomfortable with having this kind of code present in the build system at all, because, as noted, it increases attack surface and the possibility that something goes wrong, even in non-nightlies. As the classic goes, anything that can go wrong, also will go wrong, and developers will inevitably keep forgetting to add sensitive stuff to the blacklist - the default of including every event encourages mistakes. We're also a boolean away from this being accidentally added to a production build.
Our dependency list is massive - I'm not sure addressing testfairy in isolation is the right way to go, rather I would think that a general inventory and review of dependencies would be better. Likewise, we regularly perform unaudited/random upgrades to these dependencies that cause general breakage. Fortunately, our application is not wide spread enough to make it a target for slightly more sophisticated attacks, the kind that for instance would introduce a key-logger in a point-upgrade to dependency number 327 and go unnoticed past all code reviews and multiple-signatories-of-the-official-release-schemes. One could argue like Visa and say that this is a reasonable thing to do, on the assumption that paying damages is cheaper than addressing the problem.
everyone, and the rest of the internet also, as soon as someone publishes a link like this while discussing the bug: https://app.testfairy.com/projects/4803590-status/builds/8112310/sessions/12/?accessToken=sjjlm6BzJwjorBeKojPlAkystK4
what does history teach you?