Java: Insecure Loading of Class in Android App without Package Signature Checking

[masterofnow]

If a vulnerable loads classes or code of any app based solely on the package name of the app without first checking the package signature of the app, this could malicious app with the same package name to be loaded through "package namespace squatting".
If the victim user install such malicious app in the same device as the vulnerable app, the vulnerable app would load
classes or code from the malicious app, potentially leading to arbitrary code execution.

Although both uses OVAA as a target application, this QL is different from the one in #5435 that created UnsafeReflection.ql as they target different category of vulnerability.

[masterofnow]

Minor correction. I referred #5435 to when I meant to refer to #4947, in CWE-094, which also identified vulnerability in https://github.com/oversecured/ovaa.

[masterofnow]

This blog https://blog.oversecured.com/Android-arbitrary-code-execution-via-third-party-package-contexts/ gives an overview for the type of vulnerability this PR meant to identify.

[atorralba]

Hey @masterofnow, thanks for your contribution.

First of fall, I made some suggestions below simply about code optimization. Note that I:

• renamed the deprecated MethodAccess to the new MethodCall.
• Used hasQualifiedName to combine package, type and name checks for Methods. Also used string lists instead of disjunctions.
• Used the predicates' result types to avoid unnecessary instanceof checks and casts.
• Removed superfluous predicates.
• Removed unnecessary exists variables where possible.

Now, the query will still need work after all that. You're combining global flow, local flow, syntactic AST variable relations, and CFG relations to determine flow between two expressions in several predicates (namely, getClassLoaderReachableMethodAccess, getDangerousReachableMethodAccess, isSignaturesChecked , and the select clause). I'd recommend picking one mechanism and sticking to that everywhere, since the logic is the same all the time. If you don't want to use several global flow configurations (which would admittedly be overkill), you can stick to 1 global flow configuration + local flow.

Also, take another look at your SignaturePackageConfig, because I don't think it's doing what you want it to do (the source being an argument doesn't look right to me).

If you have doubts, we can further discuss all that once you've applied this first round of suggestions.

[masterofnow]

Changed Expr sinkPackageContext to DataFlow::Node sinkPackageContext, due to the following error:
Error: Failed to run query: ERROR: Expr::Expr is not compatible with DataFlowNodes::Public::Node

[masterofnow]

Changed
TaintTracking::localExprTaint(lvdePackageContext.getAnAccess(), sinkPackageContext) and
to
TaintTracking::localTaint(DataFlow::exprNode(lvdePackageContext.getAnAccess()), sinkPackageContext) and
to resolve the following error
Error: Failed to run query: ERROR: DataFlowNodes::Public::Node is not compatible with Expr::Expr

[masterofnow]

Hi @atorralba, thanks for your code review and suggestion.

I have commited all your suggestion.
However, there are a few glitches when I try to run it locally on my vscode codeql.
In particular, somehow MethodCall had to be reverted back to MethodAccess to run.

As a result, I had to do some minor adjustment and push a new commit. I have added some comments in the code change to explain why they were needed.

My codeql version in vscode is as follows:

CLI command succeeded.
Found compatible version of CodeQL CLI (version 2.15.4)

[atorralba]

Suggested change

	(maCreatePackageContext.getCallee().getDeclaringType().getQualifiedName() = "android.content.ContextWrapper" or
	maCreatePackageContext.getCallee().getDeclaringType().getQualifiedName() = "android.content.Context") and
	maCreatePackageContext.getCallee().getName() = "createPackageContext" and
	sink.asExpr() = maCreatePackageContext.getArgument(0)
	)
	maCreatePackageContext.getMethod().hasQualifiedName("android.content", ["ContextWrapper", "Context"], "createPackageContext") and
	sink.asExpr() = maCreatePackageContext.getArgument(0)
	)

[atorralba]

Ok, now that the code is somewhat tidy (and please, remember to autoformat your code in VSCode using > Format Document), let's reason about your intentions.

