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

AUTH-255: Sanitize document.signed_element_id via a prepared statement #183

Merged
merged 2 commits into from
Jan 23, 2015

Conversation

Lordnibbler
Copy link
Contributor

Status

READY

Description

Fixes a security vulnerability where OneLogin::RubySaml::Response#xpath_first_from_signed_assertion was vulnerable to arbitrary code execution through use of a maliciously formed SAML response using a Ruby eval() statement.

Todos

  • Cleanup the Gemfile
  • Test that the vulnerability occurs
  • Fix vulnerability by using a sanitized REXML variable

Deploy Notes

Release a new version of Ruby-SAML along with this PR.

Steps to Test or Reproduce

Outline the steps to test or reproduce the PR here.

git pull --prune
git checkout AUTH-255
bundle
rake
# all tests pass!

Impacted Areas in Application

List general components of the application that this PR will affect.

  • OneLogin::RubySaml::Response

@Lordnibbler Lordnibbler force-pushed the AUTH-255 branch 2 times, most recently from 6b7f922 to 485a082 Compare January 23, 2015 18:40
- this prevents arbitrary code injection via eval/system etc
@cthornton
Copy link
Contributor

👍

1 similar comment
@prateekm21
Copy link

👍

@luisvm
Copy link
Contributor

luisvm commented Jan 23, 2015

🐟

irb(main):004:0> response.send(:xpath_first_from_signed_assertion)
=> true
irb(main):005:0> puts $evalled
true
=> nil

vs

irb(main):007:0> response.send(:xpath_first_from_signed_assertion)
=> nil
irb(main):008:0> puts $evalled

=> nil

Lordnibbler added a commit that referenced this pull request Jan 23, 2015
AUTH-255: Sanitize document.signed_element_id via a prepared statement
@Lordnibbler Lordnibbler merged commit dfcbaef into master Jan 23, 2015
@Lordnibbler Lordnibbler deleted the AUTH-255 branch January 23, 2015 21:05
Lordnibbler added a commit that referenced this pull request Jan 26, 2015
AUTH-255: Sanitize document.signed_element_id via a prepared statement
Lordnibbler added a commit that referenced this pull request Jan 26, 2015
AUTH-255: Sanitize document.signed_element_id via a prepared statement
Conflicts:
	Gemfile
	test/response_test.rb
@reedloden
Copy link

I've filed #252 to figure out a better process for handling these types of issues. Would prefer all ruby-saml users aren't 0-day'd by researchers publicly posting vulnerability info directly on GitHub without there being a fixed version of the gem available for upgrading.

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 this pull request may close these issues.

5 participants