Skip to content

Commit 0e5f89a

Browse files
aschackmullaschackmull
authored
Merge pull request github#6463 from smowton/smowton/admin/gson-unsafe-deserialization
Java: add Gson support to unsafe-deserialization query
2 parents 6853f49 + 83c6406 commit 0e5f89a

File tree

152 files changed

+7581
-7238
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

152 files changed

+7581
-7238
lines changed

‎java/change-notes/2021-08-10-gson-unsafe-deserialization.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* The "Deserialization of user-controlled data" (`java/unsafe-deserialization`) query now recognizes deserialization using the `Gson` library.

‎java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,9 @@ private import FlowSummary
7777
*/
7878
private module Frameworks {
7979
private import internal.ContainerFlow
80-
private import semmle.code.java.frameworks.android.XssSinks
80+
private import semmle.code.java.frameworks.android.Android
8181
private import semmle.code.java.frameworks.android.Intent
82+
private import semmle.code.java.frameworks.android.XssSinks
8283
private import semmle.code.java.frameworks.ApacheHttp
8384
private import semmle.code.java.frameworks.apache.Collections
8485
private import semmle.code.java.frameworks.apache.Lang

‎java/ql/lib/semmle/code/java/frameworks/android/Android.qll

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,3 +202,44 @@ private class ContentProviderSourceModels extends SourceModelCsv {
202202
]
203203
}
204204
}
205+
206+
/** Interface for classes whose instances can be written to and restored from a Parcel. */
207+
class TypeParcelable extends Interface {
208+
TypeParcelable() { this.hasQualifiedName("android.os", "Parcelable") }
209+
}
210+
211+
/**
212+
* A method that overrides `android.os.Parcelable.Creator.createFromParcel`.
213+
*/
214+
class CreateFromParcelMethod extends Method {
215+
CreateFromParcelMethod() {
216+
this.hasName("createFromParcel") and
217+
this.getEnclosingCallable().getDeclaringType().getASupertype*() instanceof TypeParcelable
218+
}
219+
}
220+
221+
private class ParcelPropagationModels extends SummaryModelCsv {
222+
override predicate row(string s) {
223+
// Parcel readers that return their value
224+
s =
225+
"android.os;Parcel;false;read" +
226+
[
227+
"Array", "ArrayList", "Boolean", "Bundle", "Byte", "Double", "FileDescriptor", "Float",
228+
"HashMap", "Int", "Long", "Parcelable", "ParcelableArray", "PersistableBundle",
229+
"Serializable", "Size", "SizeF", "SparseArray", "SparseBooleanArray", "String",
230+
"StrongBinder", "TypedObject", "Value"
231+
] + ";;;Argument[-1];ReturnValue;taint"
232+
or
233+
// Parcel readers that write to an existing object
234+
s =
235+
"android.os;Parcel;false;read" +
236+
[
237+
"BinderArray", "BinderList", "BooleanArray", "ByteArray", "CharArray", "DoubleArray",
238+
"FloatArray", "IntArray", "List", "LongArray", "Map", "ParcelableList", "StringArray",
239+
"StringList", "TypedArray", "TypedList"
240+
] + ";;;Argument[-1];Argument[0];taint"
241+
or
242+
// One Parcel method that aliases an argument to a return value
243+
s = "android.os;Parcel;false;readParcelableList;;;Argument[0];ReturnValue;value"
244+
}
245+
}

‎java/ql/lib/semmle/code/java/frameworks/android/Intent.qll

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,32 +3,53 @@ private import semmle.code.java.dataflow.DataFlow
33
import semmle.code.java.dataflow.FlowSteps
44
import semmle.code.java.dataflow.ExternalFlow
55

6+
/**
7+
* The class `android.content.Intent`.
8+
*/
69
class TypeIntent extends Class {
710
TypeIntent() { hasQualifiedName("android.content", "Intent") }
811
}
912

13+
/**
14+
* The class `android.app.Activity`.
15+
*/
1016
class TypeActivity extends Class {
1117
TypeActivity() { hasQualifiedName("android.app", "Activity") }
1218
}
1319

20+
/**
21+
* The class `android.content.Context`.
22+
*/
1423
class TypeContext extends RefType {
1524
TypeContext() { hasQualifiedName("android.content", "Context") }
1625
}
1726

27+
/**
28+
* The class `android.content.BroadcastReceiver`.
29+
*/
1830
class TypeBroadcastReceiver extends Class {
1931
TypeBroadcastReceiver() { hasQualifiedName("android.content", "BroadcastReceiver") }
2032
}
2133

34+
/**
35+
* The method `Activity.getIntent`
36+
*/
2237
class AndroidGetIntentMethod extends Method {
2338
AndroidGetIntentMethod() { hasName("getIntent") and getDeclaringType() instanceof TypeActivity }
2439
}
2540

41+
/**
42+
* The method `BroadcastReceiver.onReceive`.
43+
*/
2644
class AndroidReceiveIntentMethod extends Method {
2745
AndroidReceiveIntentMethod() {
2846
hasName("onReceive") and getDeclaringType() instanceof TypeBroadcastReceiver
2947
}
3048
}
3149

50+
/**
51+
* The method `Context.startActivity` or `startActivities`.
52+
*/
3253
class ContextStartActivityMethod extends Method {
3354
ContextStartActivityMethod() {
3455
(hasName("startActivity") or hasName("startActivities")) and
@@ -44,6 +65,16 @@ private class IntentFieldsInheritTaint extends DataFlow::SyntheticFieldContent,
4465
IntentFieldsInheritTaint() { this.getField().matches("android.content.Intent.%") }
4566
}
4667

68+
/**
69+
* The method `Intent.getParcelableExtra`.
70+
*/
71+
class IntentGetParcelableExtraMethod extends Method {
72+
IntentGetParcelableExtraMethod() {
73+
hasName("getParcelableExtra") and
74+
getDeclaringType() instanceof TypeIntent
75+
}
76+
}
77+
4778
private class IntentBundleFlowSteps extends SummaryModelCsv {
4879
override predicate row(string row) {
4980
row =

‎java/ql/lib/semmle/code/java/frameworks/google/Gson.qll

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
/**
2+
* Provides classes for working with the Gson framework.
3+
*/
4+
5+
import java
6+
import semmle.code.java.dataflow.DataFlow
7+
import semmle.code.java.frameworks.android.Android
8+
import semmle.code.java.frameworks.android.Intent
9+
10+
/** The class `com.google.gson.Gson`. */
11+
class Gson extends RefType {
12+
Gson() { this.hasQualifiedName("com.google.gson", "Gson") }
13+
}
14+
15+
/** The `fromJson` deserialization method. */
16+
class GsonDeserializeMethod extends Method {
17+
GsonDeserializeMethod() {
18+
this.getDeclaringType() instanceof Gson and
19+
this.hasName("fromJson")
20+
}
21+
}
22+
23+
/**
24+
* Holds if `intentNode` is an `Intent` used in the context `(T)intentNode.getParcelableExtra(...)` and
25+
* `parcelNode` is the corresponding parameter of `Parcelable.Creator<T> { public T createFromParcel(Parcel parcelNode) { }`,
26+
* where `T` is a concrete type implementing `Parcelable`.
27+
*/
28+
predicate intentFlowsToParcel(DataFlow::Node intentNode, DataFlow::Node parcelNode) {
29+
exists(MethodAccess getParcelableExtraCall, CreateFromParcelMethod cfpm, Type createdType |
30+
intentNode.asExpr() = getParcelableExtraCall.getQualifier() and
31+
getParcelableExtraCall.getMethod() instanceof IntentGetParcelableExtraMethod and
32+
DataFlow::localExprFlow(getParcelableExtraCall, any(Expr e | e.getType() = createdType)) and
33+
parcelNode.asParameter() = cfpm.getParameter(0) and
34+
cfpm.getReturnType() = createdType
35+
)
36+
}

‎java/ql/lib/semmle/code/java/security/UnsafeDeserializationQuery.qll

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ private import semmle.code.java.frameworks.Jackson
1717
private import semmle.code.java.frameworks.Jabsorb
1818
private import semmle.code.java.frameworks.JoddJson
1919
private import semmle.code.java.frameworks.Flexjson
20+
private import semmle.code.java.frameworks.google.Gson
2021
private import semmle.code.java.frameworks.apache.Lang
2122
private import semmle.code.java.Reflection
2223

@@ -207,6 +208,10 @@ predicate unsafeDeserialization(MethodAccess ma, Expr sink) {
207208
or
208209
m instanceof FlexjsonDeserializeMethod and
209210
sink = ma.getArgument(0)
211+
or
212+
m instanceof GsonDeserializeMethod and
213+
sink = ma.getArgument(0) and
214+
any(UnsafeTypeConfig config).hasFlowToExpr(ma.getArgument(1))
210215
)
211216
}
212217

@@ -249,6 +254,8 @@ class UnsafeDeserializationConfig extends TaintTracking::Configuration {
249254
createJacksonJsonParserStep(pred, succ)
250255
or
251256
createJacksonTreeNodeStep(pred, succ)
257+
or
258+
intentFlowsToParcel(pred, succ)
252259
}
253260

254261
override predicate isSanitizer(DataFlow::Node node) {
@@ -362,9 +369,15 @@ class UnsafeTypeConfig extends TaintTracking2::Configuration {
362369
ma.getMethod() instanceof JabsorbUnmarshallMethod
363370
or
364371
ma.getMethod() instanceof JoddJsonParseMethod
372+
or
373+
ma.getMethod() instanceof GsonDeserializeMethod
365374
) and
366375
// Note `JacksonTypeDescriptorType` includes plain old `java.lang.Class`
367-
arg.getType() instanceof JacksonTypeDescriptorType and
376+
(
377+
arg.getType() instanceof JacksonTypeDescriptorType
378+
or
379+
arg.getType().(RefType).hasQualifiedName("java.lang.reflect", "Type")
380+
) and
368381
arg = sink.asExpr()
369382
)
370383
}
@@ -375,7 +388,8 @@ class UnsafeTypeConfig extends TaintTracking2::Configuration {
375388
*/
376389
override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
377390
resolveClassStep(fromNode, toNode) or
378-
looksLikeResolveClassStep(fromNode, toNode)
391+
looksLikeResolveClassStep(fromNode, toNode) or
392+
intentFlowsToParcel(fromNode, toNode)
379393
}
380394
}
381395