The pattern you're trying to model is:

Context appContext = context.createPackageContext(packageName, ...); // source
ClassLoader classLoader = appContext.getClassLoader(); // step
try {
    Object result = classLoader.loadClass("some.package.SomeClass") // suggested sink
            .getMethod("someMethod")
            .invoke(null); // your sink

where the signature of packageName hasn't been verified.

We can express this as a dataflow problem, where the source is the Context obtained from a createPackageContext call with a certain packageName argument. So a MethodAccess that calls the method android.content.Context.createPackageContext (or an override).

predicate isSource(DataFlow::Node src) {
  exists(Method m | m = src.asExpr().(MethodAccess).getMethod() |
    m.getDeclaringType().getASourceSupertype*() instanceof TypeContext and
    m.hasName("createPackageContext")
  )
}

Now we can track it to the qualifier of any problematic access of the ClassLoader class, such as ClassLoader.loadClass, and that'll be our sink:

predicate isSink(DataFlow::Node sink) {
  exists(MethodAccess ma |
    ma.getMethod().hasQualifiedName("java.lang", "ClassLoader", "loadClass")
  |
    sink.asExpr() = ma.getQualifier()
  )
}

Of course we could make it an intermediate step, and keep tracking the Class object until it reached the qualifier of another dangerous access, like getMethod, getDeclaredMethod, etc — and repeat until we found invoke or similar. But that unnecessarily complicates the query — if a class is loaded from an untrusted ClassLoader, that's bad enough, and we can assume that some interaction with it will happen, which would cause the vulnerability.

Nonetheless the source is a Context object, and the sink is a ClassLoader object, so we do need to model how the flow can go from one to the other. That can be done with an additional flow step:

predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
  exists(MethodAccess ma, Method m |
    ma.getMethod() = m and
    m.getDeclaringType().getASourceSupertype*() instanceof TypeContext and
    m.hasName("getClassLoader")
  |
    node1.asExpr() = ma.getQualifier() and
    node2.asExpr() = ma
  )
}

Now, to model the safe cases so that we don't report FPs. This is an example of a validation that would prevent this vulnerability:

PackageManager packageManager = context.getPackageManager();
for (PackageInfo info : packageManager.getInstalledPackages(0)) {
    String packageName = info.packageName;
    if (packageName.startsWith("some.package.")
            && packageManager.checkSignatures(packageName, context.getPackageName()) == PackageManager.SIGNATURE_MATCH) {

        Context appContext = context.createPackageContext(packageName, ...);
        // ...
    }

There are many ways of modeling this, but since we modeled our source around Context.createPackageContext, we can add the restriction that if the used packageName argument is trusted, then this call isn't a valid source.

To model the validation, you need to define a Guard:

import semmle.code.java.controlflow.Guards

class CheckSignaturesGuard extends Guard instanceof MethodAccess {
  CheckSignaturesGuard() {
    super.getMethod().hasQualifiedName("android.content.pm", "PackageManager", "checkSignatures")
  }

  Expr getCheckedExpr() { result = super.getArgument(0) }
}

And now we can tell that any access to the variable of the first argument of this call (packageName) that is controlled by the guard in its true branch (that is, where checkSignatures has been necessarily executed and its result is true) is safe, and can't be used to build a source:

import semmle.code.java.dataflow.SSA

predicate signatureChecked(Expr safe) {
  exists(CheckSignaturesGuard g, SsaVariable v |
    v.getAUse() = g.getCheckedExpr() and
    safe = v.getAUse() and
    g.controls(safe.getBasicBlock(), true)
  )
}

We can simply add this as a condition to our previous predicate isSource:

predicate isSource(DataFlow::Node src) {
  exists(Method m | m = src.asExpr().(MethodAccess).getMethod() |
    m.getDeclaringType().getASourceSupertype*() instanceof TypeContext and
    m.hasName("createPackageContext")
    and not signatureChecked(ma.getArgument(0))
  )
}

Note that this does only SSA analysis to determine whether the first argument of createPackageContext is safe. If you wanted this to be tracked across callables (i.e. global flow), you'd need to use a secondary DataFlow::Global configuration (similar to your SigPkgCfg but with some adjustments). I leave this as an exercise to you if you want to further improve the query.

For now, try to fit the pieces I gave you into a new TaintTracking::Global configuration, and discard your current
predicates getClassLoaderReachableMethodAccess, getDangerousReachableMethodAccess, and isSignaturesChecked. Try to write your select clause following the style of one of the many existing path-problem queries, and adapt your query metadata accordingly.

To make sure the query works as you expected, I'd recommend writing some tests too. See https://github.com/github/codeql/blob/main/docs/supported-queries.md for some guidance about writing unit tests. You can also get inspiration from other experimental queries in this regard.

[masterofnow]

I can't recall entirely but I remember one of the reason my query ended up combining data flow with CFG etc was that a plain data flow somehow could not identify the vulnerability.
I suspect it may be due to my poor familiarity with CodeQL.

Not sure if it captures what you had in mind correctly, but I tried implement the following but it also return empty result.

 import java
 import semmle.code.java.dataflow.DataFlow
 import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.controlflow.Guards
 import semmle.code.java.dataflow.SSA
 
 
 class CheckSignaturesGuard extends Guard instanceof MethodAccess {
   CheckSignaturesGuard() {
     super.getMethod().hasQualifiedName("android.content.pm", "PackageManager", "checkSignatures")
   }
 
   Expr getCheckedExpr() { result = super.getArgument(0) }
 }
 
 predicate signatureChecked(Expr safe) {
   exists(CheckSignaturesGuard g, SsaVariable v |
     v.getAUse() = g.getCheckedExpr() and
     safe = v.getAUse() and
     g.controls(safe.getBasicBlock(), true)
   )
 }
 
 class TypeContext extends TypeClass {
   TypeContext() {
     this.hasQualifiedName("android.content", "ContextWrapper") or
     this.hasQualifiedName("android.content", "Context")
   }
 }
 
 module InsecureLoadingConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node src) {
     exists(Method m | m = src.asExpr().(MethodAccess).getMethod() |
       m.getDeclaringType().getASourceSupertype*() instanceof TypeContext and
       m.hasName("createPackageContext")
       and not signatureChecked(src.asExpr().(MethodAccess).getArgument(0))
     )
   }
   
   predicate isSink(DataFlow::Node sink) {
     exists(MethodAccess ma |
       ma.getMethod().hasQualifiedName("java.lang", "ClassLoader", "loadClass")
     |
       sink.asExpr() = ma.getQualifier()
     )
   }
 
   
   predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     exists(MethodAccess ma, Method m |
       ma.getMethod() = m and
       m.getDeclaringType().getASourceSupertype*() instanceof TypeContext and
       m.hasName("getClassLoader")
     |
       node1.asExpr() = ma.getQualifier() and
       node2.asExpr() = ma
     )
   }
   
 }
 
 module InsecureLoadFlow = TaintTracking::Global<InsecureLoadingConfig>;
  
 from
   DataFlow::Node source, DataFlow::Node sink
 where
   InsecureLoadFlow::flow(source, sink)
 select
   source, sink

[atorralba]

The reason the query isn't working for you is because of how you defined TypeContext. You made it extend TypeClass, which is literally the type java.lang.Class. That's not what you want — try making it extend RefType or Class (or just use the existing TypeContext from semmle.code.java.frameworks.android.Intent), that should solve the initial issue and show results in your test code.

Now, I made a mistake when defining the checkSignatures guard. checkSignatures doesn't return a boolean but an integer, so the method call can't be a guard in itself, we need to model it around the equality test of said integer. If you change the guard definition to the following, you should see that the good case is also correctly sanitized:

