RA-1875: EMPT97 Fixed XSS Vulnerability in returnUrl parameter of HTML Form

[The-Lady]

Description of What I Changed

I added encoding to returnUrl parameter of HTML form.

Issue I Worked On

A script could be injected in the returnUrl parameter of Html forms. I encoded the parameter to prevent any XSS attacks.

Steps to reproduce the vulnerability:

1. Launch the OpenMRS application.
2. Login with username "Admin" and password "Admin123" with location as Inpatient Ward.
3. Go to find patient page and select any patient from the displayed list OR create a new patient with dummy details if there are not any patients registered.
4. Click on 'Start Visit'.
5. Click on 'Capture Vital'.
6. Change the 'returnUrl' parameter of the url of the page to ></script><script>alert(123)</script><script> and press enter.

Output: A dialog box pops up with '123' written on it.

Link to ticket

RA-1875

@isears

[HerbertYiga]

LGTM! cc @isears

[isears]

@The-Lady this will break page links when returnUrl contains legitimate HTML characters. Is it possible to patch in the .gsp? Specifically at enterHtmlFormWithSimpleUi.gsp

[The-Lady]

@isears I tried encoding returnUrl at lines 16, 65, 66 and 78 of the enterHtmlFormWithSimpleUi.gsp, but none of them worked.
Would using escapeJs instead of escapeHtml be fine? As that also seems to prevent any script or iframe to execute and I do not have any reason or knowledge to believe that a return url would be having any JS in it (although I might be wrong on the second part).

[isears]

Would using escapeJs instead of escapeHtml be fine? As that also seems to prevent any script or iframe to execute

If escapeJs is working to prevent the XSS, go for it.

EscapeHtml will only work if the variable is injected into HTML sections of the page when the .gsp is compiled. If the variable is injected into a javascript section, we need to use escapeJs.

I do not have any reason or knowledge to believe that a return url would be having any JS in it (although I might be wrong on the second part).

You are correct, no part of the returnUrl should be executed as javascript

[The-Lady]

@isears Thank you for clearing up. I have made the desired changes.

[isears]

@The-Lady when I test this locally it looks like the "Exit Form" button breaks due to the escapeJs filtering applied to the returnUrl.

It looks like this issue is more complicated than I initially realized. I think any html- or js- encoding we do on the returnUrl parameter will break the "Exit Form" button, but of course, if we don't apply any filtering to the returnUrl parameter, it remains vulnerable to injection and reflected XSS.

Ultimately I think we're going to have to fundamentally change the way the "Exit Form" button works to resolve this. Remind me to bring this issue up at our next security sync meeting I think it merits discussion.