Use OWASP Encoder for XSS attacks

Hi, I am a newbie to openmrs and would like to contribute. I found this [1] at issue-tracker. The org.apache.commons.lang.StringEscapeUtils is also a solution for this. But there are some issues which are introduce in [1] when used this.

OWASP Java Encoder [2] is recently used widely to prevent XSS attacks. I am implementing it in openmrs at the moment to give it a try. This has worked for in my work so far.

Will it be a problem if I introduce new package in to code? Any known facts, information and advices are welcome on this.

[1] https://issues.openmrs.org/browse/REPORT-492 [2] http://owasp-java-encoder.googlecode.com/svn/tags/1.1/core/apidocs/org/owasp/encoder/Encode.html

Thanks. Regards Thisura Philips

3 Likes

I found out that there is the org.openmrs.web.WebUtil class to help with HTML encoding for jsps. But the escapeHtml method is the only one related to the context.

If we have the intention of keeping everything within our code, we can implement the effective methods in WebUtil.

But considering the fact that OWASP Encoder is opensource, isn’t it better to move on to OWASP Encoder. What is preffered?

Thanks Regards ttcphilips

Awesome! Thanks so much for looking into this.

We would very much welcome a pull request to demonstrate to us what you’re thinking.

Can you point us to it when you’ve taken a crack at this?

@ttcphilips thanks for looking at this!

The approach that I would take is to:

  1. Add the OWASP encoder to our codebase
  2. Rewrite the relevant WebUtil methods to delegate to OWASP’s version
  3. Assuming that the OWASP encoder’s methods have convenient and intuitive method signatures, I would deprecate our WebUtil methods and point to the OWASP versions. But if those methods aren’t intuitive, we can have “easier” versions of them in WebUtil that delegate.

@paul @darius Thank you very much for your advice.

I have sent a PR for reporting module at [1]. But it was before getting the advices from you guys. You can review it and let me know if any thing is wrong with that. Also you can update the jira [2]

I will rewrite the relevant WebUtil.escapeHtml method to delegate to OWASP’s version. Also I will add the other methods delegating to the OWASP’s version.

I think OWASP encoder’s method signatures are convenient and intuitive. But I am just a newbie. :smile: So we can discuss. FYI the Api for OWASP Encoder is at [3]

I am thinking about adding the same functionalities for a javascript module to use to encode JavaScript variables more efficiently and contextually. How about that?

[1] https://github.com/openmrs/openmrs-module-reporting/pull/99 [2] https://issues.openmrs.org/browse/REPORT-492 [3] http://owasp-java-encoder.googlecode.com/svn/tags/1.1/core/apidocs/org/owasp/encoder/Encode.html

Thanks Regards ttcphilips

Hi Paul, Darius and Devs,

I have sent a PR at [1] for openmrs-core, introducing contextual encoding methods in WebUtil class, delegating to OWASP encoder.

The PR at [2] was sent before implementing [1]. If [1] is ok to be merged, then we can use those methods in code. But as @darius suggested, this is needed only if OWASP encoder’s methods are not that much convenient and intuitive. I think [2] is a good example using reporting module to elaborate the usage of OWASP Encoder methods in openmrs code.

Based on the above decision, we can move on with OWASP Encoder methods or WebUtil methods to prevent XSS attacks in openmrs.

[1] https://github.com/openmrs/openmrs-core/pull/1643 [2] https://github.com/openmrs/openmrs-module-reporting/pull/106

Thanks Regards ttcphilips

Good work @ttcphilips!!!

Thank you very much @r0bby!!!

1 Like

Hi all,

Since [1] is merged, shall we update the wiki at [2]? Or is there any other relevant wikipage for this?

[1] https://github.com/openmrs/openmrs-core/pull/1643 [2] https://wiki.openmrs.org/x/MxEz#CodingConventions-Security

Hi darius, I have added the OWASP encoder to our codebase. I have introduced new methods in WebUtil [1] methods to delegate to OWASP’s version. Now anyone can use those methods to encode the variables contextually.

I am wondering whether we have to update the Wiki page. Is [2] needed to be updated or are there any other suitable place?

[1] https://github.com/openmrs/openmrs-core/pull/1643 [2] https://wiki.openmrs.org/display/docs/Coding+Conventions#CodingConventions-Security Thanks.

Yes go ahead and update the wiki. If you mention the WebUtil methods, then you also need to mention that they are available from platform 2.0 and above. Else one would need to include the owasp jars.

I feel explaining all the methods in the [1] is not appropriate. Is there any particular wiki where WebUtil methods are explained or shall I create new one?

[1] https://wiki.openmrs.org/display/docs/Coding+Conventions#CodingConventions-Security

I feel that you do not have to explain all. Just a few, and refer to the class for more.

I updated the Wiki. Shall we deprecate the method escapeHtml() method in there or we keep it as it is? This method is named as escapeHtml to preserve backward compatibility. But I hope there is no issue of keeping that name. At the same time there may be a slight misunderstanding with the name being different from others while the functionality is delegated to OWASP Encoder same like other “encodeForSomeContext” type methods.

What do you think?

If we switched from owasp to something else, would you keep the name escapeHtml? :smile:

:smile: It has to be changed someday. Shall I be the one who is doing that. :smile: I am learning a lot and enjoying it with you all. :smile:

Is our public name dependent on which internal library we use? :smile:

No not at all. That means we don’t have to change that name. :open_mouth: :smile: