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

Perform checks first #126

Merged
merged 1 commit into from Oct 16, 2019
Merged

Perform checks first #126

merged 1 commit into from Oct 16, 2019

Conversation

@utkarsh2102
Copy link
Contributor

utkarsh2102 commented Oct 15, 2019

My take is that this method will raise ProtocolError in the very first place (which is a good thing!) unless the request is a valid id_res response. Once it has been verified, the methods endpoint, message, and signed_fields contain the verified information.
Thereby making everything secure from any third party interference.

Closes #124

@tobiashm

This comment has been minimized.

Copy link
Contributor

tobiashm commented Oct 16, 2019

Why have you moved check_for_fields down? AFAIR its purpose is to check if all the required fields are provided before they're being used by the other checks.

@utkarsh2102

This comment has been minimized.

Copy link
Contributor Author

utkarsh2102 commented Oct 16, 2019

@tobiashm,

Why have you moved check_for_fields down?

I did not move check_for_fields down but moved check_signature and check_nonce up. Because signature checking should come first and then the rest of it.

@tobiashm

This comment has been minimized.

Copy link
Contributor

tobiashm commented Oct 16, 2019

Okay, but I still think check_for_fields should go first, as it checks if the fields that the other methods rely on. E.g. check_signature uses the assoc_handle field which must be present.

Closes #124
@utkarsh2102 utkarsh2102 force-pushed the utkarsh2102:CVE-2019-11027 branch from 80a0228 to f526132 Oct 16, 2019
@utkarsh2102

This comment has been minimized.

Copy link
Contributor Author

utkarsh2102 commented Oct 16, 2019

I still think check_for_fields should go first..

Ack, fixed and pushed.

@tobiashm tobiashm merged commit a7914e9 into openid:master Oct 16, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tobiashm

This comment has been minimized.

Copy link
Contributor

tobiashm commented Oct 16, 2019

Thanks. Merged and released v2.9.2

@tobiashm

This comment has been minimized.

Copy link
Contributor

tobiashm commented Oct 16, 2019

Hm. Issue #125 suggests that there are some more internal assumptions about the execution order of the checks in id_res. Not sure if this has made it worse, but at least it's something that needs further investigation.

@utkarsh2102

This comment has been minimized.

Copy link
Contributor Author

utkarsh2102 commented Oct 16, 2019

Aha! Shall try to look this up and reproduce :)

@utkarsh2102 utkarsh2102 deleted the utkarsh2102:CVE-2019-11027 branch Oct 20, 2019
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 18, 2019
Update ruby-ruby-openid package to version 2.9.2.


## 2.9.2

* Perform all checks before verifying endpoints.
  [#126](openid/ruby-openid#126)

## 2.9.1

* Updated CHANGELOG.md

## 2.9.0

* Remove deprecated `autorequire` from gemspec.
  [#123](openid/ruby-openid#123)
* Rescue from `Yadis::XRI::XRIHTTPError` on discovery.
  [#106](openid/ruby-openid#106)
* Avoid SSRF for claimed_id request.
  [#121](openid/ruby-openid#121)
* Updated documentation.
  [#115](openid/ruby-openid#115), [#116](openid/ruby-openid#116), [#117](openid/ruby-openid#117), [#118](openid/ruby-openid#118)
* Reduce warnings output in test runs.
  [#119](openid/ruby-openid#119)
* Drop deprecated option from gemspec.
  [#120](openid/ruby-openid#120)
* Remove circular require.
  [#113](openid/ruby-openid#113)
* Updated Travis CI config with Ruby 2.6
  [#114](openid/ruby-openid#114)
* Simplify Bundler require; remove need for extra `:require`.
  [#112](openid/ruby-openid#112)

## 2.8.0

* Fix `admin/mkassoc` script.
  See openid/ruby-openid#103
* Allow specifying timeout for `OpenID::StandardFetcher` in environment variables.
  See openid/ruby-openid#109
* Fixed some documentation.
  See openid/ruby-openid#111
* Fixed example server.
  See openid/ruby-openid#91
* Fixed tests.
  See openid/ruby-openid#86
* Misc. changes to the CI setup.
  See
  - openid/ruby-openid#110
  - openid/ruby-openid#108
  - openid/ruby-openid#107
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.