Karl/BobStore
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge branch 'index-input-hardening' into 'master'

Browse Source 
Index input hardening

See merge request fdroid/fdroidclient!784
This commit is contained in:
Hans-Christoph Steiner 2019-01-03 14:24:10 +00:00
commit 12728d6101
19 changed files with 195 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/src/androidTest/java/org/fdroid/fdroid/installer/ApkVerifierTest.java
View File

@ -28,7 +28,7 @@ import android.support.test.runner.AndroidJUnit4;
import android.util.Log;
import org.fdroid.fdroid.AssetUtils;
import org.fdroid.fdroid.RepoXMLHandler;
import org.fdroid.fdroid.data.RepoXMLHandler;
import org.fdroid.fdroid.Utils;
import org.fdroid.fdroid.compat.FileCompatTest;
import org.fdroid.fdroid.data.Apk;

1
app/src/main/java/org/fdroid/fdroid/IndexUpdater.java
View File

@ -38,6 +38,7 @@ import org.fdroid.fdroid.data.Repo;
import org.fdroid.fdroid.data.RepoPersister;
import org.fdroid.fdroid.data.RepoProvider;
import org.fdroid.fdroid.data.RepoPushRequest;
import org.fdroid.fdroid.data.RepoXMLHandler;
import org.fdroid.fdroid.data.Schema.RepoTable;
import org.fdroid.fdroid.installer.InstallManagerService;
import org.fdroid.fdroid.installer.InstallerService;

18
app/src/main/java/org/fdroid/fdroid/Utils.java
View File

