Skip to content

Create query for vulnerability not specific to webapp security #78

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 35 commits into from
Feb 5, 2024
Merged

Create query for vulnerability not specific to webapp security #78

merged 35 commits into from
Feb 5, 2024

Conversation

jeongsoolee09
Copy link
Contributor

@jeongsoolee09 jeongsoolee09 commented Jan 26, 2024 •
edited
Loading

What this PR contributes

This PR adds two additional queries to find two vulnerabilities not specific to web applications: path injection (CWE-022, CWE-035) and formula injection (CWE-1236).

Path Injection

A UI5-specific variant of semmle.javascript.security.dataflow.TaintedPathQuery. This query catches data flows from an arbitrary remote flow source to one of these data-flow nodes:

Test suites

  • pathSinkTest.ql
    • Ensures that the path injection sinks are recognized properly.
  • path-custom-control-property-sanitized
    • Demonstrates that the sanitization against XSS via the property of the custom control does not prevent path injection.
  • path-custom-control-sanitized
    • Demonstrates that the sanitization against XSS via an imported sanitizer (sap.base.security.encodeXML) does not prevent path injection.
  • path-html-control
    • Demonstrates that a path injection is possible without using any custom controls.

Formula Injection

This query aims to find a user-controlled data being written to a CSV or JSON file (CWE-1236 is originally about injecting into CSVs, but here it also aims to catch JSON injections as well). It catches data flows from an arbitrary remote flow source to one of these data-flow nodes:

  • Storage.put(key, value)'s second argument value
  • File.save(content, path, extension, mimetype, charset, binaryordermark?)'s first argument content
    • documentation: sap.ui.core.util.File.save
    • Since it is needed to check if the MIME type is indeed CSV or JSON and these conditionals cannot be encoded as MaD rows, we instead create a class in the configuration class (advanced_security.javascript.frameworks.ui5.UI5FormulaInjectionQuery).

There are no counterparts of this query in the standard query suite.

Test suites

  • formulaSinkTest.ql
    • Ensures that the formula injection sinks are recognized properly.
  • formula-custom-control-property-sanitized
    • Demonstrates that the sanitization against XSS via the property of the custom control does not prevent formula injection.
  • formula-custom-control-sanitized
    • Demonstrates that the sanitization against XSS via an imported sanitizer (sap.base.security.encodeXML) does not prevent formula injection.
  • formula-html-control
    • Demonstrates that a formula injection is possible without using any custom controls.

Miscellaneous

  • Changed sap.ui.require at the toplevel of sink.js to sap.ui.define. Technically it is also correct to use require, but it complicates things since the query codebase targets sap.ui.define.

Future Work

  • Figure out why import semmle.javascript.security.dataflow.TaintedPathQuery as TaintedPathQuery makes the test formula-custom-control-sanitized fail.
  • Cover sap.ui.require as well as its jQuery counterpart, jquery.sap.require.

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 26, 2024
View reviewed changes
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 31, 2024
View reviewed changes
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 31, 2024
View reviewed changes
.../queries/UI5FormulaInjection/formula-custom-control-property-sanitized/webapp/control/xss.js
/* Data is sanitized against XSS. */
oRm.unsafeHtml(oControl.getText());
/* Data is not sanitized against formula injection. */
File.save(oControl.getText(), "/some/path/", "csv", "text/csv", "utf-8");

Check failure

Code scanning / CodeQL

UI5 Formula Injection