class CheckSignaturesGuard extends Guard instanceof EqualityTest {
  MethodAccess checkSignatures;

  CheckSignaturesGuard() {
    this.getAnOperand() = checkSignatures and
    checkSignatures
        .getMethod()
        .hasQualifiedName("android.content.pm", "PackageManager", "checkSignatures") and
    exists(Expr signatureCheckResult |
      this.getAnOperand() = signatureCheckResult and signatureCheckResult != checkSignatures
    |
      signatureCheckResult.(CompileTimeConstantExpr).getIntValue() = 0 or
      signatureCheckResult
          .(FieldRead)
          .getField()
          .hasQualifiedName("android.content.pm", "PackageManager", "SIGNATURE_MATCH")
    )
  }

  Expr getCheckedExpr() { result = checkSignatures.getArgument(0) }
}

predicate signatureChecked(Expr safe) {
  exists(CheckSignaturesGuard g, SsaVariable v |
    v.getAUse() = g.getCheckedExpr() and
    safe = v.getAUse() and
    g.controls(safe.getBasicBlock(), g.(EqualityTest).polarity())
  )
}

[atorralba]

Also tweak your select clause and metadata like this if you want to see pretty-printed alerts:

/*
 * ...
 * @kind path-problem
 * ...
 */

// ...

import InsecureLoadFlow::PathGraph

from InsecureLoadFlow::PathNode source, InsecureLoadFlow::PathNode sink
where InsecureLoadFlow::flowPath(source, sink)
select sink.getNode(), source, sink, "Class loaded from a $@ without signature check", source.getNode(), "third party library"

[masterofnow]

It works now, thanks!
I have pushed a new commit.

[atorralba]

Before we do the bounty evaluation, do you intend to add unit tests (see last paragraph of this comment) or are you happy with the current state of the query?

[github-actions]

QHelp previews:

java/ql/src/experimental/Security/CWE/CWE-470/LoadClassNoSignatureCheck.qhelp

Load 3rd party classes or code ('unsafe reflection') without signature check

If a vulnerable loads classes or code of any app based solely on the package name of the app without first checking the package signature of the app, this could malicious app with the same package name to be loaded through "package namespace squatting". If the victim user install such malicious app in the same device as the vulnerable app, the vulnerable app would load classes or code from the malicious app, potentially leading to arbitrary code execution.

Recommendation

Verify that the signature of an app in addition to the package name before loading the classes or code.

References

• Oversecured (Android: arbitrary code execution via third-party package contexts)
• Common Weakness Enumeration: CWE-470.

[masterofnow]

I have added the following unit test files:

• java/ql/test/experimental/query-tests/security/CWE-470/LoadClassNoSignatureCheck.qlref
• java/ql/test/experimental/query-tests/security/CWE-470/BadClassLoader.java
• java/ql/test/experimental/query-tests/security/CWE-470/GoodClassLoader.java

Because I am not familiar with the unit test tools. I was not able to properly run them using codeql-cli.
codeql test run kept failing on extraction.

So, what I did was I manually create a database via codeql database create by building BadClassLoader.java and GoodClassLoader.java using gradlew and verified using vscode codeql that the original LoadClassNoSignatureCheck.ql indeed identify BadClassLoader.java and correctly ignore GoodClassLoader.java.

[atorralba]

The reason the tests don't compile for you is that you're importing Android classes but you didn't specify where the compiler should look for them. You can use the options file in the test directory for that — point it to the stubs directory adding something like the following:

:${testdir}/../../../../stubs/google-android-9.0.0

Once you've done that you'll be able to run the test with codeql test run, and then you'll see that the test fails because you didn't add an expectations file. You can accept the output of codeql test run if you're happy with it by adding the --learn flag:

codeql test run --learn java/ql/test/experimental/query-tests/security/CWE-470/LoadClassNoSignatureCheck.qlref