@ -76,6 +76,7 @@ import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.concurrent.TimeUnit;
import java.util.regex.Pattern;
public final class Utils {
@ -98,6 +99,8 @@ public final class Utils {
private static DisplayImageOptions.Builder defaultDisplayImageOptionsBuilder;
private static DisplayImageOptions repoAppDisplayImageOptions;
private static Pattern safePackageNamePattern;
public static final String FALLBACK_ICONS_DIR = "/icons/";
/*
@ -621,6 +624,21 @@ public final class Utils {
return sb;
}
/**
* This is not strict validation of the package name, this is just to make
* sure that the package name is not used as an attack vector, e.g. SQL
* Injection.
*/
public static boolean isSafePackageName(@Nullable String packageName) {
if (TextUtils.isEmpty(packageName)) {
return false;
}
if (safePackageNamePattern == null) {
safePackageNamePattern = Pattern.compile("[a-zA-Z0-9._]+");
}
return safePackageNamePattern.matcher(packageName).matches();
}
/**
* Calculate the number of days since the given date.
*/

15
app/src/main/java/org/fdroid/fdroid/data/Apk.java
View File

@ -16,7 +16,6 @@ import android.webkit.MimeTypeMap;
import com.fasterxml.jackson.annotation.JacksonInject;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import org.fdroid.fdroid.RepoXMLHandler;
import org.fdroid.fdroid.Utils;
import org.fdroid.fdroid.data.Schema.ApkTable.Cols;
@ -461,6 +460,20 @@ public class Apk extends ValueObject implements Comparable<Apk>, Parcelable {
return null;
}
/**
* Set the Package Name property while ensuring it is sanitized.
*/
@JsonProperty("packageName")
@SuppressWarnings("unused")
void setPackageName(String packageName) {
if (Utils.isSafePackageName(packageName)) {
this.packageName = packageName;
} else {
throw new IllegalArgumentException("Repo index package entry includes unsafe packageName: '"
+ packageName + "'");
}
}
@JsonProperty("uses-permission")
@SuppressWarnings("unused")
private void setUsesPermission(Object[][] permissions) {

2
app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java
View File

@ -533,7 +533,7 @@ public class ApkProvider extends FDroidProvider {
queryBuilder.addSelection(query);
queryBuilder.addOrderBy(sortOrder);
Cursor cursor = LoggingQuery.query(db(), queryBuilder.toString(), queryBuilder.getArgs());
Cursor cursor = LoggingQuery.rawQuery(db(), queryBuilder.toString(), queryBuilder.getArgs());
cursor.setNotificationUri(getContext().getContentResolver(), uri);
return cursor;
}

13
app/src/main/java/org/fdroid/fdroid/data/App.java
View File

@ -406,6 +406,19 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
this.description = formatDescription(description);
}
/**
* Set the Package Name property while ensuring it is sanitized.
*/
@JsonProperty("packageName")
void setPackageName(String packageName) {
if (Utils.isSafePackageName(packageName)) {
this.packageName = packageName;
} else {
throw new IllegalArgumentException("Repo index app entry includes unsafe packageName: '"
+ packageName + "'");
}
}
/**
* Parses the {@code localized} block in the incoming index metadata,
* choosing the best match in terms of locale/language while filling as

2
app/src/main/java/org/fdroid/fdroid/data/AppPrefsProvider.java
View File

@ -131,7 +131,7 @@ public class AppPrefsProvider extends FDroidProvider {
query.addFields(projection);
query.addOrderBy(sortOrder);
Cursor cursor = LoggingQuery.query(db(), query.toString(), query.getArgs());
Cursor cursor = LoggingQuery.rawQuery(db(), query.toString(), query.getArgs());
cursor.setNotificationUri(getContext().getContentResolver(), uri);
return cursor;
}

2
app/src/main/java/org/fdroid/fdroid/data/AppProvider.java
View File

@ -892,7 +892,7 @@ public class AppProvider extends FDroidProvider {
query.addOrderBy(sortOrder);
query.addLimit(limit);
Cursor cursor = LoggingQuery.query(db(), query.toString(), query.getArgs());
Cursor cursor = LoggingQuery.rawQuery(db(), query.toString(), query.getArgs());
cursor.setNotificationUri(getContext().getContentResolver(), uri);
return cursor;
}

2
app/src/main/java/org/fdroid/fdroid/data/CategoryProvider.java
View File

@ -211,7 +211,7 @@ public class CategoryProvider extends FDroidProvider {
query.setOnlyCategoriesWithApps();
}
Cursor cursor = LoggingQuery.query(db(), query.toString(), query.getArgs());
Cursor cursor = LoggingQuery.rawQuery(db(), query.toString(), query.getArgs());
cursor.setNotificationUri(getContext().getContentResolver(), uri);
return cursor;
}

2
app/src/main/java/org/fdroid/fdroid/data/LoggingQuery.java
View File

@ -161,7 +161,7 @@ final class LoggingQuery {
return plan;
}
public static Cursor query(SQLiteDatabase db, String query, String[] queryBuilderArgs) {
public static Cursor rawQuery(SQLiteDatabase db, String query, String[] queryBuilderArgs) {
return new LoggingQuery(db, query, queryBuilderArgs).rawQuery();
}

2
app/src/main/java/org/fdroid/fdroid/data/PackageProvider.java
View File

@ -138,7 +138,7 @@ public class PackageProvider extends FDroidProvider {
query.addFields(projection);
query.addOrderBy(sortOrder);
Cursor cursor = LoggingQuery.query(db(), query.toString(), query.getArgs());
Cursor cursor = LoggingQuery.rawQuery(db(), query.toString(), query.getArgs());
cursor.setNotificationUri(getContext().getContentResolver(), uri);
return cursor;
}

26
app/src/main/java/org/fdroid/fdroid/data/RepoPushRequest.java
View File

@ -20,6 +20,10 @@
package org.fdroid.fdroid.data;
import android.support.annotation.Nullable;
import org.fdroid.fdroid.Utils;
import java.util.Arrays;
import java.util.List;
/**
* Represents action requests embedded in the index XML received from a repo.
@ -32,15 +36,33 @@ public class RepoPushRequest {
public static final String INSTALL = "install";
public static final String UNINSTALL = "uninstall";
public static final List<String> VALID_REQUESTS = Arrays.asList(INSTALL, UNINSTALL);
public final String request;
public final String packageName;
@Nullable
public final Integer versionCode;
/**
* Create a new instance. {@code request} is validated against the list of
* valid install requests. {@code packageName} has a safety validation to
* make sure that only valid Android/Java Package Name characters are included.
* If validation fails, the the values are set to {@code null}, which are
* handled in {@link org.fdroid.fdroid.IndexV1Updater#processRepoPushRequests(List)}
* or {@link org.fdroid.fdroid.IndexUpdater#processRepoPushRequests(List)}
*/
public RepoPushRequest(String request, String packageName, @Nullable String versionCode) {
this.request = request;
this.packageName = packageName;
if (VALID_REQUESTS.contains(request)) {
this.request = request;
} else {
this.request = null;
}
if (Utils.isSafePackageName(packageName)) {
this.packageName = packageName;
} else {
this.packageName = null;
}
Integer i;
try {

20
app/src/main/java/org/fdroid/fdroid/RepoXMLHandler.java → app/src/main/java/org/fdroid/fdroid/data/RepoXMLHandler.java
View File

@ -17,15 +17,12 @@
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
package org.fdroid.fdroid;
package org.fdroid.fdroid.data;
import android.os.Build;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import org.fdroid.fdroid.data.Apk;
import org.fdroid.fdroid.data.App;
import org.fdroid.fdroid.data.Repo;
import org.fdroid.fdroid.data.RepoPushRequest;
import org.fdroid.fdroid.Utils;
import org.fdroid.fdroid.data.Schema.ApkTable;
import org.xml.sax.Attributes;
import org.xml.sax.SAXException;
@ -332,8 +329,8 @@ public class RepoXMLHandler extends DefaultHandler {
}
@Override
public void startElement(String uri, String localName, String qName,
Attributes attributes) throws SAXException {
public void startElement(String uri, String localName, String qName, Attributes attributes)
throws SAXException {
super.startElement(uri, localName, qName, attributes);
if ("repo".equals(localName)) {
@ -344,8 +341,7 @@ public class RepoXMLHandler extends DefaultHandler {
repoDescription = cleanWhiteSpace(attributes.getValue("", "description"));
repoTimestamp = parseLong(attributes.getValue("", "timestamp"), 0);
repoIcon = attributes.getValue("", "icon");
} else if (RepoPushRequest.INSTALL.equals(localName)
|| RepoPushRequest.UNINSTALL.equals(localName)) {
} else if (RepoPushRequest.VALID_REQUESTS.contains(localName)) {
if (repo.pushRequests == Repo.PUSH_REQUEST_ACCEPT_ALWAYS) {
RepoPushRequest r = new RepoPushRequest(
localName,
@ -356,7 +352,11 @@ public class RepoXMLHandler extends DefaultHandler {
} else if ("application".equals(localName) && curapp == null) {
curapp = new App();
curapp.repoId = repo.getId();
curapp.packageName = attributes.getValue("", "id");
try {
curapp.setPackageName(attributes.getValue("", "id"));
} catch (IllegalArgumentException e) {
throw new SAXException(e);
}
// To appease the NON NULL constraint in the DB. Usually there is a description, and it
// is quite difficult to get an app to _not_ have a description when using fdroidserver.

87
app/src/test/java/org/fdroid/fdroid/updater/RepoXMLHandlerTest.java → app/src/test/java/org/fdroid/fdroid/data/RepoXMLHandlerTest.java
View File

@ -20,23 +20,25 @@
* MA 02110-1301, USA.
*/
package org.fdroid.fdroid.updater;
package org.fdroid.fdroid.data;
import android.support.annotation.NonNull;
import android.text.TextUtils;
import android.util.Log;
import org.apache.commons.io.FileUtils;
import org.fdroid.fdroid.BuildConfig;
import org.fdroid.fdroid.data.Apk;
import org.fdroid.fdroid.data.App;
import org.fdroid.fdroid.data.Repo;
import org.fdroid.fdroid.data.RepoPushRequest;
import org.fdroid.fdroid.mock.MockRepo;
import org.fdroid.fdroid.mock.RepoDetails;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.robolectric.RobolectricTestRunner;
import org.robolectric.annotation.Config;
import org.xml.sax.InputSource;
import org.xml.sax.XMLReader;
import javax.xml.parsers.SAXParser;
import javax.xml.parsers.SAXParserFactory;
import java.io.BufferedInputStream;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
@ -52,6 +54,7 @@ import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
@Config(constants = BuildConfig.class)
@RunWith(RobolectricTestRunner.class)
@ -128,6 +131,33 @@ public class RepoXMLHandlerTest {
});
}
@Test(expected = IllegalArgumentException.class)
public void testSimpleIndexWithCorruptedPackageName() throws Throwable {
Repo expectedRepo = new Repo();
expectedRepo.name = "F-Droid";
expectedRepo.signingCertificate = "308201ee30820157a0030201020204300d845b300d06092a864886f70d01010b0500302a3110300e060355040b1307462d44726f6964311630140603550403130d70616c6174736368696e6b656e301e170d3134303432373030303633315a170d3431303931323030303633315a302a3110300e060355040b1307462d44726f6964311630140603550403130d70616c6174736368696e6b656e30819f300d06092a864886f70d010101050003818d0030818902818100a439472e4b6d01141bfc94ecfe131c7c728fdda670bb14c57ca60bd1c38a8b8bc0879d22a0a2d0bc0d6fdd4cb98d1d607c2caefbe250a0bd0322aedeb365caf9b236992fac13e6675d3184a6c7c6f07f73410209e399a9da8d5d7512bbd870508eebacff8b57c3852457419434d34701ccbf692267cbc3f42f1c5d1e23762d790203010001a321301f301d0603551d0e041604140b1840691dab909746fde4bfe28207d1cae15786300d06092a864886f70d01010b05000381810062424c928ffd1b6fd419b44daafef01ca982e09341f7077fb865905087aeac882534b3bd679b51fdfb98892cef38b63131c567ed26c9d5d9163afc775ac98ad88c405d211d6187bde0b0d236381cc574ba06ef9080721a92ae5a103a7301b2c397eecc141cc850dd3e123813ebc41c59d31ddbcb6e984168280c53272f6a442b"; // NOCHECKSTYLE LineLength
expectedRepo.description = "The official repository of the F-Droid client. Applications in this repository are either official binaries built by the original application developers, or are binaries built from source by the admin of f-droid.org using the tools on https://gitorious.org/f-droid."; // NOCHECKSTYLE LineLength
expectedRepo.timestamp = 1398733213;
InputStream inputStream = getClass().getClassLoader()
.getResourceAsStream("simpleIndexWithCorruptedPackageName.xml");
SAXParserFactory factory = SAXParserFactory.newInstance();
factory.setNamespaceAware(true);
SAXParser parser = factory.newSAXParser();
XMLReader reader = parser.getXMLReader();
RepoDetails repoDetails = new RepoDetails();
MockRepo mockRepo = new MockRepo(100, Repo.PUSH_REQUEST_IGNORE);
RepoXMLHandler handler = new RepoXMLHandler(mockRepo, repoDetails);
reader.setContentHandler(handler);
InputSource is = new InputSource(new BufferedInputStream(inputStream));
try {
reader.parse(is);
} catch (org.xml.sax.SAXException e) {
throw e.getCause();
}
fail();
}
@Test
public void testPushRequestsRepoIgnore() {
Repo expectedRepo = new Repo();
@ -160,6 +190,51 @@ public class RepoXMLHandlerTest {
assertEquals(6, repoPushRequests.size());
}
@Test
public void testPushRequestsRepoCorruption() {
RepoPushRequest repoPushRequest;
repoPushRequest = new RepoPushRequest(null, null, null); // request with no data
assertEquals(repoPushRequest.request, null);
assertEquals(repoPushRequest.packageName, null);
assertEquals(repoPushRequest.versionCode, null);
repoPushRequest = new RepoPushRequest("install", "org.fdroid.fdroid", "999999999999");
assertEquals(repoPushRequest.versionCode, null);
repoPushRequest = new RepoPushRequest("install", "org.fdroid.fdroid",
String.valueOf(((long) Integer.MAX_VALUE) + 1));
assertEquals(repoPushRequest.versionCode, null);
repoPushRequest = new RepoPushRequest("install", "org.fdroid.fdroid",
String.valueOf(((long) Integer.MIN_VALUE) - 1));
assertEquals(repoPushRequest.versionCode, null);
repoPushRequest = new RepoPushRequest("Robert'); DROP TABLE Students; --", "org.fdroid.fdroid", null);
assertEquals(repoPushRequest.request, null);
assertEquals(repoPushRequest.packageName, "org.fdroid.fdroid");
assertEquals(repoPushRequest.versionCode, null);
repoPushRequest = new RepoPushRequest("install", "Robert'); DROP TABLE Students; --", "123.1.1");
assertEquals(repoPushRequest.request, "install");
assertEquals(repoPushRequest.packageName, null);
assertEquals(repoPushRequest.versionCode, null);
repoPushRequest = new RepoPushRequest("install", "--", "123");
assertEquals(repoPushRequest.request, "install");
assertEquals(repoPushRequest.packageName, null);
assertEquals(repoPushRequest.versionCode, Integer.valueOf(123));
repoPushRequest = new RepoPushRequest("uninstall", "Robert'); DROP TABLE Students; --", "123");
assertEquals(repoPushRequest.request, "uninstall");
assertEquals(repoPushRequest.packageName, null);
assertEquals(repoPushRequest.versionCode, Integer.valueOf(123));
repoPushRequest = new RepoPushRequest("badrquest", "asdfasdfasdf", "123");
assertEquals(repoPushRequest.request, null);
assertEquals(repoPushRequest.packageName, "asdfasdfasdf");
assertEquals(repoPushRequest.versionCode, Integer.valueOf(123));
}
@Test
public void testMediumRepo() {
Repo expectedRepo = new Repo();
@ -839,7 +914,7 @@ public class RepoXMLHandlerTest {
}
@NonNull
static RepoDetails getFromFile(ClassLoader classLoader, String indexFilename, int pushRequests) {
public static RepoDetails getFromFile(ClassLoader classLoader, String indexFilename, int pushRequests) {
Log.i(TAG, "test file: " + classLoader.getResource(indexFilename));
InputStream inputStream = classLoader.getResourceAsStream(indexFilename);
return RepoDetails.getFromFile(inputStream, pushRequests);

22
app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java
View File

@ -8,6 +8,7 @@ import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.ObjectReader;
import org.apache.commons.io.IOUtils;
@ -25,6 +26,7 @@ import org.fdroid.fdroid.data.InstalledAppTestUtils;
import org.fdroid.fdroid.data.Repo;
import org.fdroid.fdroid.data.RepoProvider;
import org.fdroid.fdroid.data.RepoPushRequest;
import org.fdroid.fdroid.data.RepoXMLHandlerTest;
import org.fdroid.fdroid.mock.RepoDetails;
import org.junit.Before;
import org.junit.Test;
@ -158,6 +160,26 @@ public class IndexV1UpdaterTest extends FDroidProviderTest {
getClass().getResourceAsStream("foo");
}
@Test(expected = IllegalArgumentException.class)
public void testIndexV1WithCorruptAppPackageName() throws Throwable {
try {
testBadTestyJar("testy.at.or.at_corrupt_app_package_name_index-v1.jar");
} catch (JsonMappingException e) {
throw e.getCause();
}
fail();
}
@Test(expected = IllegalArgumentException.class)
public void testIndexV1WithCorruptPackageName() throws Throwable {
try {
testBadTestyJar("testy.at.or.at_corrupt_package_name_index-v1.jar");
} catch (JsonMappingException e) {
throw e.getCause();
}
fail();
}
@Test(expected = IndexUpdater.SigningException.class)
public void testIndexV1WithBadTestyJarNoManifest() throws IOException, IndexUpdater.UpdateException {
testBadTestyJar("testy.at.or.at_no-MANIFEST.MF_index-v1.jar");

4
app/src/test/resources/simpleIndexWithCorruptedPackageName.xml Normal file
View File

File diff suppressed because one or more lines are too long

BIN
app/src/test/resources/testy.at.or.at_corrupt_app_package_name_index-v1.jar Normal file
View File

Binary file not shown.

BIN
app/src/test/resources/testy.at.or.at_corrupt_package_name_index-v1.jar Normal file
View File

Binary file not shown.

2
app/src/testShared/java/org/fdroid/fdroid/mock/RepoDetails.java
View File

@ -2,7 +2,7 @@ package org.fdroid.fdroid.mock;
import android.support.annotation.NonNull;
import org.fdroid.fdroid.RepoXMLHandler;
import org.fdroid.fdroid.data.RepoXMLHandler;
import org.fdroid.fdroid.data.Apk;
import org.fdroid.fdroid.data.App;
import org.fdroid.fdroid.data.RepoPushRequest;