From 003f5331fa2659ef53fe24fa8f96963f959b24e3 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Fri, 11 Sep 2015 11:17:51 +0200 Subject: [PATCH 1/5] revert to previous, proven index.xml writing It was added in c831cf77ccf9ecfa792d0ffdc84f272053fa945a and changed in 42d31eb0e6e5d5c9e7fe9c35435a56258ad90578 --- .../org/fdroid/fdroid/localrepo/LocalRepoManager.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/F-Droid/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java b/F-Droid/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java index ad23254c8..58b64ef9a 100644 --- a/F-Droid/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java +++ b/F-Droid/src/org/fdroid/fdroid/localrepo/LocalRepoManager.java @@ -345,11 +345,13 @@ public class LocalRepoManager { serializer = XmlPullParserFactory.newInstance().newSerializer(); } - public void build(Writer output) throws IOException, LocalRepoKeyStore.InitException { + public void build(File file) throws IOException, LocalRepoKeyStore.InitException { + Writer output = new FileWriter(file); serializer.setOutput(output); serializer.startDocument(null, null); tagFdroid(); serializer.endDocument(); + output.close(); } private void tagFdroid() throws IOException, LocalRepoKeyStore.InitException { @@ -485,16 +487,12 @@ public class LocalRepoManager { public void writeIndexJar() throws IOException { - FileWriter writer = null; try { - writer = new FileWriter(xmlIndex); - new IndexXmlBuilder(context, apps).build(writer); + new IndexXmlBuilder(context, apps).build(xmlIndex); } catch (Exception e) { Log.e(TAG, "Could not write index jar", e); Toast.makeText(context, R.string.failed_to_create_index, Toast.LENGTH_LONG).show(); return; - } finally { - Utils.closeQuietly(writer); } BufferedOutputStream bo = new BufferedOutputStream(new FileOutputStream(xmlIndexJarUnsigned)); From 03d074a0e8bb666c5c6ed2328d047059af060982 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Fri, 11 Sep 2015 11:38:58 +0200 Subject: [PATCH 2/5] always use Locale.ENGLISH for formatting Strings for the machine For Strings that are meant to be displayed to humans, using the default Locale makes sense. But this String is meant to be parsed by code, so it should force the Locale to make sure the digits are always rendered in the same format. --- F-Droid/src/org/fdroid/fdroid/net/WifiStateChangeService.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/F-Droid/src/org/fdroid/fdroid/net/WifiStateChangeService.java b/F-Droid/src/org/fdroid/fdroid/net/WifiStateChangeService.java index 860371ec8..5befdbaf2 100644 --- a/F-Droid/src/org/fdroid/fdroid/net/WifiStateChangeService.java +++ b/F-Droid/src/org/fdroid/fdroid/net/WifiStateChangeService.java @@ -207,7 +207,8 @@ public class WifiStateChangeService extends Service { // the following methods were not added until android-9/Gingerbread for (InterfaceAddress address : netIf.getInterfaceAddresses()) { if (inetAddress.equals(address.getAddress()) && !TextUtils.isEmpty(FDroidApp.ipAddressString)) { - String cidr = String.format("%s/%d", FDroidApp.ipAddressString, address.getNetworkPrefixLength()); + String cidr = String.format(Locale.ENGLISH, "%s/%d", + FDroidApp.ipAddressString, address.getNetworkPrefixLength()); FDroidApp.subnetInfo = (new SubnetUtils(cidr)).getInfo(); break; } From 69b210a4b816a2d593f17cb8475d3d7b72a1eb91 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Fri, 11 Sep 2015 14:09:56 +0200 Subject: [PATCH 3/5] fix swap to follow repo.fingerprint logic: null means unset The repo.fingerprint logic is based on null meaning that the fingerprint has not been set. This ensures that a "" does not sneak in somehow as a valid fingerprint. --- .../javax/jmdns/impl/FDroidServiceInfo.java | 16 +++++++++++++++- F-Droid/src/org/fdroid/fdroid/RepoUpdater.java | 14 ++++++++++++-- F-Droid/src/org/fdroid/fdroid/data/Repo.java | 18 +++++++++++++----- .../fdroid/fdroid/localrepo/SwapService.java | 4 +++- .../fdroid/localrepo/peers/BluetoothPeer.java | 10 ++++++---- .../fdroid/localrepo/peers/BonjourPeer.java | 3 +++ 6 files changed, 52 insertions(+), 13 deletions(-) diff --git a/F-Droid/src/javax/jmdns/impl/FDroidServiceInfo.java b/F-Droid/src/javax/jmdns/impl/FDroidServiceInfo.java index c4fba32f3..111354994 100644 --- a/F-Droid/src/javax/jmdns/impl/FDroidServiceInfo.java +++ b/F-Droid/src/javax/jmdns/impl/FDroidServiceInfo.java @@ -2,6 +2,7 @@ package javax.jmdns.impl; import android.os.Parcel; import android.os.Parcelable; +import android.text.TextUtils; import java.net.Inet4Address; import java.net.Inet6Address; @@ -21,8 +22,21 @@ public class FDroidServiceInfo extends ServiceInfoImpl implements Parcelable { super(info); } + /** + * Return the fingerprint of the signing key, or {@code null} if it is not set. + */ public String getFingerprint() { - return getPropertyString("fingerprint"); + // getPropertyString() will return "true" if the value is a zero-length byte array + // so we just do a custom version using getPropertyBytes() + byte[] data = getPropertyBytes("fingerprint"); + if (data == null || data.length == 0) { + return null; + } + String fingerprint = this.readUTF(data, 0, data.length); + if (TextUtils.isEmpty(fingerprint)) { + return null; + } + return fingerprint; } public String getRepoAddress() { diff --git a/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java b/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java index 17b964e52..b23d28f7e 100644 --- a/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java +++ b/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java @@ -36,6 +36,14 @@ import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; +/** + * Handles getting the index metadata for an app repo, then verifying the + * signature on the index metdata, implementing as a JAR signature. + *

+ * WARNING: this class is the central piece of the entire security model of + * FDroid! Avoid modifying it when possible, if you absolutely must, be very, + * very careful with the changes that you are making! + */ public class RepoUpdater { private static final String TAG = "RepoUpdater"; @@ -280,7 +288,8 @@ public class RepoUpdater { * actually in the index.jar itself. If no fingerprint, just store the * signing certificate */ boolean trustNewSigningCertificate = false; - if (TextUtils.isEmpty(repo.fingerprint)) { + // If the fingerprint has never been set, it will be null (never "" or something else) + if (repo.fingerprint == null) { // no info to check things are valid, so just Trust On First Use trustNewSigningCertificate = true; } else { @@ -290,7 +299,8 @@ public class RepoUpdater { && repo.fingerprint.equalsIgnoreCase(fingerprintFromJar)) { trustNewSigningCertificate = true; } else { - throw new UpdateException(repo, "Supplied certificate fingerprint does not match!"); + throw new UpdateException(repo, "Supplied certificate fingerprint does not match: '" + + repo.fingerprint + "' '" + fingerprintFromIndexXml + "' '" + fingerprintFromJar + "'"); } } diff --git a/F-Droid/src/org/fdroid/fdroid/data/Repo.java b/F-Droid/src/org/fdroid/fdroid/data/Repo.java index fb4cbdd06..28979ec6c 100644 --- a/F-Droid/src/org/fdroid/fdroid/data/Repo.java +++ b/F-Droid/src/org/fdroid/fdroid/data/Repo.java @@ -19,13 +19,21 @@ public class Repo extends ValueObject { public String address; public String name; public String description; - public int version; // index version, i.e. what fdroidserver built it - 0 if not specified + /** index version, i.e. what fdroidserver built it - 0 if not specified */ + public int version; public boolean inuse; public int priority; - public String pubkey; // null for an unsigned repo - public String fingerprint; // always null for an unsigned repo - public int maxage; // maximum age of index that will be accepted - 0 for any - public String lastetag; // last etag we updated from, null forces update + /** The signing certificate, {@code null} for a newly added repo */ + public String pubkey; + /** + * The SHA1 fingerprint of {@link #pubkey}, set to {@code null} when a + * newly added repo did not include fingerprint. It should never be an + * empty {@link String}, i.e. {@code ""} */ + public String fingerprint; + /** maximum age of index that will be accepted - 0 for any */ + public int maxage; + /** last etag we updated from, null forces update */ + public String lastetag; public Date lastUpdated; public boolean isSwap; diff --git a/F-Droid/src/org/fdroid/fdroid/localrepo/SwapService.java b/F-Droid/src/org/fdroid/fdroid/localrepo/SwapService.java index 1e5f9b708..09e9e9998 100644 --- a/F-Droid/src/org/fdroid/fdroid/localrepo/SwapService.java +++ b/F-Droid/src/org/fdroid/fdroid/localrepo/SwapService.java @@ -259,7 +259,9 @@ public class SwapService extends Service { values.put(RepoProvider.DataColumns.NAME, peer.getName()); values.put(RepoProvider.DataColumns.ADDRESS, peer.getRepoAddress()); values.put(RepoProvider.DataColumns.DESCRIPTION, ""); - values.put(RepoProvider.DataColumns.FINGERPRINT, peer.getFingerprint()); + String fingerprint = peer.getFingerprint(); + if (!TextUtils.isEmpty(fingerprint)) + values.put(RepoProvider.DataColumns.FINGERPRINT, peer.getFingerprint()); values.put(RepoProvider.DataColumns.IN_USE, true); values.put(RepoProvider.DataColumns.IS_SWAP, true); Uri uri = RepoProvider.Helper.insert(this, values); diff --git a/F-Droid/src/org/fdroid/fdroid/localrepo/peers/BluetoothPeer.java b/F-Droid/src/org/fdroid/fdroid/localrepo/peers/BluetoothPeer.java index 45261d6d5..4d12db070 100644 --- a/F-Droid/src/org/fdroid/fdroid/localrepo/peers/BluetoothPeer.java +++ b/F-Droid/src/org/fdroid/fdroid/localrepo/peers/BluetoothPeer.java @@ -40,13 +40,15 @@ public class BluetoothPeer implements Peer { } /** - * Bluetooth will exclusively be TOFU. Once a device is connected to a bluetooth socket, - * if we trust it enough to accept a fingerprint from it somehow, then we may as well trust it - * enough to receive an index from it that contains a fingerprint we can use. + * Return the fingerprint of the signing key, or {@code null} if it is not set. + *

+ * This is not yet stored for Bluetooth connections. Once a device is connected to a bluetooth + * socket, if we trust it enough to accept a fingerprint from it somehow, then we may as well + * trust it enough to receive an index from it that contains a fingerprint we can use. */ @Override public String getFingerprint() { - return ""; + return null; } @Override diff --git a/F-Droid/src/org/fdroid/fdroid/localrepo/peers/BonjourPeer.java b/F-Droid/src/org/fdroid/fdroid/localrepo/peers/BonjourPeer.java index def234be7..0da35cc0e 100644 --- a/F-Droid/src/org/fdroid/fdroid/localrepo/peers/BonjourPeer.java +++ b/F-Droid/src/org/fdroid/fdroid/localrepo/peers/BonjourPeer.java @@ -41,6 +41,9 @@ public class BonjourPeer extends WifiPeer { return serviceInfo.getRepoAddress(); } + /** + * Return the fingerprint of the signing key, or {@code null} if it is not set. + */ @Override public String getFingerprint() { return serviceInfo.getFingerprint(); From d37b473e10bdc4e138a65876046ff8167ee5fff3 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 16 Sep 2015 12:33:17 +0200 Subject: [PATCH 4/5] temporary workaround to NPE crash on first install The solution is really to get rid of the Fragment and do everything in the Activity, especially since this nullguard will probably leave things in a not good state. But that's better than a crash, I think. W java.lang.NullPointerException W at org.fdroid.fdroid.views.fragments.AvailableAppsFragment$CategoryObserver.onChange(AvailableAppsFragment.java:88) W at org.fdroid.fdroid.views.fragments.AvailableAppsFragment$CategoryObserver.onChange(AvailableAppsFragment.java:103) W at android.database.ContentObserver.dispatchChange(ContentObserver.java:163) W at android.database.ContentObserver$Transport.onChange(ContentObserver.java:195) W at android.database.IContentObserver$Stub.onTransact(IContentObserver.java:60) W at android.os.Binder.execTransact(Binder.java:404) W at dalvik.system.NativeStart.run(Native Method) --- .../fdroid/views/fragments/AvailableAppsFragment.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/F-Droid/src/org/fdroid/fdroid/views/fragments/AvailableAppsFragment.java b/F-Droid/src/org/fdroid/fdroid/views/fragments/AvailableAppsFragment.java index 8b907f510..8ce67a395 100644 --- a/F-Droid/src/org/fdroid/fdroid/views/fragments/AvailableAppsFragment.java +++ b/F-Droid/src/org/fdroid/fdroid/views/fragments/AvailableAppsFragment.java @@ -84,7 +84,11 @@ public class AvailableAppsFragment extends AppListFragment implements // Wanted to just do this update here, but android tells // me that "Only the original thread that created a view // hierarchy can touch its views." - getActivity().runOnUiThread(new Runnable() { + final Activity activity = getActivity(); + // this nullguard is temporary, this Fragment really needs to merged into the Activity + if (activity == null) + return; + activity.runOnUiThread(new Runnable() { @Override public void run() { if (adapter == null) { From c8b64281c2ad0bf0ad50c7933dba237bc0c0e40c Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 16 Sep 2015 12:44:17 +0200 Subject: [PATCH 5/5] sync up logcat tags with the class names --- .../src/org/fdroid/fdroid/localrepo/peers/BluetoothPeer.java | 1 + F-Droid/src/org/fdroid/fdroid/localrepo/type/BluetoothSwap.java | 2 +- .../src/org/fdroid/fdroid/localrepo/type/BonjourBroadcast.java | 2 +- F-Droid/src/org/fdroid/fdroid/localrepo/type/WifiSwap.java | 2 +- 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/F-Droid/src/org/fdroid/fdroid/localrepo/peers/BluetoothPeer.java b/F-Droid/src/org/fdroid/fdroid/localrepo/peers/BluetoothPeer.java index 4d12db070..4f29b31ec 100644 --- a/F-Droid/src/org/fdroid/fdroid/localrepo/peers/BluetoothPeer.java +++ b/F-Droid/src/org/fdroid/fdroid/localrepo/peers/BluetoothPeer.java @@ -7,6 +7,7 @@ import org.fdroid.fdroid.R; import org.fdroid.fdroid.localrepo.type.BluetoothSwap; public class BluetoothPeer implements Peer { + private static final String TAG = "BluetoothPeer"; private BluetoothDevice device; diff --git a/F-Droid/src/org/fdroid/fdroid/localrepo/type/BluetoothSwap.java b/F-Droid/src/org/fdroid/fdroid/localrepo/type/BluetoothSwap.java index 57563250a..7894ebeb9 100644 --- a/F-Droid/src/org/fdroid/fdroid/localrepo/type/BluetoothSwap.java +++ b/F-Droid/src/org/fdroid/fdroid/localrepo/type/BluetoothSwap.java @@ -14,7 +14,7 @@ import org.fdroid.fdroid.net.bluetooth.BluetoothServer; public class BluetoothSwap extends SwapType { - private static final String TAG = "BluetoothBroadcastType"; + private static final String TAG = "BluetoothSwap"; public final static String BLUETOOTH_NAME_TAG = "FDroid:"; private static BluetoothSwap mInstance = null; diff --git a/F-Droid/src/org/fdroid/fdroid/localrepo/type/BonjourBroadcast.java b/F-Droid/src/org/fdroid/fdroid/localrepo/type/BonjourBroadcast.java index bd29ea26a..3d6743858 100644 --- a/F-Droid/src/org/fdroid/fdroid/localrepo/type/BonjourBroadcast.java +++ b/F-Droid/src/org/fdroid/fdroid/localrepo/type/BonjourBroadcast.java @@ -20,7 +20,7 @@ import javax.jmdns.ServiceInfo; */ public class BonjourBroadcast extends SwapType { - private static final String TAG = "BonjourSwapService"; + private static final String TAG = "BonjourBroadcast"; private JmDNS jmdns; private ServiceInfo pairService; diff --git a/F-Droid/src/org/fdroid/fdroid/localrepo/type/WifiSwap.java b/F-Droid/src/org/fdroid/fdroid/localrepo/type/WifiSwap.java index 2d6bd629e..33ef06172 100644 --- a/F-Droid/src/org/fdroid/fdroid/localrepo/type/WifiSwap.java +++ b/F-Droid/src/org/fdroid/fdroid/localrepo/type/WifiSwap.java @@ -21,7 +21,7 @@ import java.util.Random; public class WifiSwap extends SwapType { - private static final String TAG = "WebServerType"; + private static final String TAG = "WifiSwap"; private Handler webServerThreadHandler = null; private LocalHTTPD localHttpd;