Remove FPs of js/ui5-log-injection-to-http
#190
Conversation
1. Minor comment formatting 2. Unify the TypeTrackers into one (hasDependency) 3. Some more modeling
This allows reuse of UI5LogInjection in UI5LogsToHttp.
and correctly skip the FP case
...but why has it been working even before this fix?
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| "use strict"; | ||
| return BaseObject.extend("codeql-sap-js.log.CustomLogListener", { | ||
| constructor: function () { | ||
| let message = Log.getLogEntries()[0].message; |
Check warning
Code scanning / CodeQL
Access to user-controlled UI5 Logs Medium test
| const http = new XMLHttpRequest(); | ||
| const url = "https://some.remote.server/location"; | ||
| http.open("POST", url); | ||
| http.send(message); // js/ui5-log-injection-to-http |
Check warning
Code scanning / CodeQL
UI5 Log injection in outbound network request Medium test
| Log.debug(input); // | ||
| /* 2. Create a JS object that implements Log.Listener on-the-fly. */ | ||
| Log.addLogListener({ | ||
| onLogEntry: function (oLogEntry) { |
Check warning
Code scanning / CodeQL
Access to user-controlled UI5 Logs Medium test
| const http = new XMLHttpRequest(); | ||
| const url = "https://some.remote.server/location"; | ||
| http.open("POST", url); | ||
| http.send(oLogEntry.message); // js/ui5-log-injection-to-http |
Check warning
Code scanning / CodeQL
UI5 Log injection in outbound network request Medium test
| constructor: function () { | ||
| Log.addLogListener(this); | ||
| }, | ||
| onLogEntry: function (oLogEntry) { |
Check warning
Code scanning / CodeQL
Access to user-controlled UI5 Logs Medium test
| const http = new XMLHttpRequest(); | ||
| const url = "https://some.remote.server/location"; | ||
| http.open("POST", url); | ||
| http.send(oLogEntry.message); // js/ui5-log-injection-to-http |
Check warning
Code scanning / CodeQL
UI5 Log injection in outbound network request Medium test
This step involves `DataFlow::FlowLabel`s that is subjective to the query that is using this step. Therefore, remove it from the shared flow step, and put it only in the query or query library which define the labels to be propagated through this step.
lcartey
left a comment
lcartey
left a comment
There was a problem hiding this comment.
Approved - only an unused state that I think should be removed.
| UI5LogInjectionConfiguration cfg, UI5PathNode source, UI5PathNode sink, UI5PathNode primarySource | ||
| class UI5LogEntryFlowState extends DataFlow::FlowLabel { | ||
| UI5LogEntryFlowState() { | ||
| this = ["not-logged-not-accessed", "logged-not-accessed", "logged-and-accessed"] |
There was a problem hiding this comment.
I think logged-not-accessed is unused.
…ced-security/codeql-sap-js into jeongsoolee09/fix-ui5-loginjection
What this PR contributes
Eliminate FPs
The query
js/ui5-log-injection-to-httpyielded false positives on code that does not depend on UI5, because the query did not check if the data being sent via HTTP is indeed derived from a UI5 log entry (by means ofonLogEntryorgetLogEntries). Therefore, enforce it by using flow labels.Tidy up code
Factor out various type trackers into one (
hasDependency/1)There were
sapControl/0,sapController/0,sapRenderer/0, andsapComponent/0that gives rise to classes such asCustomControl,CustomController, and so on. However, all of them differed in only the dependency paths they referred to, and keeping them more or less pollluted the code. Therefore, add ahasDependency/1parameterized with the dependency path.Split up
log-entry-flows-to-sinksThe unit test case
log-entry-flows-to-sinkscombined several kinds of sinks all in one test case (hence the name). Split it up into several smaller ones:log-entry-flows-to-http-getLogEntries: getLogEntries is used to fetch a log entry to be sent in an XmlHttpRequest.log-entry-flows-to-http-log-listener-js-object: a log listener as a plain JS object (that implements it implicitly) has a methodonLogEntrieswhich pulls a log entry on a log event and sends it in an XmlHttpRequest.log-entry-flows-to-http-log-listener-sap-module: Same as the example right above, instead it's a UI5 module that explicitly implements it.log-entry-flows-to-http-log-listener-sap-module-imported: See Future Work below.Miscellaneous
Fix access path of
SapLogEntriesin the library modelOne of the rows contributing to the type
SapLogEntrieshad this row:["SapLogEntries", "SapLogger", "Member[addLogListener].Parameter[0].Member[onLogEntry].Argument[0]"]It confused
ParameterwithArgumentand vice versa, so put them in the right places.Also, as per the UI5 documentation on sap/base/Log.Logger, all of the various logging functions do not return another Logger (they are of void type), so delete this row:
["SapLogger", "SapLogger", "Member[debug,error,fatal,info,setLevel,trace,warning,getLogger].ReturnValue"]Separate out
UI5LogInjectionQueryinto a library moduleThis is to reuse the configuration in
UI5LogsToHttp.Future work
log-entry-flows-to-http-log-listener-sap-module-imported). This is currently partially blocked as of now due to limitations inAmdModuleDefinition::Range(see github/codeql, PR #19359); this prevents us from modelingsap.ui.require, an another way of requiring a UI5 module.