The content of a saved file depends on a [user-provided value](1). The content of a saved file depends on a [user-provided value](2). The content of a saved file depends on a [user-provided value](3). The content of a saved file depends on a [user-provided value](4). The content of a saved file depends on a [user-provided value](5). The content of a saved file depends on a [user-provided value](6). The content of a saved file depends on a [user-provided value](7). The content of a saved file depends on a [user-provided value](8). The content of a saved file depends on a [user-provided value](9). The content of a saved file depends on a [user-provided value](10). The content of a saved file depends on a [user-provided value](11). The content of a saved file depends on a [user-provided value](12). The content of a saved file depends on a [user-provided value](13). The content of a saved file depends on a [user-provided value](14). The content of a saved file depends on a [user-provided value](15). The content of a saved file depends on a [user-provided value](16). The content of a saved file depends on a [user-provided value](17). The content of a saved file depends on a [user-provided value](18). The content of a saved file depends on a [user-provided value](19). The content of a saved file depends on a [user-provided value](20). The content of a saved file depends on a [user-provided value](21). The content of a saved file depends on a [user-provided value](22). The content of a saved file depends on a [user-provided value](23). The content of a saved file depends on a [user-provided value](24). The content of a saved file depends on a [user-provided value](25). The content of a saved file depends on a [user-provided value](26). The content of a saved file depends on a [user-provided value](27). The content of a saved file depends on a [user-provided value](28). The content of a saved file depends on a [user-provided value](29). The content of a saved file depends on a [user-provided value](30). The content of a saved file depends on a [user-provided value](31). The content of a saved file depends on a [user-provided value](32). The content of a saved file depends on a [user-provided value](33).
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 31, 2024
View reviewed changes
@jeongsoolee09
Change sap.ui.require to sap.ui.define
2991500 
Technically it is also correct to use `require` here, but
doing so complicates things since our queries are
geared towards using `sap.ui.define`.
@jeongsoolee09
Debug query and update .expected of UI5FormulaInjection
668370a
@jeongsoolee09
Update .expected of pathSinkTest
ab98adc
@jeongsoolee09
Fix unit test suite and update .expected
fef382b
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 1, 2024
View reviewed changes
.../ui5/test/queries/UI5FormulaInjection/formula-custom-control-sanitized/webapp/control/xss.js
oRm.unsafeHtml(xssSanitized);
oRm.close("div");
/* Data is not sanitized against formula injection. */
File.save(xssSanitized, "/some/path/", "csv", "text/csv", "utf-8");

Check failure

Code scanning / CodeQL

UI5 Formula Injection

The content of a saved file depends on a [user-provided value](1). The content of a saved file depends on a [user-provided value](2). The content of a saved file depends on a [user-provided value](3). The content of a saved file depends on a [user-provided value](4). The content of a saved file depends on a [user-provided value](5). The content of a saved file depends on a [user-provided value](6). The content of a saved file depends on a [user-provided value](7). The content of a saved file depends on a [user-provided value](8). The content of a saved file depends on a [user-provided value](9). The content of a saved file depends on a [user-provided value](10). The content of a saved file depends on a [user-provided value](11). The content of a saved file depends on a [user-provided value](12). The content of a saved file depends on a [user-provided value](13). The content of a saved file depends on a [user-provided value](14). The content of a saved file depends on a [user-provided value](15). The content of a saved file depends on a [user-provided value](16). The content of a saved file depends on a [user-provided value](17). The content of a saved file depends on a [user-provided value](18). The content of a saved file depends on a [user-provided value](19). The content of a saved file depends on a [user-provided value](20). The content of a saved file depends on a [user-provided value](21). The content of a saved file depends on a [user-provided value](22). The content of a saved file depends on a [user-provided value](23). The content of a saved file depends on a [user-provided value](24). The content of a saved file depends on a [user-provided value](25). The content of a saved file depends on a [user-provided value](26). The content of a saved file depends on a [user-provided value](27). The content of a saved file depends on a [user-provided value](28). The content of a saved file depends on a [user-provided value](29). The content of a saved file depends on a [user-provided value](30). The content of a saved file depends on a [user-provided value](31). The content of a saved file depends on a [user-provided value](32). The content of a saved file depends on a [user-provided value](33).
...test/queries/UI5FormulaInjection/formula-html-control-df/webapp/controller/app.controller.js
this.getView().setModel(oModel);

/* Data is not sanitized against formula injection. */
File.save(oModel.getProperty('/input'), "/some/path/", "csv", "text/csv", "utf-8");

Check failure

Code scanning / CodeQL

UI5 Formula Injection

