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

Fix XSS issues in default views #970

Merged
merged 3 commits into from May 25, 2017

Conversation

Projects
None yet
5 participants
@simkim
Contributor

simkim commented May 25, 2017

Default views allow client to xss resource owner to make him grant access, or client to xss admin.

Fix #969

simkim added some commits May 25, 2017

Fix xss by escaping tags
content_tag body is correctly escaped when rendered even if called is
wrapped by raw
let(:client_params) { {name: client_name } }
scenario 'resource owner visit authorization endpoint' do
visit authorization_endpoint_url(client: @client)
expect(page).not_to have_css('#xss')

This comment has been minimized.

@houndci-bot

houndci-bot May 25, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

context "with a client trying to xss resource owner" do
let(:client_name) { "<div id='xss'>XSS</div>" }
let(:client_params) { {name: client_name } }
scenario 'resource owner visit authorization endpoint' do

This comment has been minimized.

@houndci-bot

houndci-bot May 25, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -12,6 +13,15 @@
access_grant_should_not_exist
end
context "with a client trying to xss resource owner" do
let(:client_name) { "<div id='xss'>XSS</div>" }
let(:client_params) { {name: client_name } }

This comment has been minimized.

@houndci-bot

houndci-bot May 25, 2017

Space inside { missing.

@maclover7

This comment has been minimized.

Member

maclover7 commented May 25, 2017

Merging as a bugfix, will be released as part of normal process. Since we are putting restrictions on admins, they could technically just overwrite the forms, and do whatever they want. Such is a risk when using someone else's server for authorization 😬

If you have any possible security issues to report in the future, please contact the maintainers of this project before posting about them publicly.

@maclover7 maclover7 merged commit 7b1a837 into doorkeeper-gem:master May 25, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 81.709%
Details
hound No violations found. Woof!
@simkim

This comment has been minimized.

Contributor

simkim commented May 26, 2017

Sure, how can I improve the CONTRIBUTING file, are there designated maintainers for those kind of issue ?

@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Feb 17, 2018

Requested CVE for this because impact can be quite severe given it can execute on the authorization grant view.

@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Feb 21, 2018

Assigned CVE-2018-1000088 via Distributed Weakness Filing (DWF)

@f3ndot f3ndot referenced this pull request Feb 21, 2018

Merged

Add CVE-2018-1000088 #328

@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Feb 21, 2018

@tobypinder

This comment has been minimized.

tobypinder commented Feb 22, 2018

@f3ndot This writeup is comprehensive but the "Fix" section fails to identify the risk to client applications that have customised views via rails generate doorkeeper:views - if I understand correctly any future fixes to default views won't automatically be propagated back to these users as Doorkeeper will just use their templates, which will be based on the vulnerable old defaults.

I would imagine that this customisation would be performed by a significant proportion of Doorkeeper's userbase given that most would want to customise their user experiences, so I imagine this is not some niche edge-case.

@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Feb 22, 2018

@tobypinder Ah! Thanks for pointing that out. The writeup has been updated to call this particular scenario out.

reedloden added a commit to rubysec/ruby-advisory-db that referenced this pull request Feb 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment