Skip to content

Commit a301cd6

Browse files
jeongsoolee09jeongsoolee09
authored
Merge pull request #78 from advanced-security/jeongsoolee09/non-webapp-specific-vulns
Create query for vulnerability not specific to webapp security
2 parents 083bc32 + 7b19d39 commit a301cd6

File tree

76 files changed

+1180
-83
lines changed

Some content is hidden

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

76 files changed

+1180
-83
lines changed

‎javascript/frameworks/ui5/ext/ui5.model.yml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,13 @@ extensions:
5757
- ["UI5Control", "sap/ui/richtexteditor/RichTextEditor", ""]
5858
- ["UI5CodeEditor", "UI5CodeEditor", "Instance"]
5959
- ["UI5CodeEditor", "sap/ui/codeeditor/CodeEditor", ""]
60+
# Classes that provide Path injection APIs
61+
- ["UI5ClientStorage", "sap/ui/util/Storage", ""]
62+
- ["UI5ClientStorage", "global", "Member[sap].Member[ui].Member[util].Member[Storage]"]
63+
- ["UI5ClientStorage", "global", "Member[jQuery].Member[sap].Member[storage]"]
64+
- ["UI5ClientStorage", "sap/ui/core/util/File", ""]
65+
- ["UI5ClientStorage", "global", "Member[sap].Member[ui].Member[core].Member[util].Member[File]"]
66+
6067

6168
- addsTo:
6269
pack: codeql/javascript-all
@@ -94,6 +101,8 @@ extensions:
94101
- ["ResourceBundle", "Member[create].Argument[0]", "ui5-path-injection"]
95102
- ["Properties", "Member[create].Argument[0]", "ui5-path-injection"]
96103
- ["LoaderExtensions", "Member[registerResourcePath].Argument[1]", "ui5-path-injection"]
104+
- ["UI5ClientStorage", "Member[put].Argument[0]", "ui5-path-injection"]
105+
- ["UI5ClientStorage", "Member[save].Argument[1]", "ui5-path-injection"]
97106

98107
- addsTo:
99108
pack: codeql/javascript-all

