Skip to content

Commit bc899b6

Browse files
luchua-bcluchua-bc
committed
Move common code to a library and add more test cases
1 parent b10552a commit bc899b6

File tree

6 files changed

+81
-74
lines changed

6 files changed

+81
-74
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-755/NFEAndroidDoS.ql‎

Lines changed: 5 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/**
22
* @name Local Android DoS Caused By NumberFormatException
33
* @id java/android/nfe-local-android-dos
4-
* @description NumberFormatException thrown but not caught by an Android application that allows external inputs can crash the application, which is a local Denial of Service (Dos) attack.
4+
* @description NumberFormatException thrown but not caught by an Android application that allows external inputs can crash the application, constituting a local Denial of Service (DoS) attack.
55
* @kind path-problem
66
* @tags security
77
* external/cwe/cwe-755
@@ -10,74 +10,9 @@
1010
import java
1111
import semmle.code.java.frameworks.android.Intent
1212
import semmle.code.java.dataflow.FlowSources
13+
import semmle.code.java.NumberFormatException
1314
import DataFlow::PathGraph
1415

15-
/** Code from java/ql/src/Violations of Best Practice/Exception Handling/NumberFormatException.ql */
16-
private class SpecialMethodAccess extends MethodAccess {
17-
predicate isValueOfMethod(string klass) {
18-
this.getMethod().getName() = "valueOf" and
19-
this.getQualifier().getType().(RefType).hasQualifiedName("java.lang", klass) and
20-
this.getAnArgument().getType().(RefType).hasQualifiedName("java.lang", "String")
21-
}
22-
23-
predicate isParseMethod(string klass, string name) {
24-
this.getMethod().getName() = name and
25-
this.getQualifier().getType().(RefType).hasQualifiedName("java.lang", klass)
26-
}
27-
28-
predicate throwsNFE() {
29-
this.isParseMethod("Byte", "parseByte") or
30-
this.isParseMethod("Short", "parseShort") or
31-
this.isParseMethod("Integer", "parseInt") or
32-
this.isParseMethod("Long", "parseLong") or
33-
this.isParseMethod("Float", "parseFloat") or
34-
this.isParseMethod("Double", "parseDouble") or
35-
this.isParseMethod("Byte", "decode") or
36-
this.isParseMethod("Short", "decode") or
37-
this.isParseMethod("Integer", "decode") or
38-
this.isParseMethod("Long", "decode") or
39-
this.isValueOfMethod("Byte") or
40-
this.isValueOfMethod("Short") or
41-
this.isValueOfMethod("Integer") or
42-
this.isValueOfMethod("Long") or
43-
this.isValueOfMethod("Float") or
44-
this.isValueOfMethod("Double")
45-
}
46-
}
47-
48-
private class SpecialClassInstanceExpr extends ClassInstanceExpr {
49-
predicate isStringConstructor(string klass) {
50-
this.getType().(RefType).hasQualifiedName("java.lang", klass) and
51-
this.getAnArgument().getType().(RefType).hasQualifiedName("java.lang", "String") and
52-
this.getNumArgument() = 1
53-
}
54-
55-
predicate throwsNFE() {
56-
this.isStringConstructor("Byte") or
57-
this.isStringConstructor("Short") or
58-
this.isStringConstructor("Integer") or
59-
this.isStringConstructor("Long") or
60-
this.isStringConstructor("Float") or
61-
this.isStringConstructor("Double")
62-
}
63-
}
64-
65-
class NumberFormatException extends RefType {
66-
NumberFormatException() { this.hasQualifiedName("java.lang", "NumberFormatException") }
67-
}
68-
69-
private predicate catchesNFE(TryStmt t) {
70-
exists(CatchClause cc, LocalVariableDeclExpr v |
71-
t.getACatchClause() = cc and
72-
cc.getVariable() = v and
73-
v.getType().(RefType).getASubtype*() instanceof NumberFormatException
74-
)
75-
}
76-
77-
private predicate throwsNFE(Expr e) {
78-
e.(SpecialClassInstanceExpr).throwsNFE() or e.(SpecialMethodAccess).throwsNFE()
79-
}
80-
8116
/**
8217
* Taint configuration tracking flow from untrusted inputs to number conversion calls in exported Android compononents.
8318
*/
@@ -90,7 +25,7 @@ class NFELocalDoSConfiguration extends TaintTracking::Configuration {
9025
/** Holds if NFE is thrown but not caught */
9126
override predicate isSink(DataFlow::Node sink) {
9227
exists(Expr e |
93-
e.getEnclosingCallable().getDeclaringType() instanceof ExportableAndroidComponent and
28+
e.getEnclosingCallable().getDeclaringType().(ExportableAndroidComponent).isExported() and
9429
throwsNFE(e) and
9530
not exists(TryStmt t |
9631
t.getBlock() = e.getEnclosingStmt().getEnclosingStmt*() and
@@ -103,5 +38,6 @@ class NFELocalDoSConfiguration extends TaintTracking::Configuration {
10338

10439
from DataFlow::PathNode source, DataFlow::PathNode sink, NFELocalDoSConfiguration conf
10540
where conf.hasFlowPath(source, sink)
106-
select sink.getNode(), source, sink, "Local Android Denial of Service due to $@.", source.getNode(),
41+
select sink.getNode(), source, sink,
42+
"Uncaught NumberFormatException in an exported Android component due to $@.", source.getNode(),
10743
"user-provided value"

‎java/ql/test/experimental/query-tests/security/CWE-755/AndroidManifest.xml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919
<category android:name="android.intent.category.LAUNCHER" />
2020
</intent-filter>
2121
</activity>
22+
23+
<activity android:name=".SafeActivity" />
2224
</application>
2325

2426
</manifest>

‎java/ql/test/experimental/query-tests/security/CWE-755/IntentUtils.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
package com.example.app;
2+
3+
import android.app.Activity;
4+
5+
import android.os.Bundle;
6+
7+
8+
/** A utility program for getting intent extra information from Android activity */
9+
public class IntentUtils {
10+
11+
/** Get double extra */
12+
public static double getDoubleExtra(Activity a, String key) {
13+
String value = a.getIntent().getStringExtra(key);
14+
return Double.parseDouble(value);
15+
}
16+
}

‎java/ql/test/experimental/query-tests/security/CWE-755/NFEAndroidDoS.expected‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@ nodes
1515
| NFEAndroidDoS.java:44:21:44:43 | new Double(...) | semmle.label | new Double(...) |
1616
| NFEAndroidDoS.java:47:21:47:47 | valueOf(...) | semmle.label | valueOf(...) |
1717
#select
18-
| NFEAndroidDoS.java:14:21:14:51 | parseDouble(...) | NFEAndroidDoS.java:13:24:13:34 | getIntent(...) : Intent | NFEAndroidDoS.java:14:21:14:51 | parseDouble(...) | Local Android Denial of Service due to $@. | NFEAndroidDoS.java:13:24:13:34 | getIntent(...) | user-provided value |
19-
| NFEAndroidDoS.java:23:15:23:40 | parseInt(...) | NFEAndroidDoS.java:22:21:22:31 | getIntent(...) : Intent | NFEAndroidDoS.java:23:15:23:40 | parseInt(...) | Local Android Denial of Service due to $@. | NFEAndroidDoS.java:22:21:22:31 | getIntent(...) | user-provided value |
20-
| NFEAndroidDoS.java:26:16:26:42 | parseInt(...) | NFEAndroidDoS.java:25:22:25:32 | getIntent(...) : Intent | NFEAndroidDoS.java:26:16:26:42 | parseInt(...) | Local Android Denial of Service due to $@. | NFEAndroidDoS.java:25:22:25:32 | getIntent(...) | user-provided value |
21-
| NFEAndroidDoS.java:44:21:44:43 | new Double(...) | NFEAndroidDoS.java:43:24:43:34 | getIntent(...) : Intent | NFEAndroidDoS.java:44:21:44:43 | new Double(...) | Local Android Denial of Service due to $@. | NFEAndroidDoS.java:43:24:43:34 | getIntent(...) | user-provided value |
22-
| NFEAndroidDoS.java:47:21:47:47 | valueOf(...) | NFEAndroidDoS.java:43:24:43:34 | getIntent(...) : Intent | NFEAndroidDoS.java:47:21:47:47 | valueOf(...) | Local Android Denial of Service due to $@. | NFEAndroidDoS.java:43:24:43:34 | getIntent(...) | user-provided value |
18+
| NFEAndroidDoS.java:14:21:14:51 | parseDouble(...) | NFEAndroidDoS.java:13:24:13:34 | getIntent(...) : Intent | NFEAndroidDoS.java:14:21:14:51 | parseDouble(...) | Uncaught NumberFormatException in an exported Android component due to $@. | NFEAndroidDoS.java:13:24:13:34 | getIntent(...) | user-provided value |
19+
| NFEAndroidDoS.java:23:15:23:40 | parseInt(...) | NFEAndroidDoS.java:22:21:22:31 | getIntent(...) : Intent | NFEAndroidDoS.java:23:15:23:40 | parseInt(...) | Uncaught NumberFormatException in an exported Android component due to $@. | NFEAndroidDoS.java:22:21:22:31 | getIntent(...) | user-provided value |
20+
| NFEAndroidDoS.java:26:16:26:42 | parseInt(...) | NFEAndroidDoS.java:25:22:25:32 | getIntent(...) : Intent | NFEAndroidDoS.java:26:16:26:42 | parseInt(...) | Uncaught NumberFormatException in an exported Android component due to $@. | NFEAndroidDoS.java:25:22:25:32 | getIntent(...) | user-provided value |
21+
| NFEAndroidDoS.java:44:21:44:43 | new Double(...) | NFEAndroidDoS.java:43:24:43:34 | getIntent(...) : Intent | NFEAndroidDoS.java:44:21:44:43 | new Double(...) | Uncaught NumberFormatException in an exported Android component due to $@. | NFEAndroidDoS.java:43:24:43:34 | getIntent(...) | user-provided value |
22+
| NFEAndroidDoS.java:47:21:47:47 | valueOf(...) | NFEAndroidDoS.java:43:24:43:34 | getIntent(...) : Intent | NFEAndroidDoS.java:47:21:47:47 | valueOf(...) | Uncaught NumberFormatException in an exported Android component due to $@. | NFEAndroidDoS.java:43:24:43:34 | getIntent(...) | user-provided value |

‎java/ql/test/experimental/query-tests/security/CWE-755/NFEAndroidDoS.java‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,4 +46,41 @@ public void testOnCreate4(Bundle savedInstanceState) {
4646
String maxPriceStr = getIntent().getStringExtra("priceMax");
4747
double maxPrice = Double.valueOf(minPriceStr);
4848
}
49+
50+
// GOOD - parse string extra to double with caught NFE
51+
public void testOnCreate5(Bundle savedInstanceState) {
52+
super.onCreate(savedInstanceState);
53+
setContentView(-1);
54+
55+
double minPrice = 0;
56+
try {
57+
String minPriceStr = getIntent().getStringExtra("priceMin");
58+
minPrice = Double.parseDouble(minPriceStr);
59+
} catch (NumberFormatException nfe) {
60+
nfe.printStackTrace();
61+
}
62+
}
63+
64+
// GOOD - parse string extra to double with caught NFE as the supertype Throwable
65+
public void testOnCreate6(Bundle savedInstanceState) {
66+
super.onCreate(savedInstanceState);
67+
setContentView(-1);
68+
69+
double minPrice = 0;
70+
try {
71+
String minPriceStr = getIntent().getStringExtra("priceMin");
72+
minPrice = Double.parseDouble(minPriceStr);
73+
} catch (Throwable te) {
74+
te.printStackTrace();
75+
}
76+
}
77+
78+
// BAD - parse string extra to double
79+
// Note this case of invoking utility method that takes an Activity a then calls `a.getIntent().getStringExtra(...)` is not yet detected thus is beyond what the query is capable of.
80+
public void testOnCreate7(Bundle savedInstanceState) {
81+
super.onCreate(savedInstanceState);
82+
setContentView(-1);
83+
84+
double priceMin = IntentUtils.getDoubleExtra(this, "priceMin");
85+
}
4986
}

‎java/ql/test/experimental/query-tests/security/CWE-755/SafeActivity.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
package com.example.app;
2+
3+
import android.app.Activity;
4+
import android.os.Bundle;
5+
6+
/** Android activity that tests app crash by NumberFormatException, which is not exported in `AndroidManifest.xml` */
7+
public class SafeActivity extends Activity {
8+
// BAD - parse string extra to double
9+
public void testOnCreate1(Bundle savedInstanceState) {
10+
super.onCreate(savedInstanceState);
11+
setContentView(-1);
12+
13+
String minPriceStr = getIntent().getStringExtra("priceMin");
14+
double minPrice = Double.parseDouble(minPriceStr);
15+
}
16+
}

0 commit comments

Comments
 (0)