The content of a saved file depends on a [user-provided value](1). The content of a saved file depends on a [user-provided value](2). The content of a saved file depends on a [user-provided value](3). The content of a saved file depends on a [user-provided value](4). The content of a saved file depends on a [user-provided value](5). The content of a saved file depends on a [user-provided value](6). The content of a saved file depends on a [user-provided value](7). The content of a saved file depends on a [user-provided value](8). The content of a saved file depends on a [user-provided value](9). The content of a saved file depends on a [user-provided value](10). The content of a saved file depends on a [user-provided value](11). The content of a saved file depends on a [user-provided value](12). The content of a saved file depends on a [user-provided value](13). The content of a saved file depends on a [user-provided value](14). The content of a saved file depends on a [user-provided value](15). The content of a saved file depends on a [user-provided value](16). The content of a saved file depends on a [user-provided value](17). The content of a saved file depends on a [user-provided value](18). The content of a saved file depends on a [user-provided value](19). The content of a saved file depends on a [user-provided value](20). The content of a saved file depends on a [user-provided value](21). The content of a saved file depends on a [user-provided value](22). The content of a saved file depends on a [user-provided value](23). The content of a saved file depends on a [user-provided value](24). The content of a saved file depends on a [user-provided value](25). The content of a saved file depends on a [user-provided value](26). The content of a saved file depends on a [user-provided value](27). The content of a saved file depends on a [user-provided value](28). The content of a saved file depends on a [user-provided value](29). The content of a saved file depends on a [user-provided value](30). The content of a saved file depends on a [user-provided value](31). The content of a saved file depends on a [user-provided value](32). The content of a saved file depends on a [user-provided value](33).
...eworks/ui5/test/queries/UI5FormulaInjection/formula-html-control-df/webapp/view/app.view.xml
<Input placeholder="Enter Payload"
description="Try: &lt;img src=x onerror=alert(&quot;XSS&quot;)&gt;"
value="{/input}" /> <!--User input source sap.m.Input.value -->
<core:HTML id="xssSink" content="{/input}"/> <!--XSS sink sap.ui.core.HTML.content -->

Check warning

Code scanning / CodeQL

UI5 Client-side cross-site scripting

XSS vulnerability due to [user-provided value](1). XSS vulnerability due to [user-provided value](2). XSS vulnerability due to [user-provided value](3). XSS vulnerability due to [user-provided value](4). XSS vulnerability due to [user-provided value](5). XSS vulnerability due to [user-provided value](6). XSS vulnerability due to [user-provided value](7). XSS vulnerability due to [user-provided value](8). XSS vulnerability due to [user-provided value](9). XSS vulnerability due to [user-provided value](10). XSS vulnerability due to [user-provided value](11). XSS vulnerability due to [user-provided value](12). XSS vulnerability due to [user-provided value](13). XSS vulnerability due to [user-provided value](14). XSS vulnerability due to [user-provided value](15). XSS vulnerability due to [user-provided value](16). XSS vulnerability due to [user-provided value](17). XSS vulnerability due to [user-provided value](18). XSS vulnerability due to [user-provided value](19). XSS vulnerability due to [user-provided value](20). XSS vulnerability due to [user-provided value](21). XSS vulnerability due to [user-provided value](22). XSS vulnerability due to [user-provided value](23). XSS vulnerability due to [user-provided value](24). XSS vulnerability due to [user-provided value](25). XSS vulnerability due to [user-provided value](26). XSS vulnerability due to [user-provided value](27). XSS vulnerability due to [user-provided value](28). XSS vulnerability due to [user-provided value](29). XSS vulnerability due to [user-provided value](30). XSS vulnerability due to [user-provided value](31). XSS vulnerability due to [user-provided value](32). XSS vulnerability due to [user-provided value](33).
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 1, 2024
View reviewed changes
...5/test/queries/UI5PathInjection/path-custom-control-property-sanitized/webapp/control/xss.js
/* Data is sanitized against XSS. */
oRm.unsafeHtml(oControl.getText());
/* Data is not sanitized against formula injection. */
File.save("some_content", oControl.getText(), "csv", "text/csv", "utf-8");

Check failure

Code scanning / CodeQL

UI5 Path Injection

