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 case-insensitive option to word matcher #1130

Merged

Conversation

zerodivisi0n
Copy link
Contributor

@zerodivisi0n zerodivisi0n commented Oct 15, 2021

In this PR I tried to implement case-insensitive matches (#261).
I added this support for the following protocols: http, dns, file, headless, network.

Although this solution implements the feature, it has several drawbacks:

  1. It requires that matchers should also be defined in lowercase
  2. for extractors, the result will be returned in lowercase

Are these drawbacks critical to fix or current solution is enough? I think fixing them will require significant refactoring.
Do I need to add more tests? So far I have added a single test for the http protocol.

Example template:-

id: basic-get-case-insensitive

info:
  name: Basic GET Request
  author: pdteam
  severity: info

requests:
  - method: GET
    path:
      - "{{BaseURL}}"
    matchers:
      - type: word
        case-insensitive: true
        words:
          - "ThIS is TEsT MAtcHEr TExT"

@ehsandeep ehsandeep linked an issue Oct 16, 2021 that may be closed by this pull request
@ehsandeep ehsandeep added the Status: Review Needed The issue has a PR attached to it which needs to be reviewed label Oct 16, 2021
Copy link
Member

@Ice3man543 Ice3man543 left a comment

Choose a reason for hiding this comment

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

Code-wise this looks good to me, regarding tests, yeah, a few more tests with matches and extractors of different kinds would be appreciated.

@Ice3man543
Copy link
Member

Also regarding lowercase, could we do it so that if the user passed case-insensitive, matchers and everything would be automatically converted to lowercase as mentioned in your first point. This can be done in Compile function, and would remove the extra work for people writing the templates to make sure matchers and extractors are lowercase if they want case-insensitive?

@zerodivisi0n
Copy link
Contributor Author

Using Compile method to convert necessary data to lower case is a good idea, thank you.
I also added some tests for various cases. Most of the tests I added to the http protocol. And for the rest of the protocols, I added one basic test each.

v2/pkg/operators/matchers/compile.go Outdated Show resolved Hide resolved
v2/pkg/operators/extractors/compile.go Outdated Show resolved Hide resolved
@forgedhallpass
Copy link
Contributor

We need to make sure the changes do not negatively impact the request clustering functionality.

@zerodivisi0n
Copy link
Contributor Author

Looks like clustering ability does not depend on the operators.Operators fields in any way:

func (request *Request) CanCluster(other *Request) bool {
if len(request.Payloads) > 0 || len(request.Raw) > 0 || len(request.Body) > 0 || request.Unsafe || request.ReqCondition || request.Name != "" {
return false
}
if request.Method != other.Method ||
request.MaxRedirects != other.MaxRedirects ||
request.CookieReuse != other.CookieReuse ||
request.Redirects != other.Redirects {
return false
}
if !compare.StringSlice(request.Path, other.Path) {
return false
}
if !compare.StringMap(request.Headers, other.Headers) {
return false
}
return true
}

So I think these changes are safe. But I can add a simple test to check this.

@Ice3man543
Copy link
Member

@zerodivisi0n the problem is that the clusterer uses the same OperatorResult so it's modified in case-sensitive template, it can lead to other templates not behaving correctly in a cluster.

Relevant logic here - https://github.com/projectdiscovery/nuclei/blob/efbef301bfbc0d68bd8da88bb7ab0da9179aca13/v2/pkg/protocols/common/clusterer/executer.go

@zerodivisi0n
Copy link
Contributor Author

@Ice3man543 I moved PrepareData() call to Operators-s Execute method and looks like it solves the problem.

// Execute executes the operators on data and returns a result structure
func (operators *Operators) Execute(data map[string]interface{}, match MatchFunc, extract ExtractFunc, isDebug bool) (*Result, bool) {
data = operators.PrepareData(data)

In addition, this made it possible to remove PrepareData() call from each protocol. Now, this call is made in one single place.

I also added an integration test to check cluster functionality with two requests - base and case-insensitive. The test looks a bit ugly, but I decided not to change test harness a lot in this PR.

Copy link
Contributor

@forgedhallpass forgedhallpass left a comment

Choose a reason for hiding this comment

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

As agreed, the case-insensitive flag should be applied to KVal and Word matchers only. Currently the flag is set on Operator level, which is not correct/misleading (even if the underlying logic only modifies only the data related to KVal and Word). It also means that one template cannot have one case-sensitive and insensitive Word/KVal matcher.

@zerodivisi0n
Copy link
Contributor Author

Thanks for your feedback @forgedhallpass . I moved the case-insensitive flag to matchers/extractos level and updated all the necessary code. It turned out even simpler than it was.
In the templates, it will be defined as something like this:

requests:
- method: GET
path:
- "{{BaseURL}}"
matchers:
- type: word
case-insensitive: true
words:
- "ThIS is TEsT MAtcHEr TExT"

If this version will be ok for you, can I clean up the code before merging? I want to squash some commits and delete unnecessary ones.

@Ice3man543
Copy link
Member

@zerodivisi0n yeah sure, you can go ahead with the cleanup. I'll do a review after your cleanup then we can merge!

@zerodivisi0n
Copy link
Contributor Author

Hi @Ice3man543 ,
I've rearranged the code in commits and now you can review the PR.
I left one unrelated commit with refactoring (bfb69b2). If necessary, I'll remove it.

Copy link
Member

@Ice3man543 Ice3man543 left a comment

Choose a reason for hiding this comment

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

lgtm!

@ehsandeep ehsandeep added hacktoberfest-accepted Status: Completed Nothing further to be done with this issue. Awaiting to be closed. and removed Status: Review Needed The issue has a PR attached to it which needs to be reviewed labels Nov 1, 2021
@ehsandeep ehsandeep merged commit 1863e8f into projectdiscovery:dev Nov 1, 2021
@forgedhallpass
Copy link
Contributor

Thank you for your contribution @zerodivisi0n, it looks much better now!

@ehsandeep ehsandeep changed the title Add case-insensitive option to template Add case-insensitive option to word matcher Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Status: Completed Nothing further to be done with this issue. Awaiting to be closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Case-insensitive matches in a smarter way
4 participants