Skip to content
This repository has been archived by the owner on Sep 6, 2019. It is now read-only.

Remove salt setting from database #1661

Closed
an0n981 opened this issue May 15, 2014 · 48 comments
Closed

Remove salt setting from database #1661

an0n981 opened this issue May 15, 2014 · 48 comments

Comments

@an0n981
Copy link
Contributor

an0n981 commented May 15, 2014

Do to security concerns I request that the salt entry be removed from the database. In addition to the obvious concern regarding my device ID (not sure how easy it is for apps to access /data/system), I assume also that changing this manually could enable a malicious dev to easily circumvent the restrictions upload policy and influence the crowd source settings for his/her app. If that is not possible or easily realized, then I think it should at least not be included in the xml exports to prevent apps with storage access from reading this data.

@M66B
Copy link
Owner

M66B commented May 15, 2014

/data/system is very secure, basically only Android can access this folder, although this also depends on the file permissions. The file permissions of the database are quite strict:
https://github.com/M66B/XPrivacy/blob/master/src/biz/bokhorst/xprivacy/PrivacyService.java#L1778

The private application data folder is a less secure place to store this information.

Only root applications should be your concern regarding this information.

It must be included in the export file to make some settings in the export file readable/usable.

@M66B M66B added the question label May 15, 2014
@an0n981
Copy link
Contributor Author

an0n981 commented May 15, 2014

For example when importing on a different device? That is something I didn't consider.

@M66B
Copy link
Owner

M66B commented May 15, 2014

Except for taking an md5 of the android_id for use with the crowd sourced restrictions, the salt is being used to take an sha1 of the account information (e-mail!) before storing/using the account information, so that your e-mail address is not visible when storing settings about accounts.

@an0n981
Copy link
Contributor Author

an0n981 commented May 15, 2014

Since accounts can only be imported on the same device, couldn't this instead be queried from the system when needed?

@M66B
Copy link
Owner

M66B commented May 15, 2014

Image this scenario (others are thinkable): export settings, wipe system, restore Android ID, accounts, etc (for example using Titanium backup), import settings. Importing account settings will not work without salt ...

@an0n981
Copy link
Contributor Author

an0n981 commented May 15, 2014

Correct me if I wrong, but isn't the salt the SerialNumber (hard coded to the device) and not the AndroidID?

@M66B
Copy link
Owner

M66B commented May 15, 2014

The salt is just a random value, which XPrivacy only generates once, that is concatenated to the strings which are md5 or sha1 hashed (android_id and account info) to make it more difficult to reverse the hash. So, it is not something personally identifying, but it is personal in the sense that you only have that value. It is never exposed, also not while submitting restrictions, which is the whole point: even if the crowd sourced restrictions database gets hacked, it is virtually impossible to reverse the hashed android_id's.

@an0n981
Copy link
Contributor Author

an0n981 commented May 15, 2014

That this is a from XPrivacy randomly generated ID doesn't quite add up. I knew I had seen that ID somewhere outside of XPrivacy before, til just I couldn't remember where that was. TWRP uses this same ID as a folder name to store nandroid backups. Which is why I noticed this in the db in the first place.

@M66B
Copy link
Owner

M66B commented May 15, 2014

Looking it up, it seems you are right, Build.SERIAL is being used as salt. I just remember it differently.

Maybe it should be changed to a random string, but I have to think about the consequences and it would only be for new users.

@an0n981
Copy link
Contributor Author

an0n981 commented May 15, 2014

I just manipulated the crowd db twice in with very liitle effort (Not sure if you would be able to tell, but have a look at LightningWall entries). IMHO this should probably continue to use the BUILD.Serial since this can't be restricted for XPrivacy, but I think it should be queried when needed instead of being saved in the db where it can easily be altered.

@M66B
Copy link
Owner

M66B commented May 16, 2014

Did you restart after changing the setting? Did you get the device activation prompt?

@M66B
Copy link
Owner

M66B commented May 16, 2014

I will remove the setting from the database in a next version.

@M66B M66B added enhancement and removed question labels May 16, 2014
@M66B M66B changed the title [Request] Salt Setting Remove salt setting from database May 16, 2014
@an0n981
Copy link
Contributor Author

an0n981 commented May 16, 2014

The only thing I did was close the XPrivacy UI while making the switch. I didn't receive any prompts.

@M66B
Copy link
Owner

M66B commented May 16, 2014

Thanks!

@M66B M66B closed this as completed in 89f3a23 May 17, 2014
M66B pushed a commit that referenced this issue May 17, 2014
This reverts commit 89f3a23.

Conflicts:
	CHANGELOG.md

Closes #1664
Refs #1661
@M66B M66B reopened this May 17, 2014
M66B pushed a commit that referenced this issue May 28, 2014
@M66B
Copy link
Owner

M66B commented May 28, 2014

Could you please test if this version works before and after deleting the salt from the database:
http://www.xprivacy.eu/XPrivacy_2.0.25-4.apk

To be tested:

  • Allowed accounts
  • Export/import allowed accounts on the same device
  • Submit/fetch restrictions

@an0n981
Copy link
Contributor Author

an0n981 commented May 28, 2014

Just be sure:
You want me install, test, manually delete salt, test again?

@M66B
Copy link
Owner

M66B commented May 28, 2014

Yes, if you want.

@an0n981
Copy link
Contributor Author

an0n981 commented May 28, 2014

Of course, I am here to help

@M66B
Copy link
Owner

M66B commented May 28, 2014

Which is appreciated 👍

@an0n981
Copy link
Contributor Author

an0n981 commented May 28, 2014

Submit/fetch, export/import worked fine. Allowed accounts worked before deleting, but not after. Here a log of opening GitHub after deleting the salt. https://mega.co.nz/#!gp4lUTZJ!7rS_vSt63ulZvhooRfywBsRfi1WaS83KsLaIj-CFTc8

@an0n981
Copy link
Contributor Author

an0n981 commented May 28, 2014

Could it that there is no ro.build.serial available in the getprop command, but instead only ro.serialno and ro.boot.serialno

@M66B
Copy link
Owner

M66B commented May 28, 2014

ro.serialno may be different from Build.SERIAL, although this would be unusual.

ro.boot.serialno is delivered by the kernel, mostly as start parameter from the bootloader, which mostly reads it from protected read-only memory somewhere in your device (mostly only the manufacturer knows where and how, because the bootloader is often closed source and more and more inaccessible at all, even from kernel space and as a side note, in the past a lot of devices where unlocked by modifying/hacking/bypassing the boot loader in some way, but this is getting increasingly difficult, but fortunately more and more manufacturers provide a way to unlock your device, voiding your warranty, which is stupid and in some countries forbidden by law; personally I would never buy a device where this is not possible).

Android init copies ro.boot.serialno over to ro.serialno.

For me it works if I delete the salt from the database (and use the new flush cache button): selected accounts are still selected, also still after boot.

@an0n981
Copy link
Contributor Author

an0n981 commented May 28, 2014

The selected accounts are also still selected, but when I open the app ( I tested with GitHub and Twitter) it asks me to login, when I unrestrict accounts category, the app knows my login again

@M66B
Copy link
Owner

M66B commented May 28, 2014

Selected accounts management and restrictions using selected accounts use the same sha1 hash function, with the same salt.

Did you take into account the 15 seconds client side cache? (applied to the salt as well)

@an0n981
Copy link
Contributor Author

an0n981 commented May 28, 2014

Of course, even rebooted. Still the same result

@M66B
Copy link
Owner

M66B commented May 28, 2014

Then the salt stored in the database is probably different from Build.SERIAL, but this wouldn't explain the account is still selected, unless there are multiple allowed accounts in the database, hashed with a different salt.

Allowing the GitHub account works for me.

Maybe delete the allowed accounts from the database and allow them again is a solution.

@an0n981
Copy link
Contributor Author

an0n981 commented May 28, 2014

There was only 1 account in the db. Deleting it, and even recreating a new account in android and allowing it in the selected accounts doesn't work.
Do you have command to verify BUILD.Serial? getprop doesn't list it

@M66B
Copy link
Owner

M66B commented May 28, 2014

Not directly, but you can try this:

getprop | grep serial

BTW, you can use markdown in comments too.

@M66B
Copy link
Owner

M66B commented May 28, 2014

Are you sure you installed the test version and reboot thereafter?

@an0n981
Copy link
Contributor Author

an0n981 commented May 28, 2014

That only list the 2 values I listed above. Which are both the same as the salt.
If the link you posted (2.0.25-4) is the new test version, then yes I am sure

@M66B
Copy link
Owner

M66B commented May 28, 2014