The path of a saved file depends on a [user-provided value](1). The path of a saved file depends on a [user-provided value](2). The path of a saved file depends on a [user-provided value](3). The path of a saved file depends on a [user-provided value](4). The path of a saved file depends on a [user-provided value](5). The path of a saved file depends on a [user-provided value](6). The path of a saved file depends on a [user-provided value](7). The path of a saved file depends on a [user-provided value](8). The path of a saved file depends on a [user-provided value](9). The path of a saved file depends on a [user-provided value](10). The path of a saved file depends on a [user-provided value](11). The path of a saved file depends on a [user-provided value](12). The path of a saved file depends on a [user-provided value](13). The path of a saved file depends on a [user-provided value](14). The path of a saved file depends on a [user-provided value](15). The path of a saved file depends on a [user-provided value](16). The path of a saved file depends on a [user-provided value](17). The path of a saved file depends on a [user-provided value](18). The path of a saved file depends on a [user-provided value](19). The path of a saved file depends on a [user-provided value](20). The path of a saved file depends on a [user-provided value](21). The path of a saved file depends on a [user-provided value](22). The path of a saved file depends on a [user-provided value](23). The path of a saved file depends on a [user-provided value](24). The path of a saved file depends on a [user-provided value](25). The path of a saved file depends on a [user-provided value](26). The path of a saved file depends on a [user-provided value](27). The path of a saved file depends on a [user-provided value](28). The path of a saved file depends on a [user-provided value](29). The path of a saved file depends on a [user-provided value](30). The path of a saved file depends on a [user-provided value](31). The path of a saved file depends on a [user-provided value](32). The path of a saved file depends on a [user-provided value](33).
...eworks/ui5/test/queries/UI5PathInjection/path-custom-control-sanitized/webapp/control/xss.js
oRm.unsafeHtml(xssSanitized);
oRm.close("div");
/* Data is not sanitized against formula injection. */
File.save("some_content", xssSanitized, "csv", "text/csv", "utf-8");

Check failure

Code scanning / CodeQL

UI5 Path Injection

The path of a saved file depends on a [user-provided value](1). The path of a saved file depends on a [user-provided value](2). The path of a saved file depends on a [user-provided value](3). The path of a saved file depends on a [user-provided value](4). The path of a saved file depends on a [user-provided value](5). The path of a saved file depends on a [user-provided value](6). The path of a saved file depends on a [user-provided value](7). The path of a saved file depends on a [user-provided value](8). The path of a saved file depends on a [user-provided value](9). The path of a saved file depends on a [user-provided value](10). The path of a saved file depends on a [user-provided value](11). The path of a saved file depends on a [user-provided value](12). The path of a saved file depends on a [user-provided value](13). The path of a saved file depends on a [user-provided value](14). The path of a saved file depends on a [user-provided value](15). The path of a saved file depends on a [user-provided value](16). The path of a saved file depends on a [user-provided value](17). The path of a saved file depends on a [user-provided value](18). The path of a saved file depends on a [user-provided value](19). The path of a saved file depends on a [user-provided value](20). The path of a saved file depends on a [user-provided value](21). The path of a saved file depends on a [user-provided value](22). The path of a saved file depends on a [user-provided value](23). The path of a saved file depends on a [user-provided value](24). The path of a saved file depends on a [user-provided value](25). The path of a saved file depends on a [user-provided value](26). The path of a saved file depends on a [user-provided value](27). The path of a saved file depends on a [user-provided value](28). The path of a saved file depends on a [user-provided value](29). The path of a saved file depends on a [user-provided value](30). The path of a saved file depends on a [user-provided value](31). The path of a saved file depends on a [user-provided value](32). The path of a saved file depends on a [user-provided value](33).
...s/ui5/test/queries/UI5PathInjection/path-html-control-df/webapp/controller/app.controller.js
this.getView().setModel(oModel);

/* Data is not sanitized against formula injection. */
File.save("some_content", oModel.getProperty('/input'), "csv", "text/csv", "utf-8");

Check failure

Code scanning / CodeQL

UI5 Path Injection

The path of a saved file depends on a [user-provided value](1). The path of a saved file depends on a [user-provided value](2). The path of a saved file depends on a [user-provided value](3). The path of a saved file depends on a [user-provided value](4). The path of a saved file depends on a [user-provided value](5). The path of a saved file depends on a [user-provided value](6). The path of a saved file depends on a [user-provided value](7). The path of a saved file depends on a [user-provided value](8). The path of a saved file depends on a [user-provided value](9). The path of a saved file depends on a [user-provided value](10). The path of a saved file depends on a [user-provided value](11). The path of a saved file depends on a [user-provided value](12). The path of a saved file depends on a [user-provided value](13). The path of a saved file depends on a [user-provided value](14). The path of a saved file depends on a [user-provided value](15). The path of a saved file depends on a [user-provided value](16). The path of a saved file depends on a [user-provided value](17). The path of a saved file depends on a [user-provided value](18). The path of a saved file depends on a [user-provided value](19). The path of a saved file depends on a [user-provided value](20). The path of a saved file depends on a [user-provided value](21). The path of a saved file depends on a [user-provided value](22). The path of a saved file depends on a [user-provided value](23). The path of a saved file depends on a [user-provided value](24). The path of a saved file depends on a [user-provided value](25). The path of a saved file depends on a [user-provided value](26). The path of a saved file depends on a [user-provided value](27). The path of a saved file depends on a [user-provided value](28). The path of a saved file depends on a [user-provided value](29). The path of a saved file depends on a [user-provided value](30). The path of a saved file depends on a [user-provided value](31). The path of a saved file depends on a [user-provided value](32). The path of a saved file depends on a [user-provided value](33).
@jeongsoolee09 jeongsoolee09 requested a review from mbaluda February 1, 2024 22:26
@jeongsoolee09 jeongsoolee09 marked this pull request as ready for review February 1, 2024 22:26
mbaluda
mbaluda approved these changes Feb 5, 2024
View reviewed changes
Copy link
Contributor

@mbaluda mbaluda left a comment

Choose a reason for hiding this comment

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

LGTM!
Nice to see that queries look pretty standardized within the project 👍

mbaluda
mbaluda approved these changes Feb 5, 2024
View reviewed changes
Copy link
Contributor

@mbaluda mbaluda left a comment

Choose a reason for hiding this comment

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

I suggested a few changes for the help files, feel free to use what you think is good

jeongsoolee09 and others added 8 commits February 5, 2024 13:43
@jeongsoolee09 @mbaluda
Update javascript/frameworks/ui5/src/UI5FormulaInjection/UI5FormulaIn…
e611865 
…jection.md

Co-authored-by: Mauro Baluda <mbaluda@github.com>
@jeongsoolee09 @mbaluda
Update javascript/frameworks/ui5/src/UI5FormulaInjection/UI5FormulaIn…
80fce3e 
…jection.md

Co-authored-by: Mauro Baluda <mbaluda@github.com>
@jeongsoolee09 @mbaluda
Update javascript/frameworks/ui5/src/UI5FormulaInjection/UI5FormulaIn…
5b65c4f 
…jection.md

Co-authored-by: Mauro Baluda <mbaluda@github.com>
@jeongsoolee09 @mbaluda
Update javascript/frameworks/ui5/src/UI5PathInjection/UI5PathInjectio…
4598870 
…n.md

Co-authored-by: Mauro Baluda <mbaluda@github.com>
@jeongsoolee09 @mbaluda
Update javascript/frameworks/ui5/src/UI5PathInjection/UI5PathInjectio…
b72787e 
…n.md

Co-authored-by: Mauro Baluda <mbaluda@github.com>
@jeongsoolee09 @mbaluda
Update javascript/frameworks/ui5/src/UI5PathInjection/UI5PathInjectio…
bda7969 
…n.md

Co-authored-by: Mauro Baluda <mbaluda@github.com>
@jeongsoolee09 @mbaluda
Update javascript/frameworks/ui5/src/UI5PathInjection/UI5PathInjectio…
e5b4b78 
…n.md

Co-authored-by: Mauro Baluda <mbaluda@github.com>
@jeongsoolee09 @mbaluda
Update javascript/frameworks/ui5/src/UI5PathInjection/UI5PathInjectio…
7b19d39 
…n.md

Co-authored-by: Mauro Baluda <mbaluda@github.com>
@jeongsoolee09 jeongsoolee09 merged commit a301cd6 into main Feb 5, 2024
@jeongsoolee09 jeongsoolee09 deleted the jeongsoolee09/non-webapp-specific-vulns branch February 5, 2024 22:03
@jeongsoolee09
Copy link
Contributor Author

@mbaluda I ended up using all suggestions, thank you!

@jeongsoolee09 jeongsoolee09 mentioned this pull request Feb 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbaluda mbaluda mbaluda approved these changes

Assignees

@jeongsoolee09 jeongsoolee09

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jeongsoolee09 @mbaluda