Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add c14n support #226

Closed
tenderlove opened this issue Feb 7, 2010 · 35 comments · Fixed by #563
Closed

Add c14n support #226

tenderlove opened this issue Feb 7, 2010 · 35 comments · Fixed by #563
Milestone

Comments

@tenderlove
Copy link
Member

Let's add c14n support!

http://xmlsoft.org/html/libxml-c14n.html

@RichGuk
Copy link

RichGuk commented Feb 13, 2010

I was about to ask for this :D Yes please! =)

@tenderlove
Copy link
Member Author

I'm moving this to 1.4.3. We have a bunch of stuff that should be released.

@whazzmaster
Copy link

I would desperately love this to be able to do xml digital signature validation.

@gkellogg
Copy link

gkellogg commented Jul 8, 2010

I require this for doing proper XMLLiteral normalization, which requires a lexical form of Exclusive Canonical XML.

Note work that was done to incorporate it into libxml-ruby here: http://rubygems.org/gems/coupa-libxml-ruby

@tenderlove
Copy link
Member Author

I have a branch with c14n incorporated:

http://github.com/tenderlove/nokogiri/tree/c14n

I don't feel that it's ready for mass consumption yet, so we're moving it to the 1.4.4 release. I'll merge it to the 1.4 branch after we've released 1.4.3.

@whazzmaster
Copy link

I looked through the 1.4.4 docs and didn't see (though I may have missed) the c14n support there- was this pushed back to 1.5?

@amiel
Copy link

amiel commented Feb 9, 2011

I would just like to note that I would love this feature too!

What's the current status?

@alexcrichton
Copy link

Is there a reason this hadn't been merged in yet? This would be awesome if it were in Nokogiri.

I tried out the c14n branch and it mostly worked for me. When I specified a block to reject elements, ruby always segfaulted somewhere in the iterations (on both 1.8.7 and 1.9.2). I got around this though by not filtering.

@tenderlove
Copy link
Member Author

@alexcrichton can you provide an example? I'd like to keep nokogiri from segv'ing before merging this to master.

@alexcrichton
Copy link

Sure, here's a small script

require 'nokogiri'

xml = <<-XML
<saml2p:Response ID="_b20d81391d42ff19ff59341a2f9400f2" xmlns:saml2p="urn:oasis:names:tc:SAML:2.0:protocol">
<saml2:EncryptedAssertion xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion">
  <xenc:EncryptedData Id="_8a91a629c4b83b142912a639714839ec" xmlns:xenc="http://www.w3.org/2001/04/xmlenc#">
    <ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
      <xenc:EncryptedKey xmlns:xenc="http://www.w3.org/2001/04/xmlenc#">
        <xenc:EncryptionMethod xmlns:xenc="http://www.w3.org/2001/04/xmlenc#">
        </xenc:EncryptionMethod>
      </xenc:EncryptedKey>
    </ds:KeyInfo>
  </xenc:EncryptedData>
</saml2:EncryptedAssertion>
</saml2p:Response>
XML

Nokogiri::XML(xml).canonicalize{ |a, b| true }

I confirmed that it faulted on 1.9.2 and 1.8.7 using the head of the c14n branch.

Also, If I change the XML to just be <root></root>, the canonicalization works on both versions.

@alexcrichton
Copy link

If it helps, I also did a bit of digging but all I could come up with was that this pointer was something wildly invalid, causing the segfault.

The xmlC14NExecute method called the block_caller function a few times and then just decided to kick the bucket on the first call to Nokogiri_wrap_xml_node at a random point

@amiel
Copy link

amiel commented Mar 16, 2011

I already "upvoted" this, but I wanted to note here that I would also find this very useful.

I'm working on adding Soap WSSE signatures to savon (in https://github.com/carnesmedia/savon). The ruby XMLCanonicalizer library did not work, so I'm stuck using xmlstarlet via the command line...

Thank you for your time!

@amiel
Copy link

amiel commented Mar 29, 2011

@cschneid
Copy link

Has there been any progress on this since March? Is this code reasonably up to date, and workable to test out, or has it fallen behind master a bunch?

@binaryape
Copy link

I'm also eager to use C14n to process SAML - this feature would be very useful.

@cosine
Copy link

cosine commented Sep 27, 2011

I started a debugging effort to fix this about a week ago. I've been constrained on time since, so I do not know when I will have time to get back to fixing this properly. My temporary workaround is to fork/exec out to xmlstarlet to do the c14n transformations (amiel's idea), so my priority on this has decreased a little. I really have to finish the rest of a WS-Security implementation before I have time to revisit this part.

If you want to help fix this bug rightly then first note that previous comments tell the tale that the bug comes up when namespaces are used. This is a memory corruption bug, so it could be a buffer overflow or use-after-free situation. In particular, when namespaces are used, attempts to traverse up the class hierarchy of a Nokogiri::XML::Node object reveals corrupted data, causing the segmentation fault. This traversal happens automatically in a lot of situations, but I bet a good portion of the underlying C-structure is getting whacked by something.

