check repo index timestamps to prevent rollback attacks

A hacked fdroid server could "replay" old index.jar files known to have
apps with vulnerabilities in it.  That provides a long window of time for
exploiting that vulnerability.  By checking that the timestamp of an update
is never older than the current index, this attack is prevented.
parent 014ab2d2
Pipeline #3470681 failed with stage
in 32 minutes and 47 seconds
......@@ -20,6 +20,7 @@ import static org.junit.Assert.fail;
public class RepoUpdaterTest {
private Context context;
private Repo repo;
private RepoUpdater repoUpdater;
private File testFilesDir;
......@@ -30,10 +31,9 @@ public class RepoUpdaterTest {
Instrumentation instrumentation = InstrumentationRegistry.getInstrumentation();
context = instrumentation.getContext();
testFilesDir = TestUtils.getWriteableDir(instrumentation);
Repo repo = new Repo();
repo = new Repo();
repo.address = "https://fake.url/fdroid/repo";
repo.signingCertificate = this.simpleIndexSigningCert;
repoUpdater = new RepoUpdater(context, repo);
}
@Test
......@@ -42,6 +42,7 @@ public class RepoUpdaterTest {
return;
}
File simpleIndexJar = TestUtils.copyAssetToDir(context, "simpleIndex.jar", testFilesDir);
repoUpdater = new RepoUpdater(context, repo);
// these are supposed to succeed
try {
......@@ -52,6 +53,18 @@ public class RepoUpdaterTest {
}
}
@Test(expected = UpdateException.class)
public void testExtractIndexFromOutdatedJar() throws UpdateException {
File simpleIndexJar = TestUtils.copyAssetToDir(context, "simpleIndex.jar", testFilesDir);
repo.version = 10;
repo.timestamp = System.currentTimeMillis() / 1000L;
repoUpdater = new RepoUpdater(context, repo);
// these are supposed to fail
repoUpdater.processDownloadedFile(simpleIndexJar);
fail();
}
@Test(expected = UpdateException.class)
public void testExtractIndexFromJarWithoutSignatureJar() throws UpdateException {
if (!testFilesDir.canWrite()) {
......@@ -59,7 +72,9 @@ public class RepoUpdaterTest {
}
// this is supposed to fail
File jarFile = TestUtils.copyAssetToDir(context, "simpleIndexWithoutSignature.jar", testFilesDir);
repoUpdater = new RepoUpdater(context, repo);
repoUpdater.processDownloadedFile(jarFile);
fail();
}
@Test
......@@ -70,6 +85,7 @@ public class RepoUpdaterTest {
// this is supposed to fail
try {
File jarFile = TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedManifest.jar", testFilesDir);
repoUpdater = new RepoUpdater(context, repo);
repoUpdater.processDownloadedFile(jarFile);
fail();
} catch (UpdateException e) {
......@@ -88,6 +104,7 @@ public class RepoUpdaterTest {
// this is supposed to fail
try {
File jarFile = TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedSignature.jar", testFilesDir);
repoUpdater = new RepoUpdater(context, repo);
repoUpdater.processDownloadedFile(jarFile);
fail();
} catch (UpdateException e) {
......@@ -106,6 +123,7 @@ public class RepoUpdaterTest {
// this is supposed to fail
try {
File jarFile = TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedCertificate.jar", testFilesDir);
repoUpdater = new RepoUpdater(context, repo);
repoUpdater.processDownloadedFile(jarFile);
fail();
} catch (UpdateException e) {
......@@ -124,6 +142,7 @@ public class RepoUpdaterTest {
// this is supposed to fail
try {
File jarFile = TestUtils.copyAssetToDir(context, "simpleIndexWithCorruptedEverything.jar", testFilesDir);
repoUpdater = new RepoUpdater(context, repo);
repoUpdater.processDownloadedFile(jarFile);
fail();
} catch (UpdateException e) {
......@@ -142,6 +161,7 @@ public class RepoUpdaterTest {
// this is supposed to fail
try {
File jarFile = TestUtils.copyAssetToDir(context, "masterKeyIndex.jar", testFilesDir);
repoUpdater = new RepoUpdater(context, repo);
repoUpdater.processDownloadedFile(jarFile);
fail(); //NOPMD
} catch (UpdateException e) {
......
......@@ -156,9 +156,9 @@ public class RepoUpdater {
private RepoXMLHandler.IndexReceiver createIndexReceiver() {
return new RepoXMLHandler.IndexReceiver() {
@Override
public void receiveRepo(String name, String description, String signingCert, int maxAge, int version) {
public void receiveRepo(String name, String description, String signingCert, int maxAge, int version, long timestamp) {
signingCertFromIndexXml = signingCert;
repoDetailsToSave = prepareRepoDetailsForSaving(name, description, maxAge, version);
repoDetailsToSave = prepareRepoDetailsForSaving(name, description, maxAge, version, timestamp);
}
@Override
......@@ -196,6 +196,12 @@ public class RepoUpdater {
reader.setContentHandler(repoXMLHandler);
reader.parse(new InputSource(indexInputStream));
long timestamp = repoDetailsToSave.getAsLong("timestamp");
if (timestamp < repo.timestamp) {
throw new UpdateException(repo, "index.jar is older that current index! "
+ timestamp + " < " + repo.timestamp);
}
signingCertFromJar = getSigningCertFromJar(indexEntry);
// JarEntry can only read certificates after the file represented by that JarEntry
......@@ -242,7 +248,7 @@ public class RepoUpdater {
* Update tracking data for the repo represented by this instance (index version, etag,
* description, human-readable name, etc.
*/
private ContentValues prepareRepoDetailsForSaving(String name, String description, int maxAge, int version) {
private ContentValues prepareRepoDetailsForSaving(String name, String description, int maxAge, int version, long timestamp) {
ContentValues values = new ContentValues();
values.put(RepoProvider.DataColumns.LAST_UPDATED, Utils.formatTime(new Date(), ""));
......@@ -269,6 +275,10 @@ public class RepoUpdater {
values.put(RepoProvider.DataColumns.NAME, name);
}
if (timestamp != repo.timestamp) {
values.put(RepoProvider.DataColumns.TIMESTAMP, timestamp);
}
return values;
}
......
......@@ -51,6 +51,7 @@ public class RepoXMLHandler extends DefaultHandler {
// them - otherwise it will be the value specified.
private int repoMaxAge = -1;
private int repoVersion;
private long repoTimestamp;
private String repoDescription;
private String repoName;
......@@ -60,7 +61,7 @@ public class RepoXMLHandler extends DefaultHandler {
private final StringBuilder curchars = new StringBuilder();
interface IndexReceiver {
void receiveRepo(String name, String description, String signingCert, int maxage, int version);
void receiveRepo(String name, String description, String signingCert, int maxage, int version, long timestamp);
void receiveApp(App app, List<Apk> packages);
}
......@@ -79,7 +80,7 @@ public class RepoXMLHandler extends DefaultHandler {
@Override
public void endElement(String uri, String localName, String qName)
throws SAXException {
throws SAXException {
if ("application".equals(localName) && curapp != null) {
onApplicationParsed();
......@@ -239,7 +240,7 @@ public class RepoXMLHandler extends DefaultHandler {
}
private void onRepoParsed() {
receiver.receiveRepo(repoName, repoDescription, repoSigningCert, repoMaxAge, repoVersion);
receiver.receiveRepo(repoName, repoDescription, repoSigningCert, repoMaxAge, repoVersion, repoTimestamp);
}
@Override
......@@ -253,6 +254,7 @@ public class RepoXMLHandler extends DefaultHandler {
repoVersion = Utils.parseInt(attributes.getValue("", "version"), -1);
repoName = cleanWhiteSpace(attributes.getValue("", "name"));
repoDescription = cleanWhiteSpace(attributes.getValue("", "description"));
repoTimestamp = parseLong(attributes.getValue("", "timestamp"), 0);
} else if ("application".equals(localName) && curapp == null) {
curapp = new App();
curapp.packageName = attributes.getValue("", "id");
......@@ -271,4 +273,17 @@ public class RepoXMLHandler extends DefaultHandler {
private static String cleanWhiteSpace(@Nullable String str) {
return str == null ? null : str.replaceAll("\\s", " ");
}
private static long parseLong(String str, long fallback) {
if (str == null || str.length() == 0) {
return fallback;
}
long result;
try {
result = Long.parseLong(str);
} catch (NumberFormatException e) {
result = fallback;
}
return result;
}
}
......@@ -35,7 +35,8 @@ class DBHelper extends SQLiteOpenHelper {
+ "version integer not null default 0, "
+ "lastetag text, lastUpdated string,"
+ "isSwap integer boolean default 0,"
+ "username string, password string"
+ "username string, password string,"
+ "timestamp integer not null default 0"
+ ");";
private static final String CREATE_TABLE_APK =
......@@ -106,7 +107,7 @@ class DBHelper extends SQLiteOpenHelper {
+ " );";
private static final String DROP_TABLE_INSTALLED_APP = "DROP TABLE " + TABLE_INSTALLED_APP + ";";
private static final int DB_VERSION = 54;
private static final int DB_VERSION = 55;
private final Context context;
......@@ -261,6 +262,7 @@ class DBHelper extends SQLiteOpenHelper {
values.put(RepoProvider.DataColumns.IN_USE, inUse);
values.put(RepoProvider.DataColumns.PRIORITY, priority);
values.put(RepoProvider.DataColumns.LAST_ETAG, (String) null);
values.put(RepoProvider.DataColumns.TIMESTAMP, 0);
Utils.debugLog(TAG, "Adding repository " + name);
db.insert(TABLE_REPO, null, values);
......@@ -294,6 +296,7 @@ class DBHelper extends SQLiteOpenHelper {
addCredentialsToRepo(db, oldVersion);
addAuthorToApp(db, oldVersion);
useMaxValueInMaxSdkVersion(db, oldVersion);
requireTimestampInRepos(db, oldVersion);
}
/**
......@@ -502,6 +505,21 @@ class DBHelper extends SQLiteOpenHelper {
db.update(TABLE_APK, values, ApkProvider.DataColumns.MAX_SDK_VERSION + " < 1", null);
}
/**
* The {@code <repo timestamp="">} value was in the metadata for a long time,
* but it was not being used in the client until now.
*/
private void requireTimestampInRepos(SQLiteDatabase db, int oldVersion) {
if (oldVersion >= 55) {
return;
}
if (!columnExists(db, TABLE_REPO, RepoProvider.DataColumns.TIMESTAMP)) {
Utils.debugLog(TAG, "Adding " + RepoProvider.DataColumns.TIMESTAMP + " column to " + TABLE_REPO);
db.execSQL("alter table " + TABLE_REPO + " add column "
+ RepoProvider.DataColumns.TIMESTAMP + " integer not null default 0");
}
}
/**
* By clearing the etags stored in the repo table, it means that next time the user updates
* their repos (either manually or on a scheduled task), they will update regardless of whether
......
......@@ -26,7 +26,7 @@ public class Repo extends ValueObject {
/** The signing certificate, {@code null} for a newly added repo */
public String signingCertificate;
/**
* The SHA1 fingerprint of {@link #pubkey}, set to {@code null} when a
* The SHA1 fingerprint of {@link #signingCertificate}, 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;
......@@ -40,6 +40,9 @@ public class Repo extends ValueObject {
public String username;
public String password;
/** When the signed repo index was generated, used to protect against replay attacks */
public long timestamp;
public Repo() {
}
......@@ -94,6 +97,9 @@ public class Repo extends ValueObject {
case RepoProvider.DataColumns.PASSWORD:
password = cursor.getString(i);
break;
case RepoProvider.DataColumns.TIMESTAMP:
timestamp = cursor.getLong(i);
break;
}
}
}
......@@ -210,5 +216,9 @@ public class Repo extends ValueObject {
if (values.containsKey(RepoProvider.DataColumns.PASSWORD)) {
password = values.getAsString(RepoProvider.DataColumns.PASSWORD);
}
if (values.containsKey(RepoProvider.DataColumns.TIMESTAMP)) {
timestamp = toInt(values.getAsInteger(RepoProvider.DataColumns.TIMESTAMP));
}
}
}
......@@ -225,11 +225,12 @@ public class RepoProvider extends FDroidProvider {
String IS_SWAP = "isSwap";
String USERNAME = "username";
String PASSWORD = "password";
String TIMESTAMP = "timestamp";
String[] ALL = {
_ID, ADDRESS, NAME, DESCRIPTION, IN_USE, PRIORITY, SIGNING_CERT,
FINGERPRINT, MAX_AGE, LAST_UPDATED, LAST_ETAG, VERSION, IS_SWAP,
USERNAME, PASSWORD,
USERNAME, PASSWORD, TIMESTAMP,
};
}
......
......@@ -384,6 +384,7 @@ public final class LocalRepoManager {
serializer.attribute("", "pubkey", Hasher.hex(LocalRepoKeyStore.get(context).getCertificate()));
long timestamp = System.currentTimeMillis() / 1000L;
serializer.attribute("", "timestamp", String.valueOf(timestamp));
serializer.attribute("", "version", "10");
tag("description", "A local FDroid repo generated from apps installed on " + Preferences.get().getLocalRepoName());
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment