Re: Problems with exclusive c14n and default namespaces

[Raul Benito]

Samuel Meder wrote:
We just upgraded to the xmlsec 1.2 RC1 release and started noticing
problems with the exclusive c14n implementation. In particular SAML
assertion signature validation started to fail after the assertion was
sent over the wire (using axis).
 

First of all thanks for the bug report. You have hit the code that is 
not very well exercised by the test suite, so thanks a lot. Second you 
are using the slow path (i.e. using xpath transformations) and that's good.

The assertion when initially generated declared several redundant
default namespaces on the elements which Axis subsequently removed when
sending the actual message. Furthermore, the code lists #default in the
inclusive namespaces, so as far as I can tell this should make it ok for
axis to remove redundant NS declarations.
 

The #default namespace means treat the xmlns prefix as it was done in 
the inclusive c14n

Now in particular, I'm seeing the following problems with the code:
* If the element being processed lacks a default ns declaration and does
not use a prefix then xmlns="" is added unconditionally even if the
element is declared in the default namespace and the immediate parent
element defines a default NS.
 

That is the way is defined if xmlns is added in inclusive(and that's 
thing seem to be stressed by the tests).

* CanonicalizerBase.java has the following code:
        Element currentElement = (Element) currentNode;
         OutputStream writer=this._writer;
        String tagName=currentElement.getTagName();
        if (currentNodeIsVisible) {
           //This is an outputNode.
                ns.outputNodePush();
           writer.write('<');
           writeStringToUtf8(tagName,writer);
        } else {
          //Not an outputNode.
                ns.push();      
        }
        // we output all Attrs which are available
        Iterator attrs = handleAttributes(currentElement,ns);
        while (attrs.hasNext()) {
           Attr attr = (Attr) attrs.next();
           outputAttrToWriter(attr.getNodeName(), attr.getNodeValue(), writer);
        }
i.e. attributes are printed even if the element is not visible. This
seems to result in spurious xmlns="" being output in random places.
Maybe this code:
 

That's needed for rare xpath transformations but you have hit a bug 
because it should only output when the xpath select the xmlns but not in 
when it is added as inclusive(but i need to check with the spec to be sure).

                        Iterator it=this._inclusiveNSSet.iterator();
                        while (it.hasNext()) {
                                String s=(String)it.next();
                                Attr key=ns.getMappingWithoutRendered(s);
                                if (key==null) {
                                        continue;
                                }
                                result.add(key);
                        }
in Canonicalizer20010315Excl.java and the fact that the empty default
map seems to be added to ns (why is that done btw? Seems wrong to me) by
default has something to do with it?
 

The #default in inclusive prefix makes to do this behavior, as expected.
The inclusive code seems to behave a little better (not seeing
validation failures anymore), but I am a bit skeptical about the
following:
   if (isRealVisible) {    	           
   	//The element is visible, handle the xmlns definition        
       Attr xmlns = E.getAttributeNodeNS(Constants.NamespaceSpecNS, "xmlns");
       Node n=null;
       if (xmlns == null) {
       	//No xmlns def just get the already defined.
       	n=ns.getMapping(XMLNS);        		
       } else if ( !this._xpathNodeSet.contains(xmlns)) {
       	//There is a definition but the xmlns is not selected by the xpath.
       	//then xmlns=""
       	n=ns.addMappingAndRenderXNodeSet(XMLNS,"",nullNode,true);        	    		      	
       }
       //output the xmlns def if needed.
       if (n!=null) {
   			result.add(n);
   	}
       //Float all xml:* attributes of the unselected parent elements to this one. 
   	addXmlAttributes(E,result);
   }

Shouldn't the xmlns="" mapping only be added if this mapping is in scope
and the ancestor element declaring the mapping is not visible (and since
NameSpaceSymbolTable always adds that mapping...). Also I'm not quite
sure that I understand the "else if":
CanonicalizerBase:canonicalizeXPathNodeSet seems to throw a exception if
it finds a attribute node in the _xpathNodeSet which would lead me to
assume that the node set would never contain the xmlns node?
Might also be that I am completely misunderstanding things.
Canonicalization makes my brain hurt...
 

Yes it is really difficult,and  now that I have arrived home I will try 
to fix it, thanks for your report(but try to don't use the xpath code, 
it will be good for your performance).

Thanks again,
Regards,
Raul