Skip to content

Commit 0b78c55

Browse files
mbaludambaluda
authored
UI5 client side log-injection improvements
- Adds dataflow step so that existing queries can follow the path through log handlers - Implements 2 new queries 1. **UI5UnsafeLogAccess**: alerts for potential injection only if a custom log handler or a call to `getLogEntries` is present 2. **UI5LogsToHttp**: alerts on tainted logs sent over the network - Test cases - Help `.md` files
1 parent acf1628 commit 0b78c55

39 files changed

+416
-89
lines changed

‎.github/workflows/javascript.sarif.expected

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

‎javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
import javascript
66

7-
newtype CdlKind =
7+
private newtype CdlKind =
88
Service(string value) { value = "service" } or
99
Entity(string value) { value = "entity" } or
1010
Event(string value) { value = "event" } or

‎javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ Expr getRootReceiver(Expr e) {
8989
* An aggregation type for the two ways to access the fluent API
9090
* provided by the module cds.ql
9191
*/
92-
newtype TCqlClause =
92+
private newtype TCqlClause =
9393
MethodCall(MethodCallExpr callExpr) {
9494
exists(CqlQueryBase base | base = getRootReceiver(callExpr)) or
9595
exists(CqlQueryBaseCall call | call = getRootReceiver(callExpr))

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ extensions:
2525
- ["SapLogger", "global", "Member[jQuery].Member[sap].Member[log]"]
2626
# Logging functions as well as `getLogger` also serves as a constructor
2727
- ["SapLogger", "SapLogger", "Member[debug,error,fatal,info,setLevel,trace,warning,getLogger].ReturnValue"]
28+
- ["SapLogEntries", "SapLogger", "Member[addLogListener].Parameter[0].Member[onLogEntry].Argument[0]"]
29+
- ["SapLogEntries", "SapLogger", "Member[getLogEntries].ReturnValue"]
2830
- ["ResourceBundle", "ResourceBundle", "Instance"]
2931
- ["ResourceBundle", "sap/base/i18n/ResourceBundle", ""]
3032
- ["Properties", "Properties", "Instance"]
@@ -64,7 +66,6 @@ extensions:
6466
- ["UI5ClientStorage", "sap/ui/core/util/File", ""]
6567
- ["UI5ClientStorage", "global", "Member[sap].Member[ui].Member[core].Member[util].Member[File]"]
6668

67-
6869
- addsTo:
6970
pack: codeql/javascript-all
7071
extensible: "sourceModel"

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -848,7 +848,7 @@ module BindingStringParser<BindingStringReaderSig BindingStringReader> {
848848
)
849849
}
850850

851-
newtype TBindingPathComponentList =
851+
private newtype TBindingPathComponentList =
852852
MkEmptyBindingPathComponentList() or
853853
MkConstBindingPathComponentList(NameToken headToken, BindingPathComponentList tail, Token source) {
854854
exists(Token nextToken | nextToken = getNextSkippingWhitespace(headToken) |
@@ -920,7 +920,7 @@ module BindingStringParser<BindingStringReaderSig BindingStringReader> {
920920
)
921921
}
922922

923-
newtype TBindingPath =
923+
private newtype TBindingPath =
924924
MkAbsoluteBindingPath(BindingPathComponentList pathComponents, Token source) {
925925
source instanceof ForwardSlashToken and
926926
mkBindingPathComponentList(getNextSkippingWhitespace(source), pathComponents, _)
@@ -1038,7 +1038,7 @@ module BindingStringParser<BindingStringReaderSig BindingStringReader> {
10381038
mkRelativeBindingPathWithModel(first, bindingPath, last)
10391039
}
10401040

1041-
newtype TBinding =
1041+
private newtype TBinding =
10421042
MkBindingPath(Token first, BindingPath bindingPath, Token last) {
10431043
exists(
10441044
LeftBracketToken leftBracketToken, RightBracketToken rightBracketToken,

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

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,10 @@ class WebAppManifest extends File {
7878
WebApp getWebapp() { result = webapp }
7979
}
8080

81+
predicate inSameWebApp(File f1, File f2) {
82+
exists(WebApp webApp | webApp.getAResource() = f1 and webApp.getAResource() = f2)
83+
}
84+
8185
/** A UI5 bootstrapped web application. */
8286
class WebApp extends HTML::HtmlFile {
8387
SapUiCoreScriptElement coreScript;
@@ -368,9 +372,7 @@ class ControlReference extends Reference {
368372
result.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() = propertyName
369373
)
370374
) and
371-
exists(WebApp webApp |
372-
webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()
373-
)
375+
inSameWebApp(this.getFile(), result.getFile())
374376
}
375377

376378
MethodCallNode getAWrite(string propertyName) {
@@ -400,9 +402,7 @@ class ControlReference extends Reference {
400402
result.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() = propertyName
401403
)
402404
) and
403-
exists(WebApp webApp |
404-
webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()
405-
)
405+
inSameWebApp(this.getFile(), result.getFile())
406406
}
407407
}
408408

@@ -1297,7 +1297,7 @@ class Extension extends InvokeNode, MethodCallNode {
12971297
SapDefineModule getDefine() { this.getEnclosingFunction() = result.getArgument(1).asExpr() }
12981298
}
12991299

1300-
newtype TSapElement =
1300+
private newtype TSapElement =
13011301
DefinitionOfElement(Extension extension) or
13021302
ReferenceOfElement(Reference reference)
13031303

@@ -1437,9 +1437,7 @@ class PropertyMetadata extends ObjectLiteralNode {
14371437
result.getMethodName() = "setProperty" and
14381438
result.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() = name
14391439
) and
1440-
exists(WebApp webApp |
1441-
webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()
1442-
)
1440+
inSameWebApp(this.getFile(), result.getFile())
14431441
}
14441442

14451443
MethodCallNode getARead() {
@@ -1474,8 +1472,6 @@ class PropertyMetadata extends ObjectLiteralNode {
14741472
result.getMethodName() = "getProperty" and
14751473
result.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue() = name
14761474
) and
1477-
exists(WebApp webApp |
1478-
webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()
1479-
)
1475+
inSameWebApp(this.getFile(), result.getFile())
14801476
}
14811477
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import javascript
22
import DataFlow
33
import advanced_security.javascript.frameworks.ui5.UI5
44

5-
newtype TFrameOptions =
5+
private newtype TFrameOptions =
66
/*
77
* <script id='sap-ui-bootstrap'
88
* src='resources/sap-ui-core.js'

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

Lines changed: 14 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -136,9 +136,7 @@ abstract class UI5BindingPath extends BindingPath {
136136
)
137137
// and
138138
// /* This binding path and the resulting model should live inside the same webapp */
139-
// exists(WebApp webApp |
140-
// webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()
141-
// )
139+
// inSameWebApp(this.getFile(), result.getFile())
142140
}
143141

