search

robrichards / xmlseclibs

A PHP library for XML Security
BSD 3-Clause "New" or "Revised" License
387 stars 181 forks source link

Include InclusiveNamespaces specified in CanonicalizationMethod when canonicalizing SignedInfo #178

Open yamitenshi opened 5 years ago

yamitenshi commented 5 years ago

The current implementation ignores inclusive namespaces when canonicalizing the SignedInfo element. This pull request ensures that an InclusiveNamespaces element within the topmost CanonicalizationMethod element is also parsed and included in the SignedInfo canonicalization, preventing signature verification issues when the canonicalized XML would differ as a result.

This may fix #98, but I'm not 100% sure of that.

robrichards commented 5 years ago

@yamitenshi I am probably going to refactor this to a private method that processTransforms() can also call

yamitenshi commented 5 years ago

I could add that to this PR - it's a good refactor, since my changes do introduce quite a bit of duplicate code. What do you have in mind? Maybe a method that returns a list of inclusive namespace prefixes given a root node to search from?

wildspitze commented 5 years ago

I had the same requirement and needed to consider the InclusiveNamespaces in the CanonicalizationMethod. So I use @yamitenshi function canonicalizeSignedInfo(), which works well and verifies the signature successfully. Would appreciate if the adjustments would be merged into the master-branch.

yamitenshi commented 5 years ago

Oh <bad word I probably shouldn't use in a semi-professional setting>, I'd completely forgotten about this one...

I'd love to propose a refactor but I'm afraid I no longer have an XML file that requires this... I'm afraid the XML files I did have were for a previous client of mine and - even if I did still have them - were quite confidential. If you (or someone else, I suppose) could supply an example XML that fails to validate without my fix, I'd be happy to propose a better-refactored solution.

I'll try to come up with something myself, of course, but if there's an example ready, that'd simplify the whole thing tremendously.

yamitenshi commented 5 years ago

Hang on, the issue I linked (#98) probably has one... Will likely be proposing a refactor in the coming days