‎java/ql/src/Security/CWE/CWE-502/UnsafeDeserialization.qhelp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ may have unforeseen effects, such as the execution of arbitrary code.
1515
<p>
1616
There are many different serialization frameworks. This query currently
1717
supports Kryo, XmlDecoder, XStream, SnakeYaml, JYaml, JsonIO, YAMLBeans, HessianBurlap, Castor, Burlap,
18-
Jackson, Jabsorb, Jodd JSON, Flexjson and Java IO serialization through
18+
Jackson, Jabsorb, Jodd JSON, Flexjson, Gson and Java IO serialization through
1919
<code>ObjectInputStream</code>/<code>ObjectOutputStream</code>.
2020
</p>
2121
</overview>
@@ -113,6 +113,10 @@ Jodd JSON documentation on deserialization:
113113
RCE in Flexjson:
114114
<a href="https://codewhitesec.blogspot.com/2020/03/liferay-portal-json-vulns.html">Flexjson deserialization</a>.
115115
</li>
116+
<li>
117+
Android Intent deserialization vulnerabilities with GSON parser:
118+
<a href="https://blog.oversecured.com/Exploiting-memory-corruption-vulnerabilities-on-Android/#insecure-use-of-json-parsers">Insecure use of JSON parsers</a>.
119+
</li>
116120
</references>
117121

118122
</qhelp>