144142
/**
@@ -151,10 +149,7 @@ abstract class UI5BindingPath extends BindingPath {
151149
result.(DataFlow::PropWrite).getPropertyNameExpr() = p.getNameExpr() and
152150
this.getAbsolutePath() = model.getPathString(p) and
153151
/* Restrict search to inside the same webapp. */
154-
exists(WebApp webApp |
155-
webApp.getAResource() = this.getLocation().getFile() and
156-
webApp.getAResource() = result.getFile()
157-
)
152+
inSameWebApp(this.getLocation().getFile(), result.getFile())
158153
)
159154
or
160155
/* 1-2. Internal (Client-side) model, model loaded from JSON file */
@@ -164,19 +159,13 @@ abstract class UI5BindingPath extends BindingPath {
164159
this.getPath() = model.getPathStringPropName(propName) and
165160
exists(JsonObject obj, JsonValue val | val = obj.getPropValue(propName)) and
166161
/* Restrict search to inside the same webapp. */
167-
exists(WebApp webApp |
168-
webApp.getAResource() = this.getLocation().getFile() and
169-
webApp.getAResource() = result.getFile()
170-
)
162+
inSameWebApp(this.getLocation().getFile(), result.getFile())
171163
)
172164
or
173165
/* 2. External (Server-side) model */
174166
result = this.getModel().(UI5ExternalModel) and
175167
/* Restrict search to inside the same webapp. */
176-
exists(WebApp webApp |
177-
webApp.getAResource() = this.getLocation().getFile() and
178-
webApp.getAResource() = result.getFile()
179-
)
168+
inSameWebApp(this.getLocation().getFile(), result.getFile())
180169
}
181170
}
182171

@@ -212,9 +201,7 @@ abstract class UI5View extends File {
212201
/* The controller name should match between the view and the controller definition. */
213202
result.getName() = this.getControllerName() and
214203
/* The View and the Controller are in a same webapp. */
215-
exists(WebApp webApp |
216-
webApp.getAResource() = this and webApp.getAResource() = result.getFile()
217-
)
204+
inSameWebApp(this, result.getFile())
218205
}
219206

