Improve UI5Xss.ql query to detect default OData model with bindElement
#283
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The setModel check in UI5BindingPath.getASource() was globally scoped, causing detection to fail when apps with explicit setModel calls were in the same database as apps using default OData models from manifest.json. Changes: - Bindings.qll: Add getBindElementCall() method to Binding class - RemoteFlowSources.qll: Fix DefaultODataServiceModel.asBinding() to use getBindElementCall() for proper context binding matching - UI5View.qll: Scope setModel check to same webapp using inSameWebApp() - Add test case xss-fragment-odata-default-model validating the fix All 26 UI5Xss tests pass.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request enhances the detection of XSS vulnerabilities in SAP UI5 applications by improving how context bindings via bindElement are tracked when using default OData models. The changes refine the query logic to more accurately identify when fragments bound to OData entities through the default model contain XSS vulnerabilities.
Changes:
- Added
getBindElementCall()method to retrieve thebindElementcall node from context bindings - Fixed
DefaultODataServiceModel.asBinding()to correctly match bindings by theirbindElementcall - Refined model resolution logic to only consider
setModelcalls within the same webapp - Added comprehensive test case demonstrating XSS vulnerability detection with fragments and default OData models
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Bindings.qll | Added getBindElementCall() method to extract bindElement call from context bindings |
| javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/RemoteFlowSources.qll | Fixed DefaultODataServiceModel.asBinding() to correctly match bindings using the new method |
| javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll | Refined model resolution to only check setModel calls in same webapp |
| javascript/frameworks/ui5/test/queries/UI5Xss/xss-fragment-odata-default-model/* | Complete test case with manifest, controllers, views, fragments, and expected results |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| <VBox class="sapUiSmallMargin"> | ||
| <Label text="Message from OData:" /> | ||
| <!-- XSS vulnerability: HTML content bound to OData property containing unsanitized data --> | ||
| <core:HTML content="{message}" /> |
Check failure
Code scanning / CodeQL
UI5 Client-side cross-site scripting High test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What This PR Contributes
This pull request improves the detection of XSS vulnerabilities in SAP UI5 applications by enhancing the binding analysis logic and adding a comprehensive test case for scenarios involving fragments and default OData models. The main changes include refining how context bindings via
bindElementare tracked, updating the logic for identifying default model usage, and introducing a new test suite to validate the detection of XSS in this context.Outline of Changes
Key improvements include:
Improvements to OData Default Model Binding Detection:
getBindElementCall()to theBindingclass to accurately retrieve thebindElementmethod call node for context bindings.asBinding()method inDefaultODataServiceModelto match context bindings whosebindElementcall is this node, improving the association between the model and its bindings.UI5BindingPathto only considersetModelcalls within the same webapp, preventing false associations with unrelated models.New Test Case for XSS Vulnerability:
xss-fragment-odata-default-model) demonstrating an XSS vulnerability when a fragment is bound to an OData entity via the default model and displays unsanitized data. This includes all necessary configuration, controller, fragment, and manifest files. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]