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

Add audit.open_api_auth plugin #17363

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

Conversation

artem-smotrakov
Copy link
Contributor

This pull request introduces a new plugin audit.open_api_auth which is described in #17343

Implementation notes:

  • The new plugin works only with API endpoints. It requires an API specification which has to be provided by crawl.open_api plugin.
  • The crawl.open_api plugin shares API specification via the knowledge base.
  • The new plugin simply checks that endpoints return 401 error if no authentication info is provided in the request.

Please consider it as a first version of the plugin. I was going to make the plugin smaller but it turned out that it needs quite a lot of checks, so now the plugin is not that small :)

I believe the plugin may be improved. Please take a look at several enhancements in the plugin description, and let me know what you think. I am planning to work on them but would prefer to implement them separately after the first version of the plugin is integrated.

@andresriancho andresriancho self-assigned this Nov 8, 2018
Copy link
Owner

@andresriancho andresriancho left a comment

Choose a reason for hiding this comment

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

Awesome job!

def __init__(self):
AuditPlugin.__init__(self)
self._spec = None
self._expected_codes = [401]
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe add 403?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also thought about it. In a perfect world, an application should return 401 if no auth info is present at all. 403 should be usually returned if a request contains auth info, but authorization failed. So now the plugin assumes that it lives in a perfect world :) I think it is not a big deal If an application returns 403 instead of 401 but strictly speaking it doesn't sound correct to me. Anyway, I don't mind adding 403 here. I would even let a user to configure it - it may be next improvement.

# The check works only with API endpoints.
# crawl.open_api plugin has to be called before.
if not self._is_api_spec_available():
om.out.information("Could not find an API specification, skip")
Copy link
Owner

Choose a reason for hiding this comment

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

During a regular w3af run, this line will be printed to the output MANY times. I recommend using om.out.debug here.

Copy link
Owner

Choose a reason for hiding this comment

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

Also the message could be improved: "Could not find an API specification in the KB, skipping open_api_auth"


# The API spec must define authentication mechanisms.
if not self._has_security_definitions_in_spec():
om.out.information("The API spec has no security definitions, skip")
Copy link
Owner

Choose a reason for hiding this comment

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

Change message to The Open API spec has no security definitions, not running open_api_auth checks.

Also, this should be printed only once per each open-api specification document w3af finds.

# The check only runs if the API specification
# requires authentication for a endpoint.
if not self._has_auth(freq):
om.out.information("Request doesn't have auth info, skip")
Copy link
Owner

Choose a reason for hiding this comment

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

Improve message and change to debug.

:param response: The HTTP response associated with the fuzzable request.
"""
if response.get_code() not in self._expected_codes:
desc = 'A %s request without auth info was send to %s. ' \
Copy link
Owner

Choose a reason for hiding this comment

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

... request without authentication information ...

was sent to

replied with unexpected HTTP response code %d (expected one of 401, 403, etc.)

And add: This is a strong indicator of a REST API authentication bypass.

response.get_code(), self._expected_codes)

# Should severity depend on response codes?
# For example, 2xx codes results to HIGH, but 4xx may be MEDIUM/LOW
Copy link
Owner

Choose a reason for hiding this comment

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

Yes, this is a good idea to remove false positives.

The way I recommend doing it is to add a new check below if not self._has_auth(freq), like this:

        if not self._response_is_successful(freq):
            om.out.debug("OpenAPI request {some request info here} did not return a successful response. Skipping open_api_auth check for this endpoint.")

Where _response_is_successful would send the request (with credentials) and check that the response code is 200, 201, or some other codes which are commonly used to indicate success.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean adding _response_is_successful check between _remove_auth and _remove_auth and _uri_opener.send_mutant calls in audit() method? But wasn't this request already sent? Is the result already available in orig_response?

What would be a definition of a successful response? 200 may be not enough since a endpoint may return 201, 202, etc. In fact, response codes should be defined in the spec. In general, it would be a nice improvement. Do you think such a check is strictly necessary?

In fact, I meant another thing here. I am talking about assigning different severity levels what we report a vulnerability to the KB. It should be done if/after _response_is_successful check passed.

if self._spec:
return True

specification_handler = kb.kb.raw_read('open_api', 'specification_handler')
Copy link
Owner

Choose a reason for hiding this comment

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

What if there is more than one openapi document found during a scan?

Maybe you should store a list and call it kb.kb.raw_read('open_api', 'specification_handlers') (added an s at the end)

This also means that you'll have to touch other areas of this plugin to process all specification handlers, not just one.

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 although it doesn't look like a typical situation. I'll try to address it. Hope it won't make the plugin too complicated.

* Check if the response has 401 error code (access denied).

A couple of important notes:
* The plugin requires the 'crawl.open_api' plugin to be enabled.
Copy link
Owner

Choose a reason for hiding this comment

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

You can force that. Plugins can have other plugins as dependencies.
https://github.com/andresriancho/w3af/blob/master/w3af/core/controllers/plugins/plugin.py#L123

Copy link
Owner

Choose a reason for hiding this comment

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

When plugin A depends on B, and plugin A is enabled, plugin B will be automatically enabled too.

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 nice, I didn't know about it. Let me add such a dependency.

@artem-smotrakov
Copy link
Contributor Author

Here is a couple of updates:

  • audit.open_api_auth now depends on crawl.open_api.
  • audit.open_api_auth now expects multiple specs.
  • Updated debug messages.
  • Added TestOpenAPIAuthMultipleSpecs.

I also have a couple of comments/questions. Please let me know what you think.

@artem-smotrakov
Copy link
Contributor Author

artem-smotrakov commented Jan 12, 2019

@andresriancho Still waiting for feedback (and for other pull requests too). Please take a look when you have time.

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