-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Java: Add query for leaking sensitive data through a ResultReceiver #11713
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
joefarebrother
merged 12 commits into
github:main
from
joefarebrother:sensitive-result-receiver
Feb 1, 2023
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
b96edb9
Add Sensitive Result Receiver query
joefarebrother de565f9
Add test and fix a bug
joefarebrother 8449dab
Add qldoc
joefarebrother 7e7b5b4
Improve test case
joefarebrother a887592
Fix typo in qldoc
joefarebrother f52db7f
Add qhelp
joefarebrother 639c42c
Fix qhelp errors and ql-for-ql errors
joefarebrother b565f99
Improve qhelp
joefarebrother e12febf
Add change note
joefarebrother cca6a13
Update java/ql/src/Security/CWE/CWE-927/SensitiveResultReceiver.qhelp
atorralba 74dba95
Apply suggestions from docs review
joefarebrother 834fc51
Update java/ql/src/Security/CWE/CWE-927/SensitiveResultReceiver.ql
atorralba File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
62 changes: 62 additions & 0 deletions
62
java/ql/lib/semmle/code/java/security/SensitiveResultReceiverQuery.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
/** Definitions for the sensitive result receiver query. */ | ||
|
||
import java | ||
import semmle.code.java.dataflow.TaintTracking2 | ||
import semmle.code.java.dataflow.FlowSources | ||
import semmle.code.java.security.SensitiveActions | ||
|
||
private class ResultReceiverSendCall extends MethodAccess { | ||
ResultReceiverSendCall() { | ||
this.getMethod() | ||
.getASourceOverriddenMethod*() | ||
.hasQualifiedName("android.os", "ResultReceiver", "send") | ||
} | ||
|
||
Expr getReceiver() { result = this.getQualifier() } | ||
|
||
Expr getSentData() { result = this.getArgument(1) } | ||
} | ||
|
||
private class UntrustedResultReceiverConf extends TaintTracking2::Configuration { | ||
UntrustedResultReceiverConf() { this = "UntrustedResultReceiverConf" } | ||
|
||
override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource } | ||
|
||
override predicate isSink(DataFlow::Node node) { | ||
node.asExpr() = any(ResultReceiverSendCall c).getReceiver() | ||
} | ||
} | ||
|
||
private predicate untrustedResultReceiverSend(DataFlow::Node src, ResultReceiverSendCall call) { | ||
any(UntrustedResultReceiverConf c).hasFlow(src, DataFlow::exprNode(call.getReceiver())) | ||
} | ||
|
||
private class SensitiveResultReceiverConf extends TaintTracking::Configuration { | ||
SensitiveResultReceiverConf() { this = "SensitiveResultReceiverConf" } | ||
|
||
override predicate isSource(DataFlow::Node node) { node.asExpr() instanceof SensitiveExpr } | ||
|
||
override predicate isSink(DataFlow::Node node) { | ||
exists(ResultReceiverSendCall call | | ||
untrustedResultReceiverSend(_, call) and | ||
node.asExpr() = call.getSentData() | ||
) | ||
} | ||
|
||
override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) { | ||
super.allowImplicitRead(node, c) | ||
or | ||
this.isSink(node) | ||
Comment on lines
+47
to
+49
Check warningCode scanning / CodeQL Var only used in one side of disjunct.
The [variable c](1) is only used in one side of disjunct.
|
||
} | ||
} | ||
|
||
/** Holds if there is a path from sensitive data at `src` to a result receiver at `sink`, and the receiver was obtained from an untrusted source `recSrc`. */ | ||
predicate sensitiveResultReceiver( | ||
DataFlow::PathNode src, DataFlow::PathNode sink, DataFlow::Node recSrc | ||
) { | ||
exists(ResultReceiverSendCall call, SensitiveResultReceiverConf conf | | ||
conf.hasFlowPath(src, sink) and | ||
sink.getNode().asExpr() = call.getSentData() and | ||
untrustedResultReceiverSend(recSrc, call) | ||
) | ||
} |
8 changes: 8 additions & 0 deletions
8
java/ql/src/Security/CWE/CWE-927/SensitiveResultReceiver.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
// BAD: Sensitive data is sent to an untrusted result receiver | ||
void bad(String password) { | ||
Intent intent = getIntent(); | ||
ResultReceiver rec = intent.getParcelableExtra("Receiver"); | ||
Bundle b = new Bundle(); | ||
b.putCharSequence("pass", password); | ||
rec.send(0, b); | ||
} |
22 changes: 22 additions & 0 deletions
22
java/ql/src/Security/CWE/CWE-927/SensitiveResultReceiver.qhelp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd"> | ||
<qhelp> | ||
|
||
<overview> | ||
<p>If a <code>ResultReceiver</code> is obtained from an untrusted source, such as an <code>Intent</code> received by an exported component, | ||
do not send it sensitive data. Otherwise, the information may be leaked to a malicious application.</p> | ||
</overview> | ||
|
||
<recommendation> | ||
<p> | ||
Do not send sensitive data to an untrusted <code>ResultReceiver</code>. | ||
</p> | ||
</recommendation> | ||
|
||
<example> | ||
<p>In the following (bad) example, sensitive data is sent to an untrusted <code>ResultReceiver</code>. </p> | ||
<sample src="SensitiveResultReceiver.java" /> | ||
</example> | ||
|
||
<references> | ||
</references> | ||
</qhelp> |
21 changes: 21 additions & 0 deletions
21
java/ql/src/Security/CWE/CWE-927/SensitiveResultReceiver.ql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/** | ||
* @name Leaking sensitive information through a ResultReceiver | ||
* @description Sending sensitive data to a 'ResultReceiver' obtained from an untrusted source | ||
* can allow malicious actors access to your information. | ||
* @kind path-problem | ||
* @problem.severity error | ||
* @security-severity 8.2 | ||
* @precision medium | ||
* @id java/android/sensitive-result-receiver | ||
* @tags security | ||
* external/cwe/cwe-927 | ||
*/ | ||
|
||
import java | ||
import semmle.code.java.security.SensitiveResultReceiverQuery | ||
import DataFlow::PathGraph | ||
|
||
from DataFlow::PathNode src, DataFlow::PathNode sink, DataFlow::Node recSrc | ||
where sensitiveResultReceiver(src, sink, recSrc) | ||
select sink, src, sink, "This $@ is sent to a ResultReceiver obtained from $@.", src, | ||
"sensitive information", recSrc, "this untrusted source" |
4 changes: 4 additions & 0 deletions
4
java/ql/src/change-notes/2023-01-12-sensitive-result-receiver.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
--- | ||
category: newQuery | ||
--- | ||
* Added a new query, `java/android/sensitive-result-receiver`, to find instances of sensitive data being leaked to an untrusted `ResultReceiver`. |
Empty file.
15 changes: 15 additions & 0 deletions
15
java/ql/test/query-tests/security/CWE-927/SensitiveResultReceiver.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import android.os.Bundle; | ||
import android.os.ResultReceiver; | ||
import android.content.Intent; | ||
|
||
class SensitiveResultReceiver { | ||
<T> T source() { return null; } | ||
|
||
void test1(String password) { | ||
Intent intent = source(); | ||
ResultReceiver rec = intent.getParcelableExtra("hi"); | ||
Bundle b = new Bundle(); | ||
b.putCharSequence("pass", password); | ||
rec.send(0, b); // $hasSensitiveResultReceiver | ||
} | ||
} |
25 changes: 25 additions & 0 deletions
25
java/ql/test/query-tests/security/CWE-927/SensitiveResultReceiver.ql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import java | ||
import TestUtilities.InlineExpectationsTest | ||
import semmle.code.java.security.SensitiveResultReceiverQuery | ||
|
||
class TestSource extends RemoteFlowSource { | ||
TestSource() { this.asExpr().(MethodAccess).getMethod().hasName("source") } | ||
|
||
override string getSourceType() { result = "test" } | ||
} | ||
|
||
class ResultReceiverTest extends InlineExpectationsTest { | ||
ResultReceiverTest() { this = "ResultReceiverTest" } | ||
|
||
override string getARelevantTag() { result = "hasSensitiveResultReceiver" } | ||
|
||
override predicate hasActualResult(Location loc, string element, string tag, string value) { | ||
exists(DataFlow::PathNode sink | | ||
sensitiveResultReceiver(_, sink, _) and | ||
element = sink.toString() and | ||
loc = sink.getNode().getLocation() and | ||
tag = "hasSensitiveResultReceiver" and | ||
value = "" | ||
) | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.