Potential security hazard: `apk_signer_fingerprint()` looks at certs in reverse order that Android checks them

Take a look at apk_signer_fingerprint():

def get_first_signer_certificate(apkpath):
    """Get the first signing certificate from the APK, DER-encoded."""
    certs = None
    cert_encoded = None
    with zipfile.ZipFile(apkpath, 'r') as apk:
        cert_files = [n for n in apk.namelist() if SIGNATURE_BLOCK_FILE_REGEX.match(n)]
        if len(cert_files) > 1:
            logging.error(_("Found multiple JAR Signature Block Files in {path}").format(path=apkpath))
            return None
        elif len(cert_files) == 1:
            cert_encoded = get_certificate(apk.read(cert_files[0]))

    if not cert_encoded and use_androguard():
        apkobject = _get_androguard_APK(apkpath)
        certs = apkobject.get_certificates_der_v2()
        if len(certs) > 0:
            logging.debug(_('Using APK Signature v2'))
            cert_encoded = certs[0]
        if not cert_encoded:
            certs = apkobject.get_certificates_der_v3()
            if len(certs) > 0:
                logging.debug(_('Using APK Signature v3'))
                cert_encoded = certs[0]

    if not cert_encoded:
        logging.error(_("No signing certificates found in {path}").format(path=apkpath))
        return None
    return cert_encoded


def apk_signer_fingerprint(apk_path):
    """Obtain sha256 signing-key fingerprint for APK.

    Extracts hexadecimal sha256 signing-key fingerprint string
    for a given APK.

    Parameters
    ----------
    apk_path
        path to APK

    Returns
    -------
    signature fingerprint
    """
    cert_encoded = get_first_signer_certificate(apk_path)
    if not cert_encoded:
        return None
    return signer_fingerprint(cert_encoded)

These are the functions that are used to make sure APKs are signed by a given signer and are hence somewhat important from a security perspective.

However, there's a discrepancy between how these certificates are extracted and how Android actually implements signature checks. Here's an image from the Google documentation:

v3v2v1

Notice how it checks v3, then v2, and then v1. Yet the code above looks at v1, then v2, and then v3, in reverse order. So v1 could have a bogus signer that some versions of Android never even look at, yet fdroid makes a security decision based on it. Yikes! Also, it's worth noting that apk_signer_fingerprint() also does not bother validating that the signatures are correct.

There are some mitigating factors, however:

  • Sometimes a previous phase /might/ check that the signatures are correct using apksigner (or in some configurations, old school jarsigner, which is its own concern in itself).
  • Some versions of apksigner appear to make sure v1==v2==v3 when validating.

That seems kind of brittle to rely on, though. Instead, I'd recommend replacing this function with a boring call to apksigner verify --print-certs and not implement that kind of highly sensitive moving target in fdroid itself.