220207
abstract UI5Control getControl();
@@ -304,10 +291,7 @@ class JsView extends UI5View {
304291
/* 2. A custom control with implementation code found in the webapp */
305292
exists(CustomControl control |
306293
control.getName() = node.asExpr().getAChildExpr().(DotExpr).getQualifiedName() and
307-
exists(WebApp webApp |
308-
webApp.getAResource() = control.getFile() and
309-
webApp.getAResource() = node.getFile()
310-
)
294+
inSameWebApp(control.getFile(), node.getFile())
311295
)
312296
)
313297
)
@@ -367,10 +351,7 @@ class JsonView extends UI5View {
367351
/* 2. A custom control with implementation code found in the webapp */
368352
exists(CustomControl control |
369353
control.getName() = object.getPropStringValue("Type") and
370-
exists(WebApp webApp |
371-
webApp.getAResource() = control.getFile() and
372-
webApp.getAResource() = object.getFile()
373-
)
354+
inSameWebApp(control.getFile(), object.getFile())
374355
)
375356
)
376357
)
@@ -516,10 +497,7 @@ class HtmlView extends UI5View, HTML::HtmlFile {
516497
/* 2. A custom control with implementation code found in the webapp */
517498
exists(CustomControl control |
518499
control.getName() = element.getAttributeByName("sap-ui-type").getValue() and
519-
exists(WebApp webApp |
520-
webApp.getAResource() = control.getFile() and
521-
webApp.getAResource() = element.getFile()
522-
)
500+
inSameWebApp(control.getFile(), element.getFile())
523501
)
524502
)
525503
)
@@ -699,17 +677,14 @@ class XmlView extends UI5View instanceof XmlFile {
699677
/* 2. A custom control with implementation code found in the webapp */
700678
exists(CustomControl control |
701679
control.getName() = element.getNamespace().getUri() + "." + element.getName() and
702-
exists(WebApp webApp |
703-
webApp.getAResource() = control.getFile() and
704-
webApp.getAResource() = element.getFile()
705-
)
680+
inSameWebApp(control.getFile(), element.getFile())
706681
)
707682
)
708683
)
709684
}
710685
}
711686

712-
newtype TUI5Control =
687+
private newtype TUI5Control =
713688
TXmlControl(XmlElement control) or
714689
TJsonControl(JsonObject control) {
715690
exists(JsonView view | control.getParent() = view.getRoot().getPropValue("content"))
@@ -801,10 +776,7 @@ class UI5Control extends TUI5Control {
801776
*/
802777
CustomControl getDefinition() {
803778
result.getName() = this.getQualifiedType() and
804-
exists(WebApp webApp |
805-
webApp.getAResource() = this.getFile() and
806-
webApp.getAResource() = result.getFile()
807-
)
779+
inSameWebApp(this.getFile(), result.getFile())
808780
}
809781

810782
/**
@@ -840,20 +812,14 @@ class UI5Control extends TUI5Control {
840812
bindingset[propName]
841813
MethodCallNode getARead(string propName) {
842814
// TODO: in same view
843-
exists(WebApp webApp |
844-
webApp.getAResource() = this.getFile() and
845-
webApp.getAResource() = result.getFile()
846-
) and
815+
inSameWebApp(this.getFile(), result.getFile()) and
847816
result.getMethodName() = "get" + capitalize(propName)
848817
}
849818

850819
bindingset[propName]
851820
MethodCallNode getAWrite(string propName) {
852821
// TODO: in same view
853-
exists(WebApp webApp |
854-
webApp.getAResource() = this.getFile() and
855-
webApp.getAResource() = result.getFile()
856-
) and
822+
inSameWebApp(this.getFile(), result.getFile()) and
857823
result.getMethodName() = "set" + capitalize(propName)
858824
}
859825

@@ -879,7 +845,7 @@ class UI5Control extends TUI5Control {
879845
CustomController getController() { result = this.getView().getController() }
880846
}
881847

882-
newtype TUI5ControlProperty =
848+
private newtype TUI5ControlProperty =
883849
TXmlControlProperty(XmlAttribute property) or
884850
TJsonControlProperty(JsonValue property) or
885851
TJsControlProperty(ValueNode property)

‎javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/dataflow/DataFlow.qll

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ class LocalModelContentBoundBidirectionallyToHtmlISinkControl extends DomBasedXs
6161
}
6262

6363
module UI5PathGraph {
64-
newtype TNode =
64+
private newtype TNode =
6565
TUI5BindingPathNode(UI5BindingPath path) or
6666
TDataFlowNode(DataFlow::Node node)
6767

@@ -76,6 +76,12 @@ module UI5PathGraph {
7676
result = this.asUI5BindingPathNode().toString()
7777
}
7878

79+
File getFile() {
80+
result = this.asDataFlowNode().getFile()
81+
or
82+
result = this.asUI5BindingPathNode().getView()
83+
}
84+
7985
predicate hasLocationInfo(
8086
string filepath, int startline, int startcolumn, int endline, int endcolumn
8187
) {

0 commit comments

Comments
 (0)