RA-1875: EMPT110 Fixed XSS Vulnerability in AppId field on User App Page #89
Conversation
This file contains 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
HerbertYiga
reviewed
Apr 20, 2021
| @@ -52,27 +53,28 @@ public void get(PageModel model, @RequestParam(value = "appId", required = false | |||
| public String post(PageModel model, @ModelAttribute(value = "appId") @BindParams UserApp userApp, | |||
| @RequestParam("action") String action, | |||
| @SpringBean("appFrameworkService") AppFrameworkService service, HttpSession session, UiUtils ui) { | |||
|
|
|||
|
|
|||
| String htmlSafeAppId = StringEscapeUtils.escapeHtml(userApp.getAppId()); | |||
isears
requested changes
Apr 20, 2021
| try { | ||
| AppDescriptor descriptor = mapper.readValue(userApp.getJson(), AppDescriptor.class); | ||
| if (!userApp.getAppId().equals(descriptor.getId())) { | ||
| if (!htmlSafeAppId.equals(descriptor.getId())) { |
There was a problem hiding this comment.
Is using the htmlSafeAppId here instead of the real appId strictly necessary to prevent XSS? (also on line 63)
If not, let's just use the real unescaped appId and only use the html-escaped app-id when it gets displayed in UI messages.
There was a problem hiding this comment.
@isears I have restricted encoding of appId to just when error messages are displayed
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.
Description of What I Changed
I encoded the AppId (entered by the user via AppId field) before it is referenced for further processing to prevent any XSS attacks.
Issue I Worked On
A script or an iframe could be injected in the AppId field while adding app definition. I encoded the AppId in the controller file before it was referenced for usage to prevent any XSS.
Steps to reproduce the vulnerability:
</script><script>alert('XSS');</script>.Output: A dialog box pops up with 'XSS' written on it.
Link to ticket
RA-1875
@isears