Additional nodes can be added to XML without breaking signature

Description

An outside tester demonstrated that a similar attack to the DTD issue allows comments to be inserted without breaking a signature if the c14n method excludes them. This similarly corrupts the text content surfaced by the library.

The "simple" fix for this would appear to be to ignore comments with the parser, which happens to be the default behavior of the Java library. The impact would likely be twofold:

  • serialization of metadata for backups would strip comments

  • metadata with comments signed using c14n methods that include them would not verify

We presume the latter has not been a practice we see with the IdP, so presumably wouldn't be common with the SP.

Environment

None

Activity

Scott Cantor 
March 9, 2018 at 4:50 PM

The advisory has been updated with the link to the commit.

Unidentified Legacy Account 
March 9, 2018 at 7:09 AM
(edited)

Unidentified Legacy Account 
March 9, 2018 at 6:25 AM
(edited)

Hi Scott,

Can you provide me the link of the correct patch from the branch?

I am not able to find it

Thanks in advance

Scott Cantor 
March 5, 2018 at 2:37 PM

You copied the wrong patch, that's from master, not the branch. And you would seemingly either have failed to patch the parser pool source file, or you are on such an old version of Xerces that you are dead anyway. Anybody using xmltooling code that old is screwed regardless.

Unidentified Legacy Account 
March 5, 2018 at 7:26 AM
(edited)

Hi, We have back ported the patch from this link https://git.shibboleth.net/view/?p=cpp-xmltooling.git;a=commitdiff;h=74ec6fa833f46a84486a97c491e391fb8c48f1ea;hp=a9ed2105e13b1923a2bef791d55654460e6d7161 to our XMLtooling version 1.2.2 which is used in SAML Assertion parser.

 

The behaviour now we are seeing is inserting a XML Comment is throwing exception "Unmarshaller found unsupported node type" . Is the behaviour correct ?  

I was expecting the changes to basically ignore the comment and combine the previous text and subsequent text together.

Can you please clarify?

Fixed

Details

Assignee

Reporter

Components

Fix versions

Created January 26, 2018 at 1:08 AM
Updated June 24, 2021 at 1:21 PM
Resolved February 27, 2018 at 12:37 AM