Thank you for writing this up Harmeet. Before getting into the details
it is important that you understand that you are proposing to change
something, or in fact basically get rid of something, that was
discussed in a lot of detail and that has seen effort from quite a few
people. In general when recommending to change something it's a good
idea to understand where that came from and why things are done the
way they are. Based on what you've written in this and other messages
it does not seem that you have done this research. I might be wrong,
and if so I apologize, but in order to continue with a discussion like
this it is important that you know the background and understand the
problem and the reasons for the solutions.
There is a good thread that discusses most of this, and it is
available here:
http://www.nabble.com/Security-Issues-td21622188.html
On Jun 22, 2009, at 11:31 AM, Harmeet Bedi wrote:
A issue(https://issues.apache.org/jira/browse/OFBIZ-2645) is that
default value of allow-html is very restrictive.
This is as per start with most constrained best practice in security
Default is allow-html="none"
It not only does not allow html but it also does not allow simple
text like "Tom's age is likely > Paul's age". '>' breaks.
I agree this could be improved, but the trick is how do we tell the
difference? One interesting page that shows just how difficult it is
to recognize HTML is this one which is oriented around finding hacks
related to this very problem in various browsers:
http://ha.ckers.org/xss.html
What you have mentioned is definitely a weakness in the current code,
which is admittedly fairly simple, and an opportunity to improve that
code. I don't think it would be a good idea to change the overall
approach and throw away the attempt to filter incoming text,
especially text coming in from untrusted sources.
Around the time this was written I asked for recommendations from
anyone on the mailing list that might be more familiar with this
topic, especially as it relates to what a browser will or will not
interpret as markup. My limited knowledge on the topic is that you
pretty much have to have something that looks like a tag, in other
words starts with a less than or open angle bracket and has some sort
of word after it, in order for the browser to consider it markup and
try to interpret it.
One thing that might work is to allow a > if there is no <. I'm not
sure about the other way around because if there is otherwise valid
HTML a browser might not care about a missing > to close a tag. It may
also be valid to allow a < if there is a space after it, but I'm not
sure about that either.
Please keep in mind that the reason for this is to not allow abuse of
a production system. If that is not important to you or to your
clients then you can certainly disable it globally. However, I'm
guessing that's not what you would want to do and the best approach
for all of this is to simply improve the code so that it allows as
much as possible while still protecting against the security threat it
is meant to.
allow-html="safe" also does not seem to work well. It does not allow
well formed html.
Could you be more specific? Depending on what you're seeing there may
very well be a bug with the safe HTML code, or just a need to change
the antisamy-esapi.xml configuration file for it. In any case, it
would be good to find out more about what you mean by this because it
sounds like a bug.
Here is a proposal:
HTML and descriptive text with characters like '>' should be
allowable whenever there is a description text input by user.
Change services that deal with description/comments/reason/noteInfo
fields to have allow-html="any"
An example of this is 'updateWorkEffortNote' service. it could
change from
<service name="updateWorkEffortNote" engine="simple"
location="component://workeffort/script/org/ofbiz/
workeffort/workeffort/WorkEffortSimpleServices.xml"
invoke="updateWorkEffortNote" auth="true">
<description>Update a WorkEffort Note</description>
<attribute name="workEffortId" type="String" mode="IN"
optional="false"/>
<attribute name="noteId" type="String" mode="IN"
optional="false"/>
<attribute name="internalNote" type="String" mode="IN"
optional="false"/>
<attribute name="noteInfo" type="String" mode="IN"
optional="true"/>
</service>
to
<service name="updateWorkEffortNote" engine="simple"
location="component://workeffort/script/org/ofbiz/
workeffort/workeffort/WorkEffortSimpleServices.xml"
invoke="updateWorkEffortNote" auth="true">
<description>Update a WorkEffort Note</description>
<attribute name="workEffortId" type="String" mode="IN"
optional="false"/>
<attribute name="noteId" type="String" mode="IN"
optional="false"/>
<attribute name="internalNote" type="String" mode="IN"
optional="false"/>
<attribute name="noteInfo" type="String" mode="IN"
optional="true" allow-html="any"/>
</service>
if this seems acceptable, i can send patches with services for
review and commits.
I mentioned some things related to this in my comments above, but in
general no, my opinion is that we should not open up this security
hole in so many places by default. I would be very interested to hear
what others have to say and how others would like to see this working,
but it may be that they are not talking so much because this has
already been discussed.
In general it is not safe to assume that output filtering will always
take care of this problem, and so when ever text comes from an un-
trusted source, or a potentially un-trusted source, we should filter
the input to avoid the problem right there.
I guess this goes back to what I said at the beginning of this
message. It would be very valuable to put this in the context of what
has already been researched and discussed. What you have written here
would be a lot more meaningful and a lot easier to discuss if you
referred back to the original discussion and decisions that lead to
the functionality you are looking at. What I mean by that is that
anything that has been discussed and decided on can certainly be
changed, but not without referring back to the original discussion and
decisions.
-David
