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

Some features from the PR #197 (PR splitted) #225

Merged
merged 4 commits into from
Apr 29, 2015
Merged

Some features from the PR #197 (PR splitted) #225

merged 4 commits into from
Apr 29, 2015

Conversation

pitbulk
Copy link
Collaborator

@pitbulk pitbulk commented Apr 29, 2015

  • Comment the code.
  • Remove spaces and format some lines.
  • Remove unnecessary errors method.
  • Improve format_cert and format_private_key.
  • Fix xpath injection on xml_security.rb

* Comment the code
* Remove spaces and format some lines
* Remove unnecesary errors method
@pitbulk
Copy link
Collaborator Author

pitbulk commented Apr 29, 2015

@luisvm @daniel-g @Lordnibbler can I merge it?

attr_accessor :settings

# Array with the causes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this contain an array of Strings/Objects?

def service_name(name)
@name = name
end

# Set an index
# @param name [Integer] An index
#
def service_index(index)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not service_index= ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was only adding comments on this PR, the code on master is without the =.

Notice that the attribute is index, and we named the setter service_index.

@luisvm
Copy link
Contributor

luisvm commented Apr 29, 2015

👍 lgtm

@daniel-g
Copy link

👍

pitbulk added a commit that referenced this pull request Apr 29, 2015
Some features from the PR #197 (PR splitted):
* Comment the code.
* Remove spaces and format some lines.
* Remove unnecessary errors method.
* Improve format_cert and format_private_key.
* Fix xpath injection on xml_security.rb
@pitbulk pitbulk merged commit 1b4e3dd into master Apr 29, 2015
@reedloden
Copy link

What are the security concerns around "Fix xpath injection on xml_security.rb"? Is that exploitable in any way? If so, should get a CVE.

I also don't see a test focused on that issue. Is there one?

@pitbulk
Copy link
Collaborator Author

pitbulk commented Jul 9, 2015

@reedloden
Here is the change:
https://github.com/onelogin/ruby-saml/pull/225/files#diff-661b9d9743a3ff77661f224c6191165cR259
lib/xml_security.rb L259

It is similar bug than fixed here #183
Related to http://osvdb.org/show/osvdb/117903

@reedloden
Copy link

Sounds like it needs a CVE then. I'll put in a request for one.

@pitbulk
Copy link
Collaborator Author

pitbulk commented Jul 9, 2015

Ok, thanks @reedloden

@reedloden
Copy link

Requested in http://seclists.org/oss-sec/2015/q3/74

@reedloden
Copy link

Also, I went ahead and opened #252 to figure out a better process for handling security issues for the future.

@maikypedia
Copy link

maikypedia commented May 27, 2023

The CVE assigned is CVE-2015-20108

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