Skip to content

Commit 9d75c85

Browse files
committed
Sanitize document.signed_element_id via prepared statement
- this prevents arbitrary code injection via eval/system etc
1 parent 9627684 commit 9d75c85

4 files changed

Lines changed: 29 additions & 3 deletions

File tree

Gemfile

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,4 +18,5 @@ group :test do
1818
gem "timecop", "<= 0.6.0"
1919
gem "systemu", "~> 2"
2020
gem "rspec", "~> 2"
21+
gem 'pry'
2122
end

lib/onelogin/ruby-saml/response.rb

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,18 @@ def validate_response_state(soft = true)
184184
end
185185

186186
def xpath_first_from_signed_assertion(subelt=nil)
187-
node = REXML::XPath.first(document, "/p:Response/a:Assertion[@ID='#{document.signed_element_id}']#{subelt}", { "p" => PROTOCOL, "a" => ASSERTION })
188-
node ||= REXML::XPath.first(document, "/p:Response[@ID='#{document.signed_element_id}']/a:Assertion#{subelt}", { "p" => PROTOCOL, "a" => ASSERTION })
187+
node = REXML::XPath.first(
188+
document,
189+
"/p:Response/a:Assertion[@ID=$id]#{subelt}",
190+
{ "p" => PROTOCOL, "a" => ASSERTION },
191+
{ 'id' => document.signed_element_id }
192+
)
193+
node ||= REXML::XPath.first(
194+
document,
195+
"/p:Response[@ID=$id]/a:Assertion#{subelt}",
196+
{ "p" => PROTOCOL, "a" => ASSERTION },
197+
{ 'id' => document.signed_element_id }
198+
)
189199
node
190200
end
191201

test/response_test.rb

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,7 @@ class RubySamlTest < Test::Unit::TestCase
332332
OneLogin::RubySaml::Attributes.single_value_compatibility = false
333333
assert_equal nil, response.attributes[:attribute_not_exists]
334334
assert_equal nil, response.attributes.single(:attribute_not_exists)
335-
assert_equal nil, response.attributes.multi(:attribute_not_exists)
335+
assert_equal nil, response.attributes.multi(:attribute_not_exists)
336336
OneLogin::RubySaml::Attributes.single_value_compatibility = true
337337
end
338338

@@ -368,5 +368,13 @@ class RubySamlTest < Test::Unit::TestCase
368368
end
369369
end
370370

371+
context '#xpath_first_from_signed_assertion' do
372+
should 'not allow arbitrary code execution' do
373+
malicious_response_document = fixture('response_eval', false)
374+
response = OneLogin::RubySaml::Response.new(malicious_response_document)
375+
response.send(:xpath_first_from_signed_assertion)
376+
assert_equal($evalled, nil)
377+
end
378+
end
371379
end
372380
end

test/responses/response_eval.xml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
<saml:Response xmlns:saml="urn:oasis:names:tc:SAML:2.0:protocol">
2+
<ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
3+
<ds:SignedInfo>
4+
<ds:Reference URI="#x'] or eval('$evalled = true') or /[@ID='v"/>
5+
</ds:SignedInfo>
6+
</ds:Signature>
7+
</saml:Response>

0 commit comments

Comments
 (0)