‎java/ql/test/library-tests/dataflow/taintsources/remote.expected

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5,34 +5,34 @@
55
| A.java:41:5:41:53 | getInputStream(...) | A.java:41:5:41:53 | getInputStream(...) |
66
| A.java:42:5:42:45 | getInputStream(...) | A.java:42:5:42:45 | getInputStream(...) |
77
| A.java:43:5:43:47 | getHostName(...) | A.java:43:5:43:47 | getHostName(...) |
8-
| IntentSources.java:9:20:9:35 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:1564:19:1564:32 | [summary] read: <map.value> of android.content.Intent.extras of argument -1 in getStringExtra |
9-
| IntentSources.java:9:20:9:35 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:1564:19:1564:32 | [summary] read: android.content.Intent.extras of argument -1 in getStringExtra |
10-
| IntentSources.java:9:20:9:35 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:1564:19:1564:32 | [summary] to write: return (return) in getStringExtra |
11-
| IntentSources.java:9:20:9:35 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:1564:19:1564:32 | parameter this |
8+
| IntentSources.java:9:20:9:35 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:105:19:105:32 | [summary] read: <map.value> of android.content.Intent.extras of argument -1 in getStringExtra |
9+
| IntentSources.java:9:20:9:35 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:105:19:105:32 | [summary] read: android.content.Intent.extras of argument -1 in getStringExtra |
10+
| IntentSources.java:9:20:9:35 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:105:19:105:32 | [summary] to write: return (return) in getStringExtra |
11+
| IntentSources.java:9:20:9:35 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:105:19:105:32 | parameter this |
1212
| IntentSources.java:9:20:9:35 | getIntent(...) | IntentSources.java:9:20:9:35 | getIntent(...) |
1313
| IntentSources.java:9:20:9:35 | getIntent(...) | IntentSources.java:9:20:9:57 | getStringExtra(...) |
1414
| IntentSources.java:9:20:9:35 | getIntent(...) | IntentSources.java:10:29:10:35 | trouble |
15-
| IntentSources.java:16:20:16:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:1564:19:1564:32 | [summary] read: <map.value> of android.content.Intent.extras of argument -1 in getStringExtra |
16-
| IntentSources.java:16:20:16:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:1564:19:1564:32 | [summary] read: android.content.Intent.extras of argument -1 in getStringExtra |
17-
| IntentSources.java:16:20:16:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:1564:19:1564:32 | [summary] to write: return (return) in getStringExtra |
18-
| IntentSources.java:16:20:16:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:1564:19:1564:32 | parameter this |
15+
| IntentSources.java:16:20:16:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:105:19:105:32 | [summary] read: <map.value> of android.content.Intent.extras of argument -1 in getStringExtra |
16+
| IntentSources.java:16:20:16:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:105:19:105:32 | [summary] read: android.content.Intent.extras of argument -1 in getStringExtra |
17+
| IntentSources.java:16:20:16:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:105:19:105:32 | [summary] to write: return (return) in getStringExtra |
18+
| IntentSources.java:16:20:16:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:105:19:105:32 | parameter this |
1919
| IntentSources.java:16:20:16:30 | getIntent(...) | IntentSources.java:16:20:16:30 | getIntent(...) |
2020
| IntentSources.java:16:20:16:30 | getIntent(...) | IntentSources.java:16:20:16:52 | getStringExtra(...) |
2121
| IntentSources.java:16:20:16:30 | getIntent(...) | IntentSources.java:17:29:17:35 | trouble |
22-
| IntentSources.java:23:20:23:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:1863:19:1863:27 | [summary] read: android.content.Intent.extras of argument -1 in getExtras |
23-
| IntentSources.java:23:20:23:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:1863:19:1863:27 | [summary] to write: return (return) in getExtras |
24-
| IntentSources.java:23:20:23:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:1863:19:1863:27 | parameter this |
25-
| IntentSources.java:23:20:23:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/os/BaseBundle.java:600:19:600:27 | [summary] read: <map.value> of argument -1 in getString |
26-
| IntentSources.java:23:20:23:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/os/BaseBundle.java:600:19:600:27 | [summary] to write: return (return) in getString |
27-
| IntentSources.java:23:20:23:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/os/BaseBundle.java:600:19:600:27 | parameter this |
22+
| IntentSources.java:23:20:23:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:33:19:33:27 | [summary] read: android.content.Intent.extras of argument -1 in getExtras |
23+
| IntentSources.java:23:20:23:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:33:19:33:27 | [summary] to write: return (return) in getExtras |
24+
| IntentSources.java:23:20:23:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:33:19:33:27 | parameter this |
25+
| IntentSources.java:23:20:23:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/os/BaseBundle.java:12:19:12:27 | [summary] read: <map.value> of argument -1 in getString |
26+
| IntentSources.java:23:20:23:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/os/BaseBundle.java:12:19:12:27 | [summary] to write: return (return) in getString |
27+
| IntentSources.java:23:20:23:30 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/os/BaseBundle.java:12:19:12:27 | parameter this |
2828
| IntentSources.java:23:20:23:30 | getIntent(...) | IntentSources.java:23:20:23:30 | getIntent(...) |
2929
| IntentSources.java:23:20:23:30 | getIntent(...) | IntentSources.java:23:20:23:42 | getExtras(...) |
3030
| IntentSources.java:23:20:23:30 | getIntent(...) | IntentSources.java:23:20:23:59 | getString(...) |
3131
| IntentSources.java:23:20:23:30 | getIntent(...) | IntentSources.java:24:29:24:35 | trouble |
32-
| IntentSources.java:33:20:33:33 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:1564:19:1564:32 | [summary] read: <map.value> of android.content.Intent.extras of argument -1 in getStringExtra |
33-
| IntentSources.java:33:20:33:33 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:1564:19:1564:32 | [summary] read: android.content.Intent.extras of argument -1 in getStringExtra |
34-
| IntentSources.java:33:20:33:33 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:1564:19:1564:32 | [summary] to write: return (return) in getStringExtra |
35-
| IntentSources.java:33:20:33:33 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:1564:19:1564:32 | parameter this |
32+
| IntentSources.java:33:20:33:33 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:105:19:105:32 | [summary] read: <map.value> of android.content.Intent.extras of argument -1 in getStringExtra |
33+
| IntentSources.java:33:20:33:33 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:105:19:105:32 | [summary] read: android.content.Intent.extras of argument -1 in getStringExtra |
34+
| IntentSources.java:33:20:33:33 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:105:19:105:32 | [summary] to write: return (return) in getStringExtra |
35+
| IntentSources.java:33:20:33:33 | getIntent(...) | ../../../stubs/google-android-9.0.0/android/content/Intent.java:105:19:105:32 | parameter this |
3636
| IntentSources.java:33:20:33:33 | getIntent(...) | IntentSources.java:33:20:33:33 | getIntent(...) |
3737
| IntentSources.java:33:20:33:33 | getIntent(...) | IntentSources.java:33:20:33:55 | getStringExtra(...) |
3838
| IntentSources.java:33:20:33:33 | getIntent(...) | IntentSources.java:34:29:34:35 | trouble |

0 commit comments

Comments
 (0)