Bug 40897

Summary: String comparisons using '==' causes validation errors with some parsers
Product: Security - Now in JIRA Reporter: Vishal Mahajan <vmahajan>
Component: SignatureAssignee: XML Security Developers Mailing List <security-dev>
Status: RESOLVED FIXED    
Severity: blocker CC: ashundi, iyappanr, jason.halpin, robertegglestone, sssivasankari
Priority: P1    
Version: unspecified   
Target Milestone: ---   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 45637    
Attachments: First draft for a pluggable string checker.

Description Vishal Mahajan 2006-11-05 07:06:11 UTC
There has already been discussion on this issue on the project mailing list.
here's the email thread:

----------------------------------------------------------------------------
Hi Sean,

The penalty hit is taken when the strings are not equal, sadly of the
same length.
And have a lot of common begging characters. That is sadly a common
problem with namespaces URI, they are more or less equal in length and
have a lot of damn http://.../... or urn:....: whatever at the
begining. And that is why Xerces and other DOM implementations  intern
namespaces URI.

I have profile and it takes a lot of time.
My point is that all the parsers I know do the intern (or it did when
I do the implementation). And this is an old commit 8 months old(it is
true that it is not yet on a official release), and it takes a
measurable hit if not use in small messages(the kind of one that are
in xml protocols).

So I will first check other options (change the configuration of the
offending parser with a
feature[http://xerces.apache.org/xerces2-j/features.html] ).
If it does not work I will change from == to equals, but I will let
this as last resort.

On 10/5/06, Sean Mullan <Sean.Mullan@sun.com> wrote:
> String.equals will work for both interned and non-interned Strings,
> since it first checks if they are a reference to the same object. So
> using String.equals seems safer and should be comparable performance I
> would think. But maybe I'm missing something?
>
> --Sean
>
> Vishal Mahajan wrote:
> > Do others also have views on this discussion?
> >
> > Thanks,
> > Vishal
> >
> > Vishal Mahajan wrote:
> >> Hi Raul,
> >>
> >> The parser that I am working with clearly doesn't intern element
> >> namespace strings which is the reason I ran into this problem. And
> >> actually I am not sure whether it's a good idea for a parser to intern
> >> element namespace strings given that there could be huge number of
> >> elements being parsed and there's a potential risk of running out of
> >> memory. Also you mention that xerces might be interning namespace
> >> stings but looking at their code I was unable to find that. Can you
> >> point me to the relevant piece of code?
> >>
> >> Thanks,
> >>
> >> Vishal
> >>
> >> Raul Benito wrote:
> >>> Vishal the problem is that this codes is called gazillion of times,
> >>> and even it
> >>> seems a small thing, it takes a lot of accumulated time, I even think
> >>> in remove this checking altogether or control it by a property.
> >>> Perhaps there is a feature set in your DOM parser that interns the
> >>> namespaces. I have test with several DOM parsers (xerces, xmlbeans,
> >>> jaxb) and all of them the namespaces strings are interns.
> >>> If you are not able too toggle the behavior, We can begin to think in
> >>> other possibilities (create code on the fly, create an interface with
> >>> one implementation or the other a let the JVM inline it). But I think
> >>> will be the last resort.
> >>>
> >>> Regards,
> >>> Raul
> >>>
> >>> On 10/2/06, Vishal Mahajan <vmahajan@amberpoint.com> wrote:
> >>>> Any signature verification was failing for me, and I have a different
> >>>> DOM implementation in my environment, so probably you are right. It was
> >>>> such a basic error that it had to be something like this. In any
> >>>> case, I
> >>>> think we should keep string comparison safe.
> >>>>
> >>>> Vishal
> >>>>
> >>>> Raul Benito wrote:
> >>>> > Hi Vishal,
> >>>> >
> >>>> > The namespaces strings are intern, at least in xerces.
> >>>> >
> >>>> > Can you post the code that is failing?
> >>>> >
> >>>> > On 10/2/06, Vishal Mahajan <vmahajan@amberpoint.com> wrote:
> >>>> >> This problem was not allowing successful creation of signature space
> >>>> >> elements. Fix attached.
> >>>> >>
> >>>> >> Vishal
----------------------------------------------------------------------------
Comment 1 Raul Benito 2008-01-01 08:49:49 UTC
Created attachment 21336 [details]
First draft for a pluggable string checker.

Please take a look a the patch, I want feed back on the solution.Also I don't
have anyother DOM parser that don't intern namespaces, this should fix the
problem, but perhaps there are more places where this problem arise.
Comment 2 Robert Egglestone 2008-03-10 16:18:25 UTC
I've run into this problem while running under WebLogic (the same app runs fine on Tomcat).

Would it be possible to do a quick == comparison, and then only do .equals if it fails?

Would interning all namespaces mean that an attacker could cause the server to run out of memory by sending requests which contain lots of random namespaces?
Comment 3 Anli Shundi 2008-03-10 22:22:49 UTC
Can this be a case of "the flawed microbenchmark".  (google it).  .equals is slower but on the whole operation (signing, encryption, etc) the XML time is mostly spent on parsing and serialization I would guess.  And I'm skipping the cryptographic primitives...

I wonder what the test case is.
Comment 4 Raul Benito 2008-05-09 08:00:53 UTC
(In reply to comment #3)
> Can this be a case of "the flawed microbenchmark".  (google it).  .equals is
> slower but on the whole operation (signing, encryption, etc) the XML time is
> mostly spent on parsing and serialization I would guess.  And I'm skipping the
> cryptographic primitives...
> 
> I wonder what the test case is.
> 

I have do a lot of benchamark and really this is a huge improve. I can tell you why but it will be a big post. And I have do my exercises, and i benchmark the whole operation. Anyway 2 releases ago it was plain impossible to use another parser than xerces (and really a concrete one), we have improve it til the way that now, only this is possible and works with different ones that intern namespaces. 

Anyway i think that sadly for a fully spec compliant there is no other options than a special xalan+xerces (for xpath transformations).

But I still that if a parser doesn't intern namespaces will be hit by a lot of just a difference in the end, that happens with versioning in xml. I just create the patch in order to disable the NS comparison.


 
Comment 5 sean.mullan 2008-08-15 10:19:47 UTC
*** Bug 45573 has been marked as a duplicate of this bug. ***
Comment 6 Chad La Joie 2010-10-16 06:56:35 UTC
*** Bug 46681 has been marked as a duplicate of this bug. ***
Comment 7 Chad La Joie 2010-10-16 07:03:50 UTC
In rev 1023243 I committed an update that I believe catches all the places that
== is used for string comparisons and replaces it with equals().  I also
deprecated org.apache.xml.security.utils.ElementChecker and
org.apache.xml.security.utils.ElementCheckerImpl which were used to attempt to
hide the difference between equals() and ==

This should resolve this issues unless I missed an occurrence.  It would be
really helpful if those individuals who commented on this issues who are not
using Xerces could run unit test
org.apache.xml.security.test.c14n.implementations.Canonicalizer20010315ExclusiveTest
and let us know if this issue isn't resolved.
Comment 8 coheigea 2010-10-17 14:54:16 UTC
*** Bug 48681 has been marked as a duplicate of this bug. ***
Comment 9 coheigea 2010-10-17 14:55:21 UTC
*** Bug 44874 has been marked as a duplicate of this bug. ***