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

Authorization Bypass Through User-Controlled Key in url-parse #2430

Merged
merged 4 commits into from
Oct 11, 2022

Conversation

mik-patient
Copy link
Contributor

@mik-patient mik-patient commented Sep 28, 2022

Description

url-parse is a Small footprint URL parser that works seamlessly across Node.js and browser environments.

Affected versions of this package are vulnerable to Authorization Bypass via the hostname field of a parsed URL, because "url-parse" is unable to find the correct hostname when no port number is provided in the URL.

PoC:

var Url = require('url-parse'); var PAYLOAD = "http://example.com:"; console.log(Url(PAYLOAD)); // Expected hostname: example.com // Actual hostname by url-parse: example.com:

Output:

{ slashes: true, protocol: 'http:', hash: '', query: '', pathname: '/', auth: '', host: 'example.com:', port: '', hostname: 'example.com:', password: '', username: '', origin: 'http://example.com:', href: 'http://example.com:/' }

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Thank you, @mik-patient!

@mik-patient
Copy link
Contributor Author

You're welcome @crodriguezvega

@colin-axner
Copy link
Contributor

colin-axner commented Oct 3, 2022

We don't directly import url-parse. Will this still fix the issue?

@mik-patient
Copy link
Contributor Author

Hi @colin-axner Thanks your for the respond.

Assuming a user with a given identity, authorization is the process of determining whether that user can access a given resource, based on the user's privileges and any permissions or other access-control specifications that apply to the resource. When access control checks are not applied consistently - or not at all - users are able to access data or perform actions that they should not be allowed to perform. This can lead to a wide range of problems, including information exposures, denial of service, and arbitrary code execution. The software does not perform or incorrectly performs an authorization check when an actor attempts to access a resource or perform an action.

I have requested pull-request for fixed this issue.

Best Regards
@mik-patient

@colin-axner
Copy link
Contributor

@chatton do we have a way to check if the deployed docs will work with this change? I don't see an action?

@mik-patient
Copy link
Contributor Author

Hi Team @colin-axner

Thank you for your respond, im sorry for late respond because im bussy work in office now.
You can merged/approved this pull-requested for fixed/patch this vulnerabilities, I have added references about github advisitories CVeS scores.

Best regards,
@mik-patient

@chatton
Copy link
Contributor

chatton commented Oct 10, 2022

@colin-axner it looks like the build & deploy docs happens only on merge to release or main branches. We would need to add an additional task that builds docs from forks/branches with relevant changes to test this before merging it seems.

@colin-axner
Copy link
Contributor

What do you think we should do then @chatton? Since it seems possible merging this could potentially not work on deployments? Will it build if we open the pr?

@chatton
Copy link
Contributor

chatton commented Oct 10, 2022

@colin-axner unfortunately the docs building doesn't necessarily mean that it will render correctly. (In the past the broken docs built successfully)

I think what we should do is add a new task which will build the docs and perform a basic sanity check that the html content is non-empty (even something as simple as curl/grep for some docs content)

In the meantime, for this PR we can just verify locally that docs render correctly.

I did this quickly and everything looks good

I used make view-docs

I created #2503 to track this

image

@colin-axner
Copy link
Contributor

Thanks @chatton! ❤️ ❤️

@crodriguezvega crodriguezvega merged commit bf547e4 into cosmos:main Oct 11, 2022
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.

4 participants