Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use downcased app_keystore dir to avoid OS bugs #24

Closed
eighthave opened this issue Sep 5, 2014 · 7 comments
Closed

use downcased app_keystore dir to avoid OS bugs #24

eighthave opened this issue Sep 5, 2014 · 7 comments

Comments

@eighthave
Copy link

Running fdroidclient on my Motorola Xoom running CM 10.1-2014-02-16-NIGHTLY, MTM triggers the creation of two dirs based on the "KeyStore" name app_keystore and app_KeyStore. MTM ultimately ends up storing the file in app_keystore, even though that is not the name specified. app_KeyStore remains empty.

MTM should set the same to all lowercase to avoid issues related to bugs like this.

@Flowdalic
Copy link
Collaborator

I can not confirm this behavior. E.g. the MTM instances I use in my app only create app_KeyStore. Also I couldn't find any indication why this would happen after a quick glance at the code. Are you sure that this directory isn't a relict of some other mechanism?

But let's wait what @ge0rg thinks

@eighthave
Copy link
Author

this bug report is based on a fresh install of fdroid, so there is no upgrade logic that could have left an old dir in place. It is likely a bug in the ROM, but I see no good reason to use caps in those dir names, so this is a defensive fix to make things more resilient in the face of stupid ROM bugs.

@Flowdalic
Copy link
Collaborator

Hmm a quick recursive grep in fdroidclient's code shows:

 fdroidclient $ ag keystore                          >> ~/data/code/fdroidclient
test/ant.properties
14:#  'key.store' for the location of your keystore and

src/org/fdroid/fdroid/localrepo/LocalRepoKeyStore.java
67:            File appKeyStoreDir = context.getDir("keystore", Context.MODE_PRIVATE);
71:            // If there isn't a persisted BKS keystore on disk we need to
72:            // create a new empty keystore
74:                // Init a new keystore with a blank passphrase
81:             * If the keystore we loaded doesn't have an INDEX_CERT_ALIAS entry
88:                 * INDEX_CERT_ALIAS certificate in the keystore. This keypair
106:             * keystore alias is used for the correct purpose. With the default
144:             * into the keystore in a predictable place. If the IP address
173:            KeyStore keystore = getKeyStore();
174:            X509Certificate cert = (X509Certificate) keystore.getCertificate(INDEX_CERT_ALIAS);
247:         * After adding an entry to the keystore we need to create a fresh

It appears that fdroid is creating this directory (LocalRepoKeyStore.java:67)

@eighthave
Copy link
Author

oops, my mistake, sorry for the noise. For the record, it would still be nice to use all lower case for such names. It seems neater to me.

@Flowdalic
Copy link
Collaborator

No problem, I think we learned something from it. I've created: #26

@ge0rg
Copy link
Owner

ge0rg commented Sep 6, 2014

@eighthave You are right, it was not very forward-looking of me to use the capitalized directory name. However, changing the default now will "break" existing deployments, which I am not very fond of.

"Break" here means that all existing user choices will be lost, and the user will be re-prompted, which might be misunderstood as a sign of MitM.

@indrora
Copy link

indrora commented Sep 6, 2014

@ge0rg : I have some experience with that.

The best way to handle it at this point is most likely to check what version is being used (either via API call or file check) when opening the default key store. If you're dealing with a version you have to break (e.g. you're higher than one before a breaking change) then you do the "most right" thing (whatever that may be).

This future proofs the thing by a tiny bit. It also means that applications can query if they need to go and un-break themselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants