Sanitize HTML in legacyui error div to avoid XSS- EMPT 54 #136
Conversation
| <spring:bind path="implId.implementationId"> | ||
| <td style="white-space: nowrap"><openmrs:message code="ImplementationId.implementationId"/><span class="required">*</span></td> | ||
| <td style="white-space: nowrap"> | ||
| <input type="text" value="${status.value}" name="${status.expression}" maxlength="20" size="8"/> |
There was a problem hiding this comment.
do we expect cross site scripting in such cases,ie on the input tag
There was a problem hiding this comment.
If the input field is loaded in with data from the server initially. So if a script is stored on the server, it could be executed when loaded. That was my thought process, but it could be incorrect. What do you think @isears?
There was a problem hiding this comment.
Are the patches in implementationIdForm.jsp still related to EMPT54? So far I've only been able to trigger XSS in the error messages that come up in headerFull.jsp but maybe I'm missing something.
| </c:if> | ||
| <c:if test="${err != null}"> | ||
| <div id="openmrs_error"><openmrs:message code="${err}" text="${err}" arguments="${errArgs}" htmlEscape="false"/></div> | ||
| <div id="openmrs_error"><openmrs:message code="${err}" text="${err}" arguments="${errArgs}" htmlEscape="true"/></div> |
There was a problem hiding this comment.
@dkayiwa do you know why the htmlEscaping here was set to "false" originally? Can you think of anything in the legacy UI that would break by turning it on?
There was a problem hiding this comment.
@jmmacdo4 unfortunately, there's at least one page that uses HTML tags in error messages (example). So turning on htmlEscaping in this header will break the error-display UI there and probably a few other places.
I think the best approach for EMPT54 is to patch in the controller by using a java html-escaping function to sanitize the error message when it gets passed to the response here.
The alternative is to comb through every call to setAttribute(WebConstants.OPENMRS_ERROR_ATTR, "some-error-msg");, check to see if it's legitimately using HTML to format the error message, and then refactor it to not use that HTML somehow.
Feel free to reach out if you would like to discuss further. This issue may also be a good topic for next Friday's sync.
| <input type="text" name="${status.expression}" value="${status.value}" size="40" /> | ||
| <c:if test="${status.errorMessage != ''}"> | ||
| <span class="error">${status.errorMessage}</span> | ||
| <span class="error">$<c:out value="${status.errorMessage}"/></span> |
There was a problem hiding this comment.
Was the $ character before the <c:out tag on this line left in on purpose?
There was a problem hiding this comment.
No, thanks for pointing that out, I will fix that on my next push.
| </c:if> | ||
| <c:if test="${err != null}"> | ||
| <div id="openmrs_error"><openmrs:message code="${err}" text="${err}" arguments="${errArgs}" htmlEscape="false"/></div> | ||
| <div id="openmrs_error"><openmrs:message code="${err}" text="${err}" arguments="${errArgs}" htmlEscape="true"/></div> |
There was a problem hiding this comment.
@jmmacdo4 unfortunately, there's at least one page that uses HTML tags in error messages (example). So turning on htmlEscaping in this header will break the error-display UI there and probably a few other places.
I think the best approach for EMPT54 is to patch in the controller by using a java html-escaping function to sanitize the error message when it gets passed to the response here.
The alternative is to comb through every call to setAttribute(WebConstants.OPENMRS_ERROR_ATTR, "some-error-msg");, check to see if it's legitimately using HTML to format the error message, and then refactor it to not use that HTML somehow.
Feel free to reach out if you would like to discuss further. This issue may also be a good topic for next Friday's sync.
| <spring:bind path="implId.implementationId"> | ||
| <td style="white-space: nowrap"><openmrs:message code="ImplementationId.implementationId"/><span class="required">*</span></td> | ||
| <td style="white-space: nowrap"> | ||
| <input type="text" value="${status.value}" name="${status.expression}" maxlength="20" size="8"/> |
There was a problem hiding this comment.
Are the patches in implementationIdForm.jsp still related to EMPT54? So far I've only been able to trigger XSS in the error messages that come up in headerFull.jsp but maybe I'm missing something.
|
@isears I have updated the sanitization to be present in the controller. On this note, I think it would be really easy to CTRL-F |
@jmmacdo4 I'm warming up to the idea. It's possible that the example I cited above is the only thing you have to refactor. If that's the case, what's your workaround for formatting that specific exception without using HTML tags? |
|
@isears I have went ahead with the error div "redesign" we talked about. I have replaced all of the |
|
Going to close this in favor of #149 |
|
This PR has been restructured in #151 |
By adding cout to the error spans as well as adding htmlEscaping to the error div, I was able to remove the possibility for a cross site scripting attack on the implementation ID, Name, Passphrase, and Description fields.
@isears