@cosine
Copy link

cosine commented Sep 28, 2011

I looked at this more yesterday. On the surface it really looks like a use-after-free, but I think the problem is an order of magnitude more subtle. I've been instrumenting mallocs and frees and have not come up with evidence that memory supposedly being re-issued by malloc was ever issued in the first place (many ways this could escape me, however). In any case, we're ultimately still looking at memory corruption.

The bug probably exists presently in Nokogiri without c14n, but is is likely only exposed when trying to do some deep tree namespace processing like that required by c14n.

My next steps when I get back to this is to run Valgrind on it to see if any more insights pop out. I do not presently have an environment with both Valgrind and my debugging Nokogiri setup, so I have a bit of setup to create one before I can do this.

@cosine
Copy link

cosine commented Nov 17, 2011

If this pull request does everything it says it does, then I would love to see it accepted. In addition to fixing the crash bug, it adds the ability to select which canonicalization method (e.g. exclusive, with/without comments), too. I give a +1 for that. Please let me know if I can help out. I do plan to test it in my environment soon, and will share my results if no one else chimes in on it.

@nikosd
Copy link

nikosd commented Apr 11, 2012

Shouldn't this be closed? If I understand it correctly this is already integrated in 1.5.2 (https://github.com/tenderlove/nokogiri/blob/v1.5.2/lib/nokogiri/xml/node.rb).

P.S.: Thanks all for your work on this!

@matthauck
Copy link

It doesn't look like c14n has been supported yet. For example in SaveContextVisitor, the c14nExclusiveInclusivePrefixes list gets set with given inclusive namespace, but it is not referenced any other time throughout the file, in particular, it is not applied in getAttrsAndNamespaces()

@matthauck
Copy link

I should add: I really hope nokogiri can get this fully implemented soon since I need to add saml support for a project I'm on and love nokogiri and would hate to have to add another xml library just for canonicalization...

@matthauck
Copy link

One more note: I am also getting unnecessary parent attributes / namespaces added into my canonicalized section as well as duplicated attributes:

<ds:SignedInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" Destination="..." ID="_5968e6c72142cdc8018b3afae5329dc2b8dfb73b12" IssueInstant="2012-09-12T07:58:13Z" Version="2.0"><ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#" Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#">&lt;/ds:CanonicalizationMethod>
...

@yokolet
Copy link
Member

yokolet commented Sep 13, 2012

@matthauck Are you using JRuby version? C14N for pure Java version is still experimental and needs much improvement.

If you have a simple test case that exhibits the problem, I'm happy to add that to Nokogiri test.

@matthauck
Copy link

Ah, okay. Yes, I am on jruby. I will try to add a test case when I get the time

@nikosd
Copy link

nikosd commented Nov 19, 2012

@matthauck what's your approach with xml signing and saml? I'm working on https://github.com/rsaml/rsaml and it's one of the missing pieces of the puzzle.

@matthauck
Copy link

Since I'm on jruby, I ended up cheating a bit and taking advantage of java's libraries for XML signing/verifying: http://docs.oracle.com/javase/6/docs/technotes/guides/security/xmldsig/XMLDigitalSignature.html

@nikosd
Copy link

nikosd commented Nov 19, 2012

I'm also considering this solution since a don't see a light in the end of the tunnel... 😒

@flavorjones
Copy link
Member

@matthauck That's disappointing. A test case demonstrating what you saw as deficient would have been extremely helpful.

@matthauck
Copy link

Constrained by project deadlines, I didn't have time to wait around for it to be supported... Sorry. I'll try to get around to the test case still, but life's a bit busy and can't promise anything...

@flavorjones
Copy link
Member

@matthauck I understand completely.

@matthauck
Copy link

Okay, started to take a look at this tonight, and it looks like there is already test cases in there that are known to fail on jruby--not sure you need any input from me. I found this in test_c14n.rb:

skip("C14N Exclusive implementation will complete by next version after 1.5.1") if Nokogiri.jruby?

The test does indeed fail in jruby while passing fine in cruby. Looks like this was forgotten about since nokogiri is already past 1.5.1... Exclusive canonicalization is indeed what I was after, which would explain why it doesn't work.

@nikosd
Copy link

nikosd commented Nov 22, 2012

To anyone interested, for the signing/verification part of SAML (not the c14n) I went through the ugly hack of passing system commands to the cli version of libxmlsec. Hopefully I will try to approach it with a ruby wrapper around the actual lib but since this is my first attempt on such a thing no acceptable patch can be expected in the foreseeable future 😄

@uday-rayala
Copy link

Hi,

Just wanted to know about the status of this issue. Has this issue being prioritised for any upcoming release?

Thanks,
Uday.

@yokolet
Copy link
Member

yokolet commented Aug 19, 2013

Status of C14N for JRuby is work in progress so far, and is under c14n4j branch. Other than block parameter thing, all tests passed on that branch. But, it's been a while. Merging master may trigger more work.

@uday-rayala
Copy link

Thanks for the update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.