Add audit.open_api_auth plugin#17363
Add audit.open_api_auth plugin#17363artem-smotrakov wants to merge 20 commits intoandresriancho:developfrom
Conversation
…ons for all auth schemes
| def __init__(self): | ||
| AuditPlugin.__init__(self) | ||
| self._spec = None | ||
| self._expected_codes = [401] |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
During a regular w3af run, this line will be printed to the output MANY times. I recommend using om.out.debug here.
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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. ' \ |
There was a problem hiding this comment.
... 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
When plugin A depends on B, and plugin A is enabled, plugin B will be automatically enabled too.
There was a problem hiding this comment.
That's nice, I didn't know about it. Let me add such a dependency.
|
Here is a couple of updates:
I also have a couple of comments/questions. Please let me know what you think. |
|
@andresriancho Still waiting for feedback (and for other pull requests too). Please take a look when you have time. |
2 participants
This pull request introduces a new plugin
audit.open_api_authwhich is described in #17343Implementation notes:
crawl.open_apiplugin.crawl.open_apiplugin shares API specification via the knowledge base.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.