Java: CWE-266 - Query to detect Intent URI Permission Manipulation in Android applications

[atorralba]

This PR adds a query to detect Intent URI Permission Manipulation in Android applications. The query detects when an exported Activity is obtaining a user-provided Intent and redirecting it back to the calling application. Also, it requires that a non-exported Content Provider with the attribute android:grantUriPermissions="true" exists in the project.

Description

When an externally provided Intent is returned to an Activity with setResult, a malicious application could exploit this by manipulating the Intent to grant itself permissions to access arbitrary Content Providers that are accessible by the vulnerable application.

Evaluation

The query finds 3 TP when run on a set of 1k Android open source projects.

By adding the source defined in #6963, this goes up to 4 TP results (the extra result occurs in an intentionally vulnerable application).

[mchammer01]

I'll review this on behalf of Docs 🙂

[mchammer01]

@atorralba - LGTM ✨
I've made a few minor comments about capitalization of some terms, and about the query description.

1. I leave it up to you if you think we should capitalize Activity, Intent and Content Providers throughout the docs.

2. I think we should simplify the query description by following the guidelines (ideally using something of the type Syntax X causes behavior Y. and putting code elements in single quotes).

Approving this from an editorial point of view to avoid blocking you.

[atorralba]

Thanks for your review @mchammer01!

I applied your suggestion regarding the change note, and I also improved the query's description, let me know if it can be further improved.

Regarding the capitalization of certain words, Activity and Intent are class names, and because of that the consensus has been to capitalize them AFAIK.

"Content Providers", on the other hand, is not a class name (that would be ContentProvider), and in Android's official documentation the term is not capitalized, so we should probably change it.

[mchammer01]

@atorralba 👋🏻 - thanks for working on the query description, I like it ✨
Just made a minor suggestion on it.
Thanks for explaining about the capitalization of some terms. Happy for you to leave Intent and Activity as they are, and to use content providers instead of Content Providers 🙂

[smowton]

Looks good, just some minor comments

[aschackmull]

Isn't this step slightly odd to include in general?

[atorralba]

I was surprised by taint not being tracked in cases like the following:

int flag = INTERESTING_FAG || UNINTERESTING_FLAG;
sink(flag);

So that's why I added it as a general step (I thought other people would expect it to work out-of-the-box). But I admit its applicability is limited, so if you think it doesn't add that much value generally, I can make it a specific step just for the query.

[aschackmull]

It makes sense when specifically tracking flags, but I don't think it makes much sense otherwise. Global data flow on int-sized data from remote flow sources can already travel quite far, and I don't think we'll want to extend that with bitwise operations in general.

[atorralba]

Oh, now that I changed it back I remembered why I did it like this. Actually, it's the sanitizer which needs the step (because it uses TaintTracking::localExprTaint), not the TaintTracking::Configuration itself:

private class IntentFlagsOrDataChangedSanitizer extends IntentUriPermissionManipulationSanitizer {
  IntentFlagsOrDataChangedSanitizer() {
    exists(MethodAccess ma, Method m |
      // ...
      m.hasName("removeFlags") and
      TaintTracking::localExprTaint(any(GrantReadUriPermissionFlag f).getAnAccess(),
        ma.getArgument(0)) and
      TaintTracking::localExprTaint(any(GrantWriteUriPermissionFlag f).getAnAccess(),
        ma.getArgument(0))
      or
      m.hasName("setFlags") and
      not TaintTracking::localExprTaint(any(GrantUriPermissionFlag f).getAnAccess(),
        ma.getArgument(0))
      // ...
    )
  }
}

Should I write a configuration specifically for tracking flag taint?

[aschackmull]

Local taint tracking is just a simple transitive closure of a step relation (jump-to-def on localExprTaint to have a look). So I'd suggest something like:

private predicate flagTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
  TaintTracking::localTaintStep(n1, n2) or
  // insert bitwise step here
}

and then use flagTaintStep* instead of TaintTracking::localExprTaint.

[atorralba]

That's elegant! Thanks for spoon-feeding the solution, this is a trick I'm going to remember well.

Done in 265f8a3. Note that I used a slightly different name (named after what the predicate does, not after how I intend to use it). Also, since I had started adding additional taint steps to the query, I left the abstract step class because it won't hurt to have it available in the future.

[atorralba]

Force-pushed to rebase and move the change note to the new location.