Merge branch 'little-security-fixes' into 'master'

Little security fixes

Closes #555

See merge request fdroid/fdroidserver!572

.gitlab-ci.yml

@@ -133,7 +133,7 @@ lint_format_safety_bandit_checks:
         -ii
         -s B110,B310,B322,B404,B408,B410,B603,B607
         -x fdroidserver/dscanner.py,docker/install_agent.py,docker/drozer.py
-        -r $CI_PROJECT_DIR
+        -r $CI_PROJECT_DIR fdroid
         || export EXITVALUE=1
     - safety check --full-report || export EXITVALUE=1
     - pylint --rcfile=.pylint-rcfile --output-format=colorized --reports=n

fdroidserver/common.py

@@ -75,6 +75,10 @@ VERCODE_OPERATION_RE = re.compile(r'^([ 0-9/*+-]|%c)+$')
 CERT_PATH_REGEX = re.compile(r'^META-INF/.*\.(DSA|EC|RSA)$')
 APK_NAME_REGEX = re.compile(r'^([a-zA-Z][\w.]*)_(-?[0-9]+)_?([0-9a-f]{7})?\.apk')
 STANDARD_FILE_NAME_REGEX = re.compile(r'^(\w[\w.]*)_(-?[0-9]+)\.\w+')
+FDROID_PACKAGE_NAME_REGEX = re.compile(r'''^[a-f0-9]+$''', re.IGNORECASE)
+STRICT_APPLICATION_ID_REGEX = re.compile(r'''(?:^[a-z_]+(?:\d*[a-zA-Z_]*)*)(?:\.[a-z_]+(?:\d*[a-zA-Z_]*)*)*$''')
+VALID_APPLICATION_ID_REGEX = re.compile(r'''(?:^[a-z_]+(?:\d*[a-zA-Z_]*)*)(?:\.[a-z_]+(?:\d*[a-zA-Z_]*)*)*$''',
+                                        re.IGNORECASE)
 
 MAX_VERSION_CODE = 0x7fffffff  # Java's Integer.MAX_VALUE (2147483647)
 
@@ -1516,8 +1520,12 @@ def parse_androidmanifests(paths, app):
     if max_version is None:
         max_version = "Unknown"
 
-    if max_package and not is_valid_java_package_name(max_package):
+    if max_package:
-        raise FDroidException(_("Invalid package name {0}").format(max_package))
+        msg = _("Invalid package name {0}").format(max_package)
+        if not is_valid_package_name(max_package):
+            raise FDroidException(msg)
+        elif not is_strict_application_id(max_package):
+            logging.warning(msg)
 
     return (max_version, max_vercode, max_package)
 