‎javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5FormulaInjectionQuery.qll

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
import javascript
2+
import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow
3+
4+
/**
5+
* Call to [`sap.ui.util.Storage.put`](https://sapui5.hana.ondemand.com/sdk/#/api/module:sap/ui/util/Storage%23methods/put)
6+
* or its jQuery counterpart, [`jQuery.sap.Storage.put`](https://sapui5.hana.ondemand.com/sdk/#/api/jQuery.sap.storage%23methods/jQuery.sap.storage.put).
7+
*/
8+
private class StoragePutCall extends CallNode {
9+
StoragePutCall() {
10+
/* 1. This is a call to `sap.ui.util.Storage.put` */
11+
// 1-1. Required from `sap/ui/util/Storage`
12+
exists(RequiredObject storageClass |
13+
this.getReceiver().getALocalSource() = storageClass and
14+
this.getCalleeName() = "put"
15+
)
16+
or
17+
// 1-2. Direct call to `sap.ui.util.Storage.put`
18+
this =
19+
globalVarRef("sap")
20+
.getAPropertyRead("ui")
21+
.getAPropertyRead("util")
22+
.getAPropertyRead("Storage")
23+
.getAMemberCall("put")
24+
or
25+
/* 2. This is a call to `jQuery.sap.storage.put` */
26+
this =
27+
globalVarRef("jQuery")
28+
.getAPropertyRead("sap")
29+
.getAPropertyRead("storage")
30+
.getAMemberCall("put")
31+
}
32+
33+
string getKeyName() {
34+
result = this.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue()
35+
}
36+
37+
string getContentToBeSaved() {
38+
result = this.getArgument(1).getALocalSource().asExpr().(StringLiteral).getValue()
39+
}
40+
}
41+
42+
/**
43+
* Call to [`sap.ui.core.util.File.save`](https://sapui5.hana.ondemand.com/sdk/#/api/sap.ui.core.util.File%23methods/sap.ui.core.util.File.save).
44+
*/
45+
private class FileSaveCall extends CallNode {
46+
FileSaveCall() {
47+
/* 1. Required from `sap/ui/core/util/File` */
48+
exists(RequiredObject fileClass |
49+
this.getReceiver().getALocalSource() = fileClass and
50+
this.getCalleeName() = "save"
51+
)
52+
or
53+
/* 2. Direct call to `sap.ui.core.util.File.save` */
54+
this =
55+
globalVarRef("sap")
56+
.getAPropertyRead("ui")
57+
.getAPropertyRead("core")
58+
.getAPropertyRead("util")
59+
.getAPropertyRead("File")
60+
.getAMemberCall("save")
61+
}
62+
63+
/**
64+
* Gets the MIME type the file will saved under.
65+
*/
66+
string getMimeType() {
67+
result = this.getArgument(3).getALocalSource().asExpr().(StringLiteral).getValue()
68+
}
69+
70+
/**
71+
* Gets the file extension to be attached to the filename.
72+
*/
73+
string getExtension() {
74+
result = this.getArgument(2).getALocalSource().asExpr().(StringLiteral).getValue()
75+
}
76+
77+
/**
78+
* Holds if the file MIME type is `"text/csv"`.
79+
*/
80+
predicate mimeTypeIsCsv() { this.getMimeType() = "text/csv" }
81+
82+
/**
83+
* Holds if the file MIME type is `"application/json"`.
84+
*/
85+
predicate mimeTypeIsJson() { this.getMimeType() = "application/json" }
86+
87+
/**
88+
* Holds if the file extension is `"csv"`. It can be used as a fallback
89+
* to detect a CSV data being written if `this.mimeTypeIsCsv()` fails.
90+
*/
91+
predicate extensionIsCsv() { this.getExtension() = "csv" }
92+
93+
/**
94+
* Holds if the file extension is `"json"`. It can be used as a fallback
95+
* to detect a JSON data being written if `this.mimeTypeIsJson()` fails.
96+
*/
97+
predicate extensionIsJson() { this.getExtension() = "json" }
98+
99+
/**
100+
* Gets the content object to be saved into the file.
101+
*/
102+
DataFlow::Node getContentToBeSaved() { result = this.getArgument(0) }
103+
104+
/**
105+
* Gets the path the file will be saved under.
106+
*/
107+
string getPathToBeSavedUnder() {
108+
result = this.getArgument(1).getALocalSource().asExpr().(StringLiteral).getValue()
109+
}
110+
}
111+
112+
class UI5FormulaInjectionConfiguration extends TaintTracking::Configuration {
113+
UI5FormulaInjectionConfiguration() { this = "UI5 Formula Injection" }
114+
115+
override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
116+
117+
override predicate isSink(DataFlow::Node node) {
118+
exists(StoragePutCall storagePutCall | node = storagePutCall.getArgument(1))
119+
or
120+
exists(FileSaveCall fileSaveCall |
121+
node = fileSaveCall.getArgument(0) and
122+
(
123+
/* 1. Primary check: match on the MIME type */
124+
fileSaveCall.mimeTypeIsCsv() or
125+
fileSaveCall.mimeTypeIsJson() or
126+
/* 2. Fallback check: match on the file extension */
127+
fileSaveCall.extensionIsCsv() or
128+
fileSaveCall.extensionIsJson()
129+
)
130+
)
131+
}
132+
}