Now that we're at it, and that you have a good and a bad example, I recommend you add them to the QHelp file as <example> elements. See any other .qhelp files for references on how to do this.

[masterofnow]

It worked! Thanks again!
I just pushed a new commit.

[github-actions]

QHelp previews:

java/ql/src/experimental/Security/CWE/CWE-470/LoadClassNoSignatureCheck.qhelp

Load 3rd party classes or code ('unsafe reflection') without signature check

If an application loads classes or code from another app based solely on its package name without first checking its package signature, this could allow a malicious app with the same package name to be loaded through "package namespace squatting". If the victim user install such malicious app in the same device as the vulnerable app, the vulnerable app would load classes or code from the malicious app, potentially leading to arbitrary code execution.

Recommendation

Verify the package signature in addition to the package name before loading any classes or code from another application.

Example

The BadClassLoader class illustrates class loading with the android.content.pm.PackageInfo.packageName.startsWith() method without any check on the package signature.

package poc.sample.classloader;

import android.app.Application;
import android.content.pm.PackageInfo;
import android.content.Context;
import android.util.Log;

public class BadClassLoader extends Application {
    @Override
    public void onCreate() {
        super.onCreate();
        for (PackageInfo p : getPackageManager().getInstalledPackages(0)) {
            try {
                if (p.packageName.startsWith("some.package.")) {
                    Context appContext = createPackageContext(p.packageName,
                            CONTEXT_INCLUDE_CODE | CONTEXT_IGNORE_SECURITY);
                    ClassLoader classLoader = appContext.getClassLoader();
                    Object result = classLoader.loadClass("some.package.SomeClass")
                            .getMethod("someMethod")
                            .invoke(null);
                }
            } catch (Exception e) {
                Log.e("Class loading failed", e.toString());
            }
        }
    }
}

The GoodClassLoader class illustrates class loading with correct package signature check using the android.content.pm.PackageManager.checkSignatures() method.

package poc.sample.classloader;

import android.app.Application;
import android.content.pm.PackageInfo;
import android.content.Context;
import android.content.pm.PackageManager;
import android.util.Log;

public class GoodClassLoader extends Application {
    @Override
    public void onCreate() {
        super.onCreate();
        PackageManager pm = getPackageManager();
        for (PackageInfo p : pm.getInstalledPackages(0)) {
            try {
                if (p.packageName.startsWith("some.package.") &&
                        (pm.checkSignatures(p.packageName, getApplicationContext().getPackageName()) == PackageManager.SIGNATURE_MATCH)
                ) {
                    Context appContext = createPackageContext(p.packageName,
                            CONTEXT_INCLUDE_CODE | CONTEXT_IGNORE_SECURITY);
                    ClassLoader classLoader = appContext.getClassLoader();
                    Object result = classLoader.loadClass("some.package.SomeClass")
                            .getMethod("someMethod")
                            .invoke(null);
                }
            } catch (Exception e) {
                Log.e("Class loading failed", e.toString());
            }
        }
    }
}

References

• Oversecured (Android: arbitrary code execution via third-party package contexts)
• Common Weakness Enumeration: CWE-470.

[atorralba]

Some final docs suggestions. Also note that you need to include the BadClassLoader.java and GoodClasLoader.java files in the java/ql/src/experimental/Security/CWE/CWE-470/ directory too for the QHelp to render correctly.

You can verify that the QHelp is correct with the following command:

codeql generate query-help --format=markdown -- "java/ql/src/experimental/Security/CWE/CWE-470/LoadClassNoSignatureCheck.qhelp"

[masterofnow]

All done!

[atorralba]

🎉

[masterofnow]

Thanks @atorralba!
Is there anything else I need to do here from this point onwards?
What about on the github/securitylab#800 side?

[atorralba]

The evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

So after query review, the SecLab takes over again, and they'll make the final decision about your bounty. Can't give you a deadline because of the holidays, but at least there's nothing else you need to do, other than waiting for a bit :)