search

SAP / odfuzz

Apache License 2.0
18 stars 12 forks source link

DO NOT MERGE - add env variable IGNORE_METADATA_RESTRICTIONS #65

Closed prashdsouza closed 4 years ago

prashdsouza commented 4 years ago

When the scan is triggered by FioriDAST we would like to have more possiblities of fuzzed url even if sap:filterable="false". This pull request is to enable FioriDAST to ignore restriction.

CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

phanak-sap commented 4 years ago

I was puzzled by the need for this PR, since for the purpose of the integration was created DirectBuilder and it so far does not support reading the restrictions.yaml as in CLI version. And since no restriction are loaded and applied, the proposed code change should in my mind create similar results.

Then we with Prashnath found that the env variable in the PR really produces different results even without any restrictions.yaml file loaded.

Root cause is line https://github.com/SAP/odfuzz/blob/master/odfuzz/entities.py#L357, not the part where the PR is injecting the feature.

And I am inclined to think at the moment that the line is there on purpose, to not generate invalid URLs.

This needs to be investigated further, if the PR will bring value it hopes it will.

lubosmj commented 4 years ago

@prashdsouza, it seems like you want to disable checks for all sap related restrictions, not just sap:filterable. Bear in mind, that ODfuzz heavily relies on metadata provided by a remote server. Currently, ODfuzz initializes queryable entities based on multiple sap restrictions, such as sap:searchable, sap:toable, sap:pageable, sap:filter-restriction, etc. Refer to https://wiki.scn.sap.com/wiki/display/EmTech/SAP+Annotations+for+OData+Version+2.0 to learn more.

So I propose:

  1. either to leave this PR as it is, where we are basically ignoring only sap:filterable along with sap:sortable (do not forget to state that not all the restrictions are going to be ignored; I am just referring to the environment variable called IGNORE_METADATA_RESTRICTIONS)
  2. or to update the PR and add an option to disable all sap related restrictions.

Since ODfuzz was intended to generate valid URLs, why would we want to have an option to change this? Once a server says that you should not send requests that contain certain properties, why would we want to ignore that?

prashdsouza commented 4 years ago

@lubosmj I agree with your proposal 2, So with the latest commit our tests have checked that atleast the below sap properties will be ignored.

sap: pageable sap: searchable sap: countable sap: topable

However note that except searchable the remaining by default are true. Also with the previous commit (ignoring filterable and sortable) we confirmed that there are no real 404 get requests arising out of these 2 commits.

In the end our goal is to generate fuzzing payloads for properties which even server chooses to currently ignore. This way we can detect vulnerabilites / 5XX issues without relying on what the OData service currently decides to do.

Kindly let me know if we are good to go ahead with the merge. If so a version update which will be then referred to by the integrating tools.

lubosmj commented 4 years ago

I have no objections to merge these changes. I just wanted to share my thoughts on this.

@phanak-sap, please, do the needful.

prashdsouza commented 4 years ago

@lubosmj Thank you for the feeback. @phanak-sap After ignoring restrictions as above there are no additions to 404 error codes. This means as per my understanding that the queries generated after ignoring restrictions are valid.

phanak-sap commented 4 years ago

Thank you both @lubosmj and @prashdsouza. I must say in this case I was not sure if it is a good idea to add this feature or not.

It is quite bad that this PR cannot be easily accompanied by some kind of deterministic test, that would generate URLs with metadata file containing all four sap properties and both true/false settings...