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

Fix for CVE-2017-11428 does not normalize text before returning it #445

Closed
Umofomia opened this issue Feb 28, 2018 · 4 comments
Closed

Fix for CVE-2017-11428 does not normalize text before returning it #445

Umofomia opened this issue Feb 28, 2018 · 4 comments

Comments

@Umofomia
Copy link
Contributor

Umofomia commented Feb 28, 2018

The fix for CVE-2017-11428 essentially replaced the call to element.text with element.texts.join. However, the behavior of text differs from texts in the following way (emphasis mine):

This method returns the value of the first text child node, which ignores the raw setting, so always returns normalized text.
https://ruby-doc.org/stdlib-2.3.0/libdoc/rexml/rdoc/REXML/Element.html#method-i-text

texts does not normalize the text before returning it. This primarily breaks cases where the raw text contains elements that are escaped. Here's an example of the main difference:

REXML::Document.new('<foo>the pen &gt; the sword</foo>').elements.first.text
# => "the pen > the sword"
REXML::Document.new('<foo>the pen &gt; the sword</foo>').elements.first.texts.join
# => "the pen &gt; the sword"

Proposed fix

Since REXML::Element#text calls REXML::Text#value to normalize the text value, change the call to elements.texts.join with elements.texts.map(&:value).join to normalize the text before returning it:

REXML::Document.new('<foo>the pen &gt; the sword</foo>').elements.first.texts.map(&:value).join
# => "the pen > the sword"

I can look into creating a pull request for this fix if you agree.

@Umofomia
Copy link
Contributor Author

I ended up creating pull request #446 to address this. If you decide it looks good once you review it, can a new version be released that has this fix? It's currently blocking our ability to deploy the fix for CVE-2017-11428. Thanks!

@Umofomia
Copy link
Contributor Author

I noticed you also backported the fix for CVE-2017-11428 to prior versions of ruby-saml too, so you may want to consider backporting this fix to those versions too.

@pitbulk
Copy link
Collaborator

pitbulk commented Feb 28, 2018

Hi, thanks for that research. We gonna review it and see if that change have any other implications and merge it asap when validated.

@pitbulk
Copy link
Collaborator

pitbulk commented Feb 28, 2018

Version 1.7.2 is live, thank for your research and PR

shyam-habarakada pushed a commit to 10Kft/ruby-saml that referenced this issue May 24, 2018
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

No branches or pull requests

2 participants