‎javascript/frameworks/ui5/src/UI5FormulaInjection/UI5FormulaInjection.md

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
# Formula injection
2+
3+
UI5 applications that save local data, fetched from an uncontrolled remote source, into a CSV file format using generic APIs such as [`sap.ui.core.util.File.save`](https://sapui5.hana.ondemand.com/sdk/#/api/sap.ui.core.util.File%23methods/sap.ui.core.util.File.save) are vulnerable to formula injection, or CSV injection.
4+
5+
## Recommendation
6+
7+
### Escape the leading special characters
8+
9+
CSV cells containing a leading special characters such as the equal sign (`=`) may be interpreted as spreadsheet formulas. Therefore, these prefixes should be escaped with surrounding single quotes in order to have them interpreted as literal strings.
10+
11+
### Use a dedicated API function
12+
13+
As in other injection attacks, it is not recommended to use string concatenation to assemble data passed into a generic file-saving function, even if sanitizers are to be used. Instead, a dedicated library function should be used. For example, if the target being exported is a [sap.m.Table](https://sapui5.hana.ondemand.com/sdk/#/api/sap.m.Table) and the resulting file is intended to be opened using a spreadsheet program, then using one of the API functions provided by [`sap.ui.export.Spreadsheet`](https://sapui5.hana.ondemand.com/#/entity/sap.ui.export.Spreadsheet) is the preferred method of achieving the same exporting functionality.
14+
15+
## Example
16+
17+
The following controller is exporting a CSV file obtained from an event parameter by surrounding it in a pair of semicolons (`;`) as CSV separators.
18+
19+
``` javascript
20+
sap.ui.define([
21+
"sap/ui/core/Controller",
22+
"sap/ui/core/util/File"
23+
], function(Controller, File) {
24+
return Controller.extend("vulnerable.controller.app", {
25+
onSomeEvent: function(oEvent) {
26+
let response = oEvent.getProperty("someProperty").someField;
27+
let csvRow = ";" + response + ";";
28+
File.save(csvRow, "someFile", "csv", "text/csv", "utf-8");
29+
}
30+
});
31+
});
32+
```
33+
34+
## References
35+
36+
- OWASP: [CSV Injection](https://owasp.org/www-community/attacks/CSV_Injection)
37+
- [CWE-1236](https://cwe.mitre.org/data/definitions/1236.html): Improper Neutralization of Formula Elements in a CSV File
38+
- API Documentation: [`sap.ui.export.Spreadsheet`](https://sapui5.hana.ondemand.com/#/entity/sap.ui.export.Spreadsheet)
39+
- API Documentation: [`sap.ui.core.util.File.save`](https://sapui5.hana.ondemand.com/sdk/#/api/sap.ui.core.util.File%23methods/sap.ui.core.util.File.save)

‎javascript/frameworks/ui5/src/UI5FormulaInjection/UI5FormulaInjection.ql

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
/**
2+
* @name UI5 Formula Injection
3+
* @description Saving data from an uncontrolled remote source using filesystem or local storage
4+
* leads to disclosure of sensitive information or forgery of entry.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 7.8
8+
* @precision medium
9+
* @id js/ui5-formula-injection
10+
* @tags security
11+
* external/cwe/cwe-1236
12+
*/
13+
14+
import javascript
15+
import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow
16+
import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow::UI5PathGraph
17+
import advanced_security.javascript.frameworks.ui5.UI5FormulaInjectionQuery
18+
19+
from
20+
UI5FormulaInjectionConfiguration config, UI5PathNode source, UI5PathNode sink,
21+
UI5PathNode primarySource
22+
where
23+
config.hasFlowPath(source.getPathNode(), sink.getPathNode()) and
24+
primarySource = source.getAPrimarySource()
25+
select sink, primarySource, sink, "The content of a saved file depends on a $@.", primarySource,
26+
"user-provided value"

‎javascript/frameworks/ui5/src/UI5PathInjection/UI5PathInjection.md

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
# Client-side path injection
2+
3+
UI5 applications that access files using a dynamically configured path are vulnerable to injection attacks that allow an attacker to manipulate the file location.
4+
5+
## Recommendation
6+
7+
### Make path argument independent of the user input
8+
9+
If possible, do not parameterize the path on a user input. Either hardcode the path string in the source, or use only strings that are created within the application.
10+
11+
### Keep an allow-list of safe paths
12+
13+
Keep a strict allow-list of safe paths to load from or send requests to. Before loading a script from a location outside the application or making an API request to a location, check if the path is contained in the list of safe paths. Also, make sure that the allow-list is kept up to date.
14+
15+
### Check the script into the repository or use package managers
16+
17+
Since the URL of the script may be pointing to a web server vulnerable to being hijacked, it may be a good idea to check a stable version of the script into the repository to increase the degree of control. If not possible, use a trusted package manager such as `npm`.
18+
19+
## Example
20+
21+
### Including scripts from an untrusted domain
22+
23+
``` javascript
24+
sap.ui.require([
25+
"sap/ui/dom/includeScript"
26+
],
27+
function(includeScript) {
28+
includeScript("http://some.vulnerable.domain/some-script.js");
29+
}
30+
);
31+
```
32+
33+
If the vulnerable domain is outside the organization and controlled by an untrusted third party, this may result in arbitrary code execution in the user's browser.
34+
35+
### Using user input as a name of a file to be saved
36+
37+
Suppose a controller is configured to receive a response from a server as follows.
38+
39+
``` javascript
40+
sap.ui.define([
41+
"sap/ui/core/mvc/Controller",
42+
"sap/ui/core/util/File"
43+
],
44+
function(Controller, File) {
45+
return Controller.extend("vulnerable.controller.app", {
46+
onInit: function() {
47+
let oDataV2Model = this.getOwnerComponent().getModel("some-ODatav2-model");
48+
this.getView().setModel(oDataV2Model);
49+
},
50+
51+
onSomeEvent: function() {
52+
let remoteResponse = this.getView().getModel().getProperty("someProperty");
53+
File.save("some-content", remoteResponse, "txt", "text/plain", "utf-8");
54+
}
55+
});
56+
});
57+
```
58+
59+
Even if the server which updates the OData V2 model is in a trusted domain such as within the organization, the server may still contain tainted information if the UI5 application in question is vulnerable to other security attacks, say XSS. This may allow an attacker to save a file in the victim's local filesystem.
60+
61+
## References
62+
63+
- [CWE-829](https://cwe.mitre.org/data/definitions/829.html): Inclusion of Functionality from Untrusted Control Sphere
64+
- [CWE-073](https://cwe.mitre.org/data/definitions/73.html): External Control of File Name or Path
65+
- [API Documentation of `sap.ui.core.util.File`](https://sapui5.hana.ondemand.com/sdk/#/api/sap.ui.core.util.File%23methods/sap.ui.core.util.File.save)
66+
- [API Documentation of `sap.ui.dom.includeScript`](https://sapui5.hana.ondemand.com/sdk/#/api/module:sap/ui/dom/includeScript) and [`sap.ui.dom.includeStyleSheet`](https://sapui5.hana.ondemand.com/sdk/#/api/module:sap/ui/dom/includeStylesheet)
67+
- [API Documentation of `jQuery.sap.includeScript`](https://sapui5.hana.ondemand.com/sdk/#/api/module:sap/ui/dom/includeScript) and [`jQuery.sap.includeStyleSheet`](https://sapui5.hana.ondemand.com/sdk/#/api/module:sap/ui/dom/includeScript)

‎javascript/frameworks/ui5/src/UI5PathInjection/UI5PathInjection.ql

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/**
2+
* @name UI5 Path Injection
3+
* @description Constructing path from an uncontrolled remote source to be passed
4+
* to a filesystem API allows for manipulation of the local filesystem.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 7.8
8+
* @precision medium
9+
* @id js/ui5-path-injection
10+
* @tags security
11+
* external/cwe/cwe-022
12+
* external/cwe/cwe-035
13+
*/
14+
15+
import javascript
16+
import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow
17+
import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow::UI5PathGraph
18+
19+
// import semmle.javascript.security.dataflow.TaintedPathQuery as TaintedPathQuery
20+
class UI5PathInjectionConfiguration extends TaintTracking::Configuration {
21+
UI5PathInjectionConfiguration() { this = "UI5 Path Injection" }
22+
23+
override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
24+
25+
override predicate isSink(DataFlow::Node node) {
26+
node = ModelOutput::getASinkNode("ui5-path-injection").asSink()
27+
}
28+
}
29+
30+
from
31+
UI5PathInjectionConfiguration config, UI5PathNode source, UI5PathNode sink,
32+
UI5PathNode primarySource
33+
where
34+
config.hasFlowPath(source.getPathNode(), sink.getPathNode()) and
35+
primarySource = source.getAPrimarySource()
36+
select sink, primarySource, sink, "The path of a saved file depends on a $@.", primarySource,
37+
"user-provided value"

‎javascript/frameworks/ui5/test/models/sink/formulaSinkTest.expected

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
| sink.js:118:36:118:40 | code1 | code1 |
2+
| sink.js:119:48:119:52 | code1 | code1 |
3+
| sink.js:120:47:120:51 | code1 | code1 |
4+
| sink.js:122:27:122:31 | code0 | code0 |
5+
| sink.js:123:27:123:31 | code0 | code0 |
6+
| sink.js:125:44:125:48 | code0 | code0 |
7+
| sink.js:126:44:126:48 | code0 | code0 |

‎javascript/frameworks/ui5/test/models/sink/formulaSinkTest.ql

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
/**
2+
* @id formula-injection-sinks
3+
* @name Formula injection sinks
4+
* @kind problem
5+
* @problem.severity error
6+
*/
7+
8+
import javascript
9+
import advanced_security.javascript.frameworks.ui5.UI5FormulaInjectionQuery
10+
import advanced_security.javascript.frameworks.ui5.dataflow.DataFlow as UI5DataFlow
11+
12+
from UI5FormulaInjectionConfiguration config, DataFlow::Node sink
13+
where config.isSink(sink)
14+
select sink, sink.toString()

0 commit comments

Comments
 (0)