@@ -1530,12 +1538,25 @@ def is_valid_package_name(name):
     files use the SHA-256 sum.
 
     """
-    return re.match("^([a-f0-9]+|[A-Za-z_][A-Za-z_0-9.]+)$", name)
+    return VALID_APPLICATION_ID_REGEX.match(name) is not None \
+        or FDROID_PACKAGE_NAME_REGEX.match(name) is not None
 
 
-def is_valid_java_package_name(name):
+def is_strict_application_id(name):
-    """Check whether name is a valid Java package name aka Application ID"""
+    """Check whether name is a valid Android Application ID
-    return re.match("^[A-Za-z_][A-Za-z_0-9.]+$", name)
+
+    The Android ApplicationID is basically a Java Package Name, but
+    with more restrictive naming rules:
+
+    * It must have at least two segments (one or more dots).
+    * Each segment must start with a letter.
+    * All characters must be alphanumeric or an underscore [a-zA-Z0-9_].
+
+    https://developer.android.com/studio/build/application-id
+
+    """
+    return STRICT_APPLICATION_ID_REGEX.match(name) is not None \
+        and '.' in name
 
 
 def getsrclib(spec, srclib_dir, subdir=None, basepath=False,

fdroidserver/lint.py

@@ -488,7 +488,13 @@ def check_for_unsupported_metadata_files(basedir=""):
             if not exists:
                 print(_('"%s/" has no matching metadata file!') % f)
                 return_value = True
-        elif not os.path.splitext(f)[1][1:] in formats:
+        elif os.path.splitext(f)[1][1:] in formats:
+            packageName = os.path.splitext(os.path.basename(f))[0]
+            if not common.is_valid_package_name(packageName):
+                print('"' + packageName + '" is an invalid package name!\n'
+                      + 'https://developer.android.com/studio/build/application-id')
+                return_value = True
+        else:
             print('"' + f.replace(basedir, '')
                   + '" is not a supported file format: (' + ','.join(formats) + ')')
             return_value = True

fdroidserver/update.py

@@ -23,8 +23,10 @@ import sys
 import os
 import shutil
 import glob
+import logging
 import re
 import socket
+import warnings
 import zipfile
 import hashlib
 import json
@@ -36,9 +38,6 @@ from argparse import ArgumentParser
 import collections
 from binascii import hexlify
 
-from PIL import Image, PngImagePlugin
-import logging
-
 from . import _
 from . import common
 from . import index
@@ -46,6 +45,10 @@ from . import metadata
 from .common import SdkToolsPopen
 from .exception import BuildException, FDroidException
 
+from PIL import Image, PngImagePlugin
+warnings.simplefilter('error', Image.DecompressionBombWarning)
+Image.MAX_IMAGE_PIXELS = 0xffffff  # 4096x4096
+
 METADATA_VERSION = 20
 
 # less than the valid range of versionCode, i.e. Java's Integer.MIN_VALUE
@@ -1064,9 +1067,12 @@ def scan_apk(apk_file):
     else:
         scan_apk_aapt(apk, apk_file)
 
-    if not common.is_valid_java_package_name(apk['packageName']):
+    if not common.is_valid_package_name(apk['packageName']):
         raise BuildException(_("{appid} from {path} is not a valid Java Package Name!")
                              .format(appid=apk['packageName'], path=apk_file))
+    elif not common.is_strict_application_id(apk['packageName']):
+        logging.warning(_("{appid} from {path} is not a valid Java Package Name!")
+                        .format(appid=apk['packageName'], path=apk_file))
 
     # Get the signature, or rather the signing key fingerprints
     logging.debug('Getting signature of {0}'.format(os.path.basename(apk_file)))

tests/common.TestCase

@@ -159,8 +159,10 @@ class CommonTest(unittest.TestCase):
                              "debuggable APK state was not properly parsed!")
 
     def test_is_valid_package_name(self):
-        for name in ["org.fdroid.fdroid",
+        for name in ["cafebabe",
+                     "org.fdroid.fdroid",
                      "org.f_droid.fdr0ID",
+                     "SpeedoMeterApp.main",
                      "05041684efd9b16c2888b1eddbadd0359f655f311b89bdd1737f560a10d20fb8"]:
             self.assertTrue(fdroidserver.common.is_valid_package_name(name),
                             "{0} should be a valid package name".format(name))
@@ -171,18 +173,22 @@ class CommonTest(unittest.TestCase):
             self.assertFalse(fdroidserver.common.is_valid_package_name(name),
                              "{0} should not be a valid package name".format(name))
 
-    def test_is_valid_java_package_name(self):
+    def test_is_strict_application_id(self):
+        """see also tests/valid-package-names/"""
         for name in ["org.fdroid.fdroid",
                      "org.f_droid.fdr0ID"]:
-            self.assertTrue(fdroidserver.common.is_valid_java_package_name(name),
+            self.assertTrue(fdroidserver.common.is_strict_application_id(name),
-                            "{0} should be a valid package name".format(name))
+                            "{0} should be a strict application id".format(name))
         for name in ["0rg.fdroid.fdroid",
                      ".f_droid.fdr0ID",
+                     "oneword",
+                     "cafebabe",
+                     "SpeedoMeterApp.main",
                      "org.fdroid/fdroid",
                      "/org.fdroid.fdroid",
                      "05041684efd9b16c2888b1eddbadd0359f655f311b89bdd1737f560a10d20fb8"]:
-            self.assertFalse(fdroidserver.common.is_valid_java_package_name(name),
+            self.assertFalse(fdroidserver.common.is_strict_application_id(name),
-                             "{0} should not be a valid package name".format(name))
+                             "{0} should not be a strict application id".format(name))
 
     def test_prepare_sources(self):
         testint = 99999999

tests/valid-package-names/RandomPackageNames.java

@@ -0,0 +1,237 @@
+
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.Writer;
+import java.io.InputStreamReader;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.io.OutputStreamWriter;
+import java.io.UnsupportedEncodingException;
+import java.nio.ByteBuffer;
+import java.nio.CharBuffer;
+import java.nio.IntBuffer;
+import java.util.Random;
+
+// apt-get install libcommons-lang3-java
+//import org.apache.commons.lang3.RandomStringUtils;
+
+public class RandomPackageNames {
+
+    private static Writer validWriter;
+    private static Writer invalidWriter;
+
+    private static final String[] py = {
+        "python3", "-c",
+        "import sys,re\n"
+        + "m = re.search(r'''"
+        //        + "^(?:[a-z_]+(?:\\d*[a-zA-Z_]*)*)(?:\\.[a-z_]+(?:\\d*[a-zA-Z_]*)*)*$"
+        + "^[a-z_]+(?:\\d*[a-zA-Z_]*)(?:\\.[a-z_]+(?:\\d*[a-zA-Z_]*)*)*$"
+        + "''', sys.stdin.read())\n"
+        + "if m is not None:\n"
+        + "  with open('/tmp/foo', 'w') as fp:\n"
+        + "    fp.write(m.group() + '\\n')\n"
+        + "sys.exit(m is None)"
+    };
+    
+    public static boolean checkAgainstPython(String packageName)
+        throws IOException, InterruptedException {
+
+        ProcessBuilder pb = new ProcessBuilder(py);
+        Process process = pb.start();
+        OutputStream output = process.getOutputStream();
+        output.write(packageName.getBytes());
+        output.write("\n".getBytes());
+        output.flush();
+        output.close();
+
+        int exitVal = process.waitFor();
+        return exitVal == 0;
+    }
+
+    private static boolean isValidJavaIdentifier(String packageName) {
+        if (packageName.length() == 0 || !Character.isJavaIdentifierStart(packageName.charAt(0))) {
+            //System.out.println("invalid first char: '" + packageName + "'");
+            return false;
+        }
+        for (int codePoint : packageName.codePoints().toArray()) {
+            if (codePoint != 46 && !Character.isJavaIdentifierPart(codePoint)) {
+                //System.out.println("invalid char: '"
+                //                   + new StringBuilder().appendCodePoint(codePoint).toString() + "' "
+                //                   + codePoint);
+                return false;
+            }
+        }
+        return true;
+    }
+
+    private static void write(String packageName) throws IOException {
+        if (isValidJavaIdentifier(packageName)) {
+            validWriter.write(packageName);
+            validWriter.write("\n");
+        } else {
+            invalidWriter.write(packageName);
+            invalidWriter.write("\n");
+        }
+    }
+
+    private static void compare(String packageName)
+        throws IOException, InterruptedException {
+        boolean python = checkAgainstPython(packageName);
+        boolean java = isValidJavaIdentifier(packageName);
+        if (python && !java) {
+            System.out.println("MISMATCH: '" + packageName + "' "
+                               + (python ? "py:✔" : "py:☹") + " "
+                               + (java ? "ja:✔" : "ja:☹") + " ");
+        }
+    }
+    
+    public static void main (String[] args)
+        throws IOException, InterruptedException, UnsupportedEncodingException {
+        int[] data;
+        byte[] bytes;
+        ByteBuffer byteBuffer;
+        Random random = new Random();
+
+        validWriter = new OutputStreamWriter(new FileOutputStream("valid.txt"), "UTF-8");
+        invalidWriter = new OutputStreamWriter(new FileOutputStream("invalid.txt"), "UTF-8");
+        
+        //System.out.print(".");
+
+        char[] validFirstLetters = new char[27];
+        validFirstLetters[0] = 95; // _
+        for (int i = 1; i < 27; i++) {
+            validFirstLetters[i] = (char) (i + 96);
+        }
+
+        char[] validLetters = new char[64];
+        int j = 0;
+        for (char c = 32; c < 123; c++) {
+            if ((c == 46) || (c > 47 && c < 58) || (c > 64 && c < 91) || (c > 96)) {
+                validLetters[j] = c;
+                j++;
+            }
+        }
+
+        for (File f : new File("/home/hans/code/fdroid/fdroiddata/metadata").listFiles()) {
+            String name = f.getName();
+            if (name.endsWith(".yml") || name.endsWith(".txt")) {
+                compare(name.substring(0, name.length() - 4));
+            }
+        }
+        compare("SpeedoMeterApp.main");
+        compare("uk.co.turtle-player");
+        compare("oVPb");
+        compare(" _LS");
+        compare("r.vq");
+        compare("r.vQ");
+        compare("ra.vQ");
+        compare("s.vQ");
+        compare("r.tQ");
+        compare("r.vR");
+        compare("any.any");
+        compare("org.fdroid.fdroid");
+        compare("me.unfollowers.droid");
+        compare("me_.unfollowers.droid");
+        compare("me._unfollowers.droid");
+        compare("me.unfo11llowers.droid");
+        compare("me11.unfollowers.droid");
+        compare("m11e.unfollowers.droid");
+        compare("1me.unfollowers.droid");
+        compare("me.unfollowers23.droid");
+        compare("me.unfollowers.droid23d");
+        compare("me.unfollowers_.droid");
+        compare("me.unfollowers._droid");
+        compare("me.unfollowers_._droid");
+        compare("me.unfollowers.droid_");
+        compare("me.unfollowers.droid32");
+        compare("me.unfollowers.droid/");
+        compare("me:.unfollowers.droid");
+        compare(":me.unfollowers.droid");
+        compare("me.unfollowers.dro;id");
+        compare("me.unfollowe^rs.droid");
+        compare("me.unfollowers.droid.");
+        compare("me.unfollowers..droid");
+        compare("me.unfollowers.droid._");
+        compare("me.unfollowers.11212");
+        compare("me.1.unfollowers.11212");
+        compare("me..unfollowers.11212");
+        compare("abc");
+        compare("abc.");
+        compare(".abc");
+
+        for (int i = 0; i < 300000; i++) {
+            String packageName;
+
+            int count = random.nextInt(10) + 1;
+            byte valid = (byte) random.ints(97, 122).limit(1).toArray()[0];
+
+            // only valid
+            data = random.ints(46, 122)
+                .limit(count)
+                .filter(c -> (c == 46) || (c > 47 && c < 58) || (c > 64 && c < 91) || (c > 96))
+                .toArray();
+            byteBuffer = ByteBuffer.allocate(data.length);
+            for (int value : data) {
+                byteBuffer.put((byte)value);
+            }
+            if (data.length > 0) {
+                bytes = byteBuffer.array();
+                bytes[0] = valid;
+                packageName = new String(byteBuffer.array(), "UTF-8");
+                //System.out.println(packageName + ": " + isValidJavaIdentifier(packageName));
+                compare(packageName);
+                write(packageName);
+            }
+
+            // full US-ASCII
+            data = random.ints(32, 126).limit(count).toArray();
+            byteBuffer = ByteBuffer.allocate(data.length);
+            for (int value : data) {
+                byteBuffer.put((byte)value);
+            }
+            bytes = byteBuffer.array();
+            packageName = new String(bytes, "UTF-8");
+            //System.out.println(packageName + ": " + isValidJavaIdentifier(packageName));
+            compare(packageName);
+            write(packageName);
+
+            // full US-ASCII with valid first letter
+            data = random.ints(32, 127).limit(count).toArray();
+            byteBuffer = ByteBuffer.allocate(data.length * 4);
+            byteBuffer.asIntBuffer().put(data);
+            bytes = byteBuffer.array();
+            bytes[0] = valid;
+            packageName = new String(bytes, "UTF-8");
+            //System.out.println(packageName + ": " + isValidJavaIdentifier(packageName));
+            compare(packageName);
+            write(packageName);
+
+            // full unicode
+            data = random.ints(32, 0xFFFD).limit(count).toArray();
+            byteBuffer = ByteBuffer.allocate(data.length * 4);
+            byteBuffer.asIntBuffer().put(data);
+            packageName = new String(byteBuffer.array(), "UTF-32");
+            //System.out.println(packageName + ": " + isValidJavaIdentifier(packageName));
+            compare(packageName);
+            write(packageName);
+
+            // full unicode with valid first letter
+            data = random.ints(32, 0xFFFD).limit(count).toArray();
+            byteBuffer = ByteBuffer.allocate(data.length * 4);
+            byteBuffer.asIntBuffer().put(data);
+            bytes = byteBuffer.array();
+            bytes[0] = 0;
+            bytes[1] = 0;
+            bytes[2] = 0;
+            bytes[3] = 120;
+            packageName = new String(bytes, "UTF-32");
+            //System.out.println(packageName + ": " + isValidJavaIdentifier(packageName));
+            compare(packageName);
+            write(packageName);
+        }
+
+        validWriter.close();
+        invalidWriter.close();
+    }
+}

tests/valid-package-names/random-package-names

@@ -0,0 +1,10 @@
+#!/bin/sh
+
+set -e
+set -x
+
+export CLASSPATH=/usr/share/java/commons-lang3.jar:.
+
+cd $(dirname $0)
+javac -classpath $CLASSPATH RandomPackageNames.java
+java -classpath $CLASSPATH  RandomPackageNames

tests/valid-package-names/test.py

@@ -0,0 +1,32 @@
+#!/usr/bin/env python3
+
+import re
+
+
+def test(packageName):
+    m = ANDROID_APPLICATION_ID_REGEX.match(packageName.strip())
+    return m is not None
+
+
+ANDROID_APPLICATION_ID_REGEX = re.compile(r'''(?:^[a-z_]+(?:\d*[a-zA-Z_]*)*)(?:\.[a-z_]+(?:\d*[a-zA-Z_]*)*)*$''')
+valid = 0
+invalid = 0
+
+test('org.fdroid.fdroid')
+with open('valid.txt', encoding="utf-8") as fp:
+    for packageName in fp:
+        packageName = packageName.strip()
+        if not test(packageName):
+            valid += 1
+            # print('should be valid:', packageName)
+
+with open('invalid.txt', encoding="utf-8") as fp:
+    for packageName in fp:
+        packageName = packageName.strip()
+        if test(packageName):
+            invalid += 1
+            print('should be not valid: "' + packageName + '"')
+
+
+print(valid, 'Java thinks is valid, but the Android regex does not')
+print(invalid, 'invalid mistakes')