Cariddi v2

[edoardottt]

This discussion will be used to share ideas / roadmap for cariddi v2.
cc @cyb3rjerry

• Wrap the results in a struct #96
• Wrap the New() params into a single object #95
• Fix code scanning alert - Incomplete regular expression for hostnames #98
• Second ctrl+c should force quit the program #89
• Refactor visitHTMLLink() #100
• Improve Regex to avoid false positives #101

[humpty-tony]

👋 So here would be my initial proposed changes

Wrap the New() params into a single object.

The benefits of this would be better readability, better control over the mutability of the object considering multiple instances of it can be spawned and would be a little more idiomatic from a Go perspective.
The function would then look something more like:

func New(scanCtx *ScanCtx){
...
}

Where the ScanCtx struct could look like:

type ScanCtx struct {
	Debug         bool
	ErrorsFlags   bool
	EndpointsFlag bool
	EndpointsFile bool
	InfoFlag      bool
	Insecure      bool
	Plain         bool
	SecretsFlag   bool
	FileType      int
	DelayTime     int
	Threads       int
	Html          string
	Ignore        string
	Target        string
	Txt           string
	SecretsFile   []string
	Headers       map[string]string
}

Wrap the results in a struct that has a sync.Mutex to prevent data races

Similarily to my previous point, we could also return a single object that has a Mutex attribute to make sure we don't cause data races when writing them.

The object could look something like

type Results struct {
	Mutex      sync.Mutex
	Results    []string
	Secrets    []scanner.SecretsMatched
	Endpoints  []scanner.EndpointMatched
	Extensions []scanner.FileTypeMatched
	Errors     []scanner.ErrorMatched
	Infos      []scanner.InfoMatched
}

We could then easily write to the object by doing something like:

results.Mutex.Lock()
results.Results = append(results.Results, something...)
results.Mutex.Unlock()

A good example of how to use Mutexes to prevent race conditions can be seen here

[humpty-tony]

Instead of using Mutexes we could also instead pipe all results into a chan that then gets processed. We don't really care for the order of the results, only that there is some results. This could be a very clean way of handling concurency

[edoardottt]

Wrap the New() params into a single object.

Absolutely ok for this one.

Regarding the struct, it should contain all the parameters passed to the New function now, something like this:

type Scan struct {
	Target        string
	Txt           string
	Html          string
	Delay         int
	Concurrency   int
	Ignore        string
	IgnoreTxt     string
	Cache         bool
	Timeout       int
	Intensive     bool
	Rua           bool
	Proxy         string
	Insecure      bool
	SecretsFlag   bool
	SecretsFile   []string
	Plain         bool
	EndpointsFlag bool
	EndpointsFile []string
	FileType      int
	Headers       map[string]string
	ErrorsFlag    bool
	InfoFlag      bool
	Debug         bool
	UserAgent     string
}

Moreover, I think that also CreateColly function

cariddi/pkg/crawler/colly.go

Line 322 in 4d59028

func CreateColly(delayTime int, concurrency int, cache bool, timeout int,

can take that struct as input.

What do you think?

[humpty-tony]

I think this sounds reasonable. We might, however, want a "sub-struct" for results so we can add a little more control from a mutability POV
We could do for example

type Results struct {
       Results    []string
       Secrets    []scanner.SecretsMatched
       Endpoints  []scanner.EndpointsMatched
       Extensions []scanner.FileTypeMatched
       Errors     []scanner.ErrorMatched
       Infos      []scanner.InfoMatched
}

type Scan struct {
	Target        string
	Txt           string
	Html          string
	Delay         int
	Concurrency   int
	Ignore        string
	IgnoreTxt     string
	Cache         bool
	Timeout       int
	Intensive     bool
	Rua           bool
	Proxy         string
	Insecure      bool
	SecretsFlag   bool
	Plain         bool
	EndpointsFlag bool
	FileType      int
	Headers       map[string]string
	ErrorsFlag    bool
	InfoFlag      bool
	Debug         bool
	UserAgent     string
        Results       *Results
}

This way, the config is set directly within the Scran struct, and the actual results are stored within the Results attribute

[edoardottt]

Wrap the results in a struct

Absolutely ok also with this one.

[edoardottt]

Prevent data races

It's up to you to decide which method to use, I'm okay both with mutex or channels.
Maybe with channels could be a lil bit more annoying to remove duplicates (There shouldn't be duplicates...but just to be sure), BUT using this method we can write results in output files while the crawler is scanning. Now instead it crawls everything and then it prints to file at the end.
But we should also rewrite the output write functions to be more flexible.

P.S. With millions of URLs crawled, remove duplicates could be a huge bottleneck, we should think carefully on which method to use

[humpty-tony]

Yeah the following method is typically what I do since empty structs have a size of zero and hashtables (maps) have a complexity near O(1)

var secretsList :=  make(map[string]struct{})

if _, ok := secretsList["newSecret"]; !ok { // If key doesn't exist, append it
    secretsList["newSecrets"] = struct{}{}
}

[edoardottt]

Okay, seems good. I have to think about this.

Some random thoughts:

• Remember to fork also the devel branch and create PRs against the cariddi/devel branch.
• I think even if we write results to file while crawling it's better to print them on cli at the end (not talking about URLs, but secrets, infos...)

For now we can work on the three issues ready and then go back at it :) I will update the first message of the discussion

[humpty-tony]

@edoardottt Do you happen to have a specific URL that you used for testing that could be used as a benchmark?

[humpty-tony]

Sounds good, got a server I could use for this. If you can make the test pages I'll gladly set it up in my k8s cluster so we'll be able to deploy it with certificates, a clean subdomain, etc.. so it can be as realistic as possible

[edoardottt]

I'm going to sleep rn :/ tomorrow I will setup everything :) thanks again for your contribution ❤️

[humpty-tony]

Yeah we can do all of this in due time, no rush :)

[edoardottt]

We could also use this https://github.com/google/security-crawl-maze. Of course a test page for secrets, infos, etc... can be created as a standalone test :)

[humpty-tony]

Yeah I think we can use that for sure, we might want to sprinkle secrets left and right into it however

[humpty-tony]

Improve Regex

I think we might eventually want to add a little more specificity to the Regex rules as they currently tend to return false positives

[edoardottt]

absolutely agree.

• Improve Regex to avoid false positives #101

[humpty-tony]

Refactor visitHTMLLink()

visitHTMLLink() currently takes 11 params while we could most likely find a way to simply pass the original Scan struct and do manipulations within the function

This would turn the 9 visitHTMLLink(arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9){...} into visitHTMLLInk(&Scan)

[edoardottt]

absolutely agree :)

• Refactor visitHTMLLink() #100

[edoardottt]

Also CreateColly
https://github.com/edoardottt/cariddi/blob/devel/pkg/crawler/colly.go#L357
can take the scan struct as input :) @cyb3rjerry

[edoardottt]

@cyb3rjerry I've just updated the devel branch, I've removed the insecure flag. Now it skips verifying certificates accepting also invalid ones

[edoardottt]

Give a look also to this new tool :P @cyb3rjerry https://github.com/edoardottt/csprecon, let me know if you think it can be improved :)