I really don't see why it doesn't work for you :-(

If the salt in the database is the same as the listed values, it is safe to assume it is Build.SERIAL.
The code change is minimal: writing the salt = Build.SERIAL to the database has been replaced by defaulting to Build.SERIAL when reading from the database (so that everything is 100% backward compatible). If you remove the salt from the database, it should be replaced by the default, which should be exactly the same. Check the commit: 518939b

Moreover, I tested this with the GitHub application on my device and it works for me.

One thought: are you restricting identifications for XPrivacy itself?

@an0n981
Copy link
Contributor Author

an0n981 commented May 28, 2014

XPrivacy is completely unrestricted

M66B pushed a commit that referenced this issue May 28, 2014
@M66B
Copy link
Owner

M66B commented May 28, 2014

Maybe a logcat can help.

@an0n981
Copy link
Contributor Author

an0n981 commented May 28, 2014

From boot or just while opening the app?

@M66B
Copy link
Owner

M66B commented May 28, 2014

Just opening the app.
I will check which setting is read for the allowed account.

@M66B
Copy link
Owner

M66B commented May 28, 2014

This version prevents restricting the serial number for XPrivacy:
http://www.xprivacy.eu/XPrivacy_2.0.25-5.apk

Please try it before capturing a logcat.

@an0n981
Copy link
Contributor Author

an0n981 commented May 28, 2014

Didn't work either. Here a log https://mega.co.nz/#!Iw5FnQzL!NOmHl06ocnqhOdEV6CPkwLKIDYGiRl-MWTkFl6J2yW4

GitHub UID='10143'

Here something interesting

05-28 22:04:28.828 I/XPrivacy(5885): get setting uid=0 /Salt=E5CD9F335F7E8FA 2 ms

05-28 22:05:26.623 I/XPrivacy(5936): get setting uid=0 /Salt=F31131921E8981ED 2 ms

Neither is correct

@M66B
Copy link
Owner

M66B commented May 28, 2014

I guess I know what the problem is: please try without restricting SERIAL for GitHub.

@an0n981
Copy link
Contributor Author

an0n981 commented May 28, 2014

Already thought of that, didn't help and there is no usage data for github/serial

@M66B
Copy link
Owner

M66B commented May 28, 2014

Serial is randomized for 10143:

05-28 22:04:28.058 I/XPrivacy(2694): get service 10143/SERIAL(null) identification=!restricted? (cached) 0 ms
05-28 22:04:28.063 I/XPrivacy(5885): get client 10143/SERIAL(null) identification=restricted 1 ms
05-28 22:04:28.063 I/XPrivacy(5885): get setting uid=10143 /Serial=#Random# 2 ms

@an0n981
Copy link
Contributor Author

an0n981 commented May 28, 2014

You are correct, sorry I unrestricted %serialno instead of Serial (which of course has no usage data). Now it works. But unrestricting serial serial for apps that have allowed accounts doesn't sound like a good fix

@M66B
Copy link
Owner

M66B commented May 28, 2014

Of course this is not a fix, but knowing the problem is half the solution.

Actually this has been a privacy leak from the beginning, since any application can read the salt setting and thus get the serial number.

The challenge now is to find a way to get a salted hash without lifting the serial restriction, which is more difficult that it appears. I will sleep over this one night like Sherlock smokes his cigars ;-)

M66B pushed a commit that referenced this issue May 28, 2014
@M66B
Copy link
Owner

M66B commented May 28, 2014

I have not tested it yet, but this version should work without lifting the serial restriction:
http://www.xprivacy.eu/XPrivacy_2.0.25-6.apk

The privacy leak has been solved by letting the privacy service do the hashing (a one cigar problem ;-) and the salt will automatically be deleted from the database for everybody.

M66B pushed a commit that referenced this issue May 28, 2014
@an0n981
Copy link
Contributor Author

an0n981 commented May 28, 2014

You may need to buy some more cigars. With this version even when unrestricting serial it doesn't work.

@M66B
Copy link
Owner

M66B commented May 28, 2014

Indeed my dear Watson, but this version is tested to be working:
http://www.xprivacy.eu/XPrivacy_2.0.25-7.apk

@an0n981
Copy link
Contributor Author

an0n981 commented May 28, 2014

Your genius never ceases to amaze me! Confirmed with Twitter and GitHub

@M66B M66B closed this as completed May 29, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants