Community
Participate
Working Groups
Overview: It is possible to accidentally introduce a XSS vulnerability using the RAP Markup feature. Steps to Reproduce: From any RAP project, including the demos, create a label using markup, and call setText(text) on the label before calling setData(RWT.MARKUP_ENABLED, true). Example: String xss = "<b><img src=x onerror=\"alert(document.cookie);\"</b>"; Label label = new Label(parent, SWT.NONE); label.setText(xss); label.setData(RWT.MARKUP_ENABLED, true); Actual Results: The javascript in the text will get executed in the client browser and display the user's cookie. Expected Results: Calling setText() on a label should not execute any javascript contained in it. Build/Platform: Tested on RAP 3.8 E4 with Chrome 78.0.3904.87 Additional Information: While the RAP developer guide Markup discussion (https://www.eclipse.org/rap/developers-guide/devguide.php?topic=markup.html&version=2.0) briefly mentions to call setData first after the constructor, it is not obvious that not calling it first introduces an XSS vulnerability as the text does not go through the validity checks that are called if setText() is called after MARKUP is enabled. Swapping the two lines prevents the vulnerability: String xss = "<b><img src=x onerror=\"alert(document.cookie);\"</b>"; Label label = new Label(parent, SWT.NONE); label.setData(RWT.MARKUP_ENABLED, true); label.setText(xss); Calling in the wrong sequence could be an easy mistake for developers to make, as the markup in the first example typically works as well. If the input to the label comes from an untrusted source, XSS will be possible.
Project team, there's help in the handbook for managing vulnerabilities. https://www.eclipse.org/projects/handbook/#vulnerability
We can reproduce and confirm that it is indeed possible to circumvent the HTML markup validation by calling the methods in the order described in this bug which violates the documented order as described in the RAP Developer's Guide. While we think that this is an extremely rare occurrence, it is still a possibility that may happen. We discussed internally different possible solutions. At the moment we think it will be best to check that text is not yet set at the time when the markup enabled flag is set. If we detect that there is already some kind of value set on the widget (which is a programming error!), we will throw an IllegalStateException that is wrapped into a SWTException in setData instead of setting markup enabled.
That's seems like a reasonable solution. Probably best to just enforce the programmer to fix their code to use the correct order. In previous security testing of our application there have been no issues for years until a recent test showed this XSS. After investigating I discovered it was just due to the markup being added after the fact to an existing label which naturally had setText called immediately after the constructor. Since the markup worked fine, it was not obvious there were any unintended side affects. I agree this should rarely happen, but could be risky if it does. Thanks for reproducing and looking at a solution.
New Gerrit change created: https://git.eclipse.org/r/152958
Gerrit change https://git.eclipse.org/r/152958 was merged to [master]. Commit: http://git.eclipse.org/c/rap/org.eclipse.rap.git/commit/?id=8a0c79555a9c87aaf2da3f3316e611d6a260cbda
New Gerrit change created: https://git.eclipse.org/r/153315
Gerrit change https://git.eclipse.org/r/153315 was merged to [master]. Commit: http://git.eclipse.org/c/rap/org.eclipse.rap.git/commit/?id=8ba5f8ce85a3fe0f9b7d780b11075d0c310c82ec
The proposed solution from comment #2 is now implemented and available with RAP 3.11 (delivered as part of the Eclipse Simultaneous Release 2019-12), and checks during runtime that the call order is correct. For versions older than RAP 3.11 we recommend to check the order of the calls manually. Any calls to setData() with RWT.MARKUP_ENABLED (corresponds to "org.eclipse.rap.rwt.markupEnabled"), or RWT.TOOLTIP_MARKUP_ENABLED (corresponds to "org.eclipse.rap.rwt.tooltipMarkupEnabled") that are enabling markup (Boolean.TRUE) must be done *before* setting text, items (in lists, trees, grid), and tooltips.
Thanks for the fix!
