Skip to content

UI5 client side log-injection improvements #136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 15 commits into from
Aug 7, 2024
Merged

UI5 client side log-injection improvements #136

merged 15 commits into from
Aug 7, 2024

Conversation

@mbaluda
Copy link
Contributor

@mbaluda mbaluda commented Aug 1, 2024 •
edited
Loading

  • Adds dataflow step so that existing queries can follow the path through log handlers 6874

  • Implements 2 new queries

    1. UI5UnsafeLogAccess: alerts for potential injection only if a custom log handler or a call to getLogEntries is present 6873
    2. UI5LogsToHttp: alerts on tainted logs sent over the network 6869

  • Test cases

  • Help .md files

github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 1, 2024
View reviewed changes
.../ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/webapp/utils/CustomLogListener.js
const http = new XMLHttpRequest();
const url = "https://some.remote.server/location";
http.open("POST", url);
http.send(oEvent.message); // js/ui5-log-injection-to-http

Check warning

Code scanning / CodeQL

UI5 Log injection in outbound network request

Outbound network request depends on [user-provided](1) log data.
...s/ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/webapp/utils/LogEntriesToHttp.js
const url = "https://some.remote.server/location";
http.open("POST", url);
http.send(oEvent.message);
http.send(message); // js/ui5-log-injection-to-http

Check warning

Code scanning / CodeQL

UI5 Log injection in outbound network request

Outbound network request depends on [user-provided](1) log data.
.../ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/webapp/utils/CustomLogListener.js
constructor: function () {
Log.addLogListener(this);
},
onLogEntry: function (oEvent) {

Check warning

Code scanning / CodeQL

Access to user-controlled UI5 Logs

Processing UI5 log entries that depend on [user-provided data](1). Processing UI5 log entries that depend on [user-provided data](2). Processing UI5 log entries that depend on [user-provided data](3). Processing UI5 log entries that depend on [user-provided data](4). Processing UI5 log entries that depend on [user-provided data](5). Processing UI5 log entries that depend on [user-provided data](6). Processing UI5 log entries that depend on [user-provided data](7). Processing UI5 log entries that depend on [user-provided data](8).
...s/ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/webapp/utils/LogEntriesToHttp.js
Log.addLogListener(this);
},
onLogEntry: function (oEvent) {
let message = Log.getLogEntries()[0].message;

Check warning

Code scanning / CodeQL

Access to user-controlled UI5 Logs

Processing UI5 log entries that depend on [user-provided data](1). Processing UI5 log entries that depend on [user-provided data](2). Processing UI5 log entries that depend on [user-provided data](3). Processing UI5 log entries that depend on [user-provided data](4). Processing UI5 log entries that depend on [user-provided data](5). Processing UI5 log entries that depend on [user-provided data](6). Processing UI5 log entries that depend on [user-provided data](7). Processing UI5 log entries that depend on [user-provided data](8).
.../ui5/test/queries/UI5LogInjection/log-entry-flows-to-sinks/webapp/utils/CustomLogListener.js
http.send(oEvent.message); // js/ui5-log-injection-to-http

$('myId').html(oEvent.message) //Xss
jQuery.sap.globalEval(oEvent.message); //UI5 Xss

Check warning

Code scanning / CodeQL

UI5 Client-side cross-site scripting

XSS vulnerability due to [user-provided value](1).
@jeongsoolee09
Copy link
Contributor

jeongsoolee09 commented Aug 1, 2024

At first glance, it looks good overall. I wonder why you removed the condition that states "two locations must be in a same application"?

@mbaluda
Copy link
Contributor Author

mbaluda commented Aug 1, 2024

At first glance, it looks good overall. I wonder why you removed the condition that states "two locations must be in a same application"?

I just replaced it with a predicate doing the same thing

jeongsoolee09
jeongsoolee09 reviewed Aug 2, 2024
View reviewed changes
jeongsoolee09
jeongsoolee09 reviewed Aug 2, 2024
View reviewed changes
Copy link
Contributor

@jeongsoolee09 jeongsoolee09 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The queries are robust, and I have nothing to point out about their structures. I left some comments mainly on naming.

@mbaluda mbaluda self-assigned this Aug 7, 2024
@mbaluda mbaluda requested a review from jeongsoolee09 August 7, 2024 14:38
@mbaluda mbaluda merged commit 0b78c55 into main Aug 7, 2024
@mbaluda mbaluda deleted the mbaluda/logi-df branch August 7, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeongsoolee09 jeongsoolee09 jeongsoolee09 approved these changes

@knewbury01 knewbury01 Awaiting requested review from knewbury01

Assignees

@mbaluda mbaluda

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mbaluda @jeongsoolee09