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

Don't force fuzzing auth headers for API endpoints #17358

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

artem-smotrakov
Copy link
Contributor

I noticed that the OpenAPI plugin forces fuzzing for auth headers. For example, it can happen if the API spec defines authentication with API keys passed in a header. Usually, a user provides such an auth header which then should be included to all HTTP request without any modification. So, it doesn't make much sense to force fuzzing for this header.

This patch fixes this issue. It updates RequestFactory to filter out auth headers.

@andresriancho
Copy link
Owner

Good point, I agree that this shouldn't be fuzzed for each endpoint, but shouldn't w3af at least fuzz it for one endpoint?

That way, if the REST API has a "SQL injection in the API key verification method" w3af would be able to find it.

@artem-smotrakov
Copy link
Contributor Author

Makes sense to me. Let me update the patch. Thanks!

@artem-smotrakov
Copy link
Contributor Author

artem-smotrakov commented Oct 25, 2018

I have updated the patch:

  • RequestFactory doesn't set headers for fuzzing. This feature is now in OpenAPI which I think is a better place for it.
  • Authentication headers which are defined in securityDefinitions are forced to be fuzzed only once. It doesn't apply to Authorization header - not sure if it should.
  • Moved test_no_forcing_fuzzing_auth_headers test to test_main.py
  • Added TestOpenAPIFuzzAuthHeaders
  • Fixed a bug in TestOpenAPIFindAllEndpointsWithAuth. I also fixed it in Update OpenAPI plugin to fuzz URL parts by default #17357 . Sorry for possible confusions. I create multiple pull requests to make the changes easier to review. But there are also some test bugs which create some noise. I am trying to get rid of them if possible but the fixes sometimes affect multiple pull requests :)

if parameter.location == 'header':

# Fuzz auth headers only once.
if self._is_already_fuzzed_auth_header(parameter.name, operation.swagger_spec):
Copy link
Owner

Choose a reason for hiding this comment

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

The parser shouldn't have any responsibility associated with fuzzing. The parser should just return what is found in the OpenAPI, then other parts of the code should be responsible of fuzzing (or not) the authentication header.

Not sure where to put this... maybe the solution is to modify the fuzzer.py code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. I wanted to have only API scans in scope. Then, I think the logic should look like the following:

  • We need to maintain a list of parameters which is used for auth. The list may be stored in the KB.
  • The OpenAPI plugin should add auth parameters to this list (it should consider parameters in headers and query string)
  • Probably we should also consider auth settings defined by http-settings and auth plugins but I would implement it separately.
  • create_mutants() method should take the list into account, or we can update HeadersMutant and QSMutant classes to take care about fuzzing auth parameters only once.

Copy link
Contributor Author

@artem-smotrakov artem-smotrakov Nov 9, 2018

Choose a reason for hiding this comment

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

Does it sound okay? If yes, I'll try to implement it.

@artem-smotrakov
Copy link
Contributor Author

I have updated the patch with what I described in my comment above. A couple of notes:

  • It pays attention only on authentication headers for APIs. It may make sense to update QSMutant here as well.
  • Probably this approach may be applied not only to API but I would prefer to keep this PR about API only, and update the rest in a separate PR (if necessary).
  • There are several TODOs which I am planning to fix once we agree on the way we'd like to go.
  • I need to test it a bit carefully, so please don't merge the PR yet :)

Looking forward to hearing your feedback!

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.

2 participants