OneLogin::RubySaml::Response is broken on rexml 3.3.9

[msxavi]

Hi there,
Given an invalid SAMLResponse, the Response object now raises REXML::ParseException on rexml 3.3.9.

  1) SamlController POST #consume without an original page when invalid response
     Failure/Error:
       @sso_response ||= OneLogin::RubySaml::Response.new(
         params[:SAMLResponse],
         settings: sso_config.saml_settings,
         allowed_clock_drift: 60,
         skip_subject_confirmation: true
       )

     REXML::ParseException:
       Malformed XML: Content at the start of the document (got 'invalid')
       Line: 1
       Position: 7
       Last 80 unconsumed characters:
     # /usr/local/bundle/gems/rexml-3.3.9/lib/rexml/parsers/baseparser.rb:517:in `pull_event'

Which means the response interface is now broken on 1.17.0

response.is_valid?

Related to ruby/rexml#211

TIA

[pitbulk]

Hi @msxavi

We could catch the REXML::ParseException when building the Response (and others), and raise an ArgumentError similar than

raise ArgumentError.new("Response XML is invalid")

as we do here

[johnnyshields]

We need to get off REXML entirely and move to Nokogiri. REXML is not thread safe, and RubySaml patches it in a way that could potentially conflict with other gems.

[johnnyshields]

Work started here: #736

[jwoodrow]

Hi, thanks for the work on maintaining the ruby-saml gem !

I'm getting the same errors even with REXML 3.3.3, the problem being that versions prior to 3.3.6 have a high CVE and prior to 3.3.9 have 3 moderate CVEs

Seeing as 2.x release seems in progress, is 3.x something coming anytime soon or should we start thinking of dropping SAML support on our applications ?

[johnnyshields]

@jwoodrow we are happy to accept a patch that fixes these specific REXML errors. We are also working on REXML removal as our next major step, but no specific timeline for it yet.

[jwoodrow]

Hey @johnnyshields

I'm not too knowledgable about how rexml is used in the case of ruby-saml but I could at least provide the stack trace for the error (since it occurs in the CI when testing an invalid SAML response)

SamlSessionsController#post when the identity provider response is not valid when the identity provider response is not valid redirects to the login page with flash error
     Failure/Error: @saml_response = OneLogin::RubySaml::Response.new(saml_response_params[:SAMLResponse], settings: saml_settings)

     REXML::ParseException:
       Malformed XML: Content at the start of the document (got 'fc8xT8MwEAXgv5')
       Line: 1
       Position: 14
       Last 80 unconsumed characters:
     # ./vendor/bundle/ruby/3.3.0/gems/rexml-3.3.3/lib/rexml/parsers/baseparser.rb:487:in `pull_event'
     # ./vendor/bundle/ruby/3.3.0/gems/rexml-3.3.3/lib/rexml/parsers/baseparser.rb:218:in `pull'
     # ./vendor/bundle/ruby/3.3.0/gems/rexml-3.3.3/lib/rexml/parsers/treeparser.rb:22:in `parse'
     # ./vendor/bundle/ruby/3.3.0/gems/rexml-3.3.3/lib/rexml/document.rb:448:in `build'
     # ./vendor/bundle/ruby/3.3.0/gems/rexml-3.3.3/lib/rexml/document.rb:101:in `initialize'
     # ./vendor/bundle/ruby/3.3.0/gems/ruby-saml-1.17.0/lib/xml_security.rb:191:in `initialize'
     # ./vendor/bundle/ruby/3.3.0/gems/ruby-saml-1.17.0/lib/onelogin/ruby-saml/response.rb:67:in `new'
     # ./vendor/bundle/ruby/3.3.0/gems/ruby-saml-1.17.0/lib/onelogin/ruby-saml/response.rb:67:in `initialize'

I'm down to try and look into it as a monkey patch to begin with (until our specs pass) and then try and open up a pull request, but I'm truly at a loss when is comes to this baseparser file and the xml_security file too 😓

[pitbulk]

We are working on get off REXML entirely.

[jwoodrow]

If you need some real world testing once a branch that seems to be running and passing tests, I can give it a shot and give you some updates if you want

[kwent]

Looking forward to this.

[johnnyshields]

@kwent this issue should be fixed on this branch, please try it: #745

This will be merged into v2.x soon.

[johnnyshields]

@pitbulk this can be closed after v2.0.0 release