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

Added custom prefix, suffix functionalities and -random-agent. #56

Closed
wants to merge 13 commits into from

Conversation

BBerastegui
Copy link

@BBerastegui BBerastegui commented Sep 19, 2017

I added the option to specify custom prefix and suffix to the wordlist entries, just in case we want to try any kind of weird path combination, such as %2f or //, etc.

Copy link
Contributor

@0xdevalias 0xdevalias left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. May require minor changes to merge into 1.4-dev refactoring when it comes to it. Does the current CLI param flag implementation support --param format? If so, would make more sense to use --prefix and --suffix IMO.

@BBerastegui
Copy link
Author

BBerastegui commented Oct 10, 2017

The --param format seems not to be "officially" supported, but it works if being use in the command line.

@BBerastegui
Copy link
Author

I also added the -random-agent feature with the user-agent dictionary from sqlmap.

@BBerastegui BBerastegui changed the title Added custom prefix and suffix functionalities. Added custom prefix, suffix functionalities and -random-agent. Oct 11, 2017
@OJ
Copy link
Owner

OJ commented Jan 14, 2018

Hi @BBerastegui Thanks very much for this. I've just landed 1.4 (finally), sorry for the delay. Could you please rebase this against those changes? Thank you very much!

@BBerastegui
Copy link
Author

I'm trying to do so :P

But:

libgobuster/dir.go:128: undefined: uuid.Must

I can't find what's going on here. Any clue what I'm missing?

:'(

@OJ
Copy link
Owner

OJ commented Jan 16, 2018

Be sure to update your uuid lib code, that'll remove the compiler error.

@BBerastegui
Copy link
Author

As a reference, for those getting:

undefined: uuid.Must error when doing go build .

You should force update of the dependencies (doing only go get in the uuid library wasn't working for me):

go get -u -v

@BBerastegui
Copy link
Author

Me using Github like:

I'm sorry for this mess. Shame.

if s.RandomUserAgent && s.UserAgent == "" {
b, err := ioutil.ReadFile("user-agents.txt")
if err != nil {
fmt.Print(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

You catch the error here, print it, but then execution will just continue I believe? We would probably be better returning the error here, or (maybe, but it's gross) panic'ing?

Copy link
Author

Choose a reason for hiding this comment

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

How do you suggest to return if error then?

return nil, nil ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Normally it would be ‘return nil, er’ or similar. On my phone so can’t check the return type, may need to add a 3rd type (error) if it’s already returning 2. Then return nil, nil, error.

Can’t remember if I started implementing multierror in this or not, if so, possibly using that type might be beneficial.

Would also have to add an error check around anywhere that calls this.

Ideally we would be bubbling any errors back up to the highest point, so it can decide how to handle them (or die). Rather than the current panics we have scattered around the place.

Copy link
Contributor

Choose a reason for hiding this comment

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

A little divergent from initial PR, but implementing that pattern for this function would help to clear up some of the existing tech debt.

Copy link
Author

Choose a reason for hiding this comment

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

Function MakeRequest returns (*int, *int64) so it gives an error if you try to return nil, err.

I'll leave it as the rest of the returns in that function, as nil, nil

@@ -59,6 +61,22 @@ func MakeRequest(s *State, fullUrl, cookie string) (*int, *int64) {
req.Header.Set("User-Agent", s.UserAgent)
}

if s.RandomUserAgent && s.UserAgent == "" {
b, err := ioutil.ReadFile("user-agents.txt")
Copy link
Contributor

Choose a reason for hiding this comment

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

Hardcoded filename, is there a way to specify this from the CLI parameters?

@@ -36,39 +36,42 @@ type StringSet struct {
// Contains State that are read in from the command
// line when the program is invoked.
type State struct {
Client *http.Client
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this is all marked as such a big block of changes. Did the whitespacing used change or something? Does this match gofmt?

Copy link
Author

Choose a reason for hiding this comment

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

This does match gofmt, but I have no idea either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, fair enough. Not a big issues

@0xdevalias
Copy link
Contributor

0xdevalias commented Feb 14, 2018 via email

@BBerastegui
Copy link
Author

GoGet must now then return one additional argument too:

func GoGet(s *State, url, uri, cookie string) (*int, *int64, error) { return MakeRequest(s, url+uri, cookie) }

Would you like to keep sending this error up to the moon?

Before:
wildcardResp, _ := GoGet(s, s.Url, fmt.Sprintf("%s", guid), s.Cookies)
After:
wildcardResp, _, err := GoGet(s, s.Url, fmt.Sprintf("%s", guid), s.Cookies)

And same in all cases where GoGet is being called?

@0xdevalias
Copy link
Contributor

0xdevalias commented Feb 14, 2018 via email

@BBerastegui
Copy link
Author

I'll left TODO's for error handling as you guys prefer to do it.

@0xdevalias
Copy link
Contributor

Thanks for making those changes @BBerastegui !

@OJ I haven't actually run this PR to confirm it all works as expected, but assuming it does, i'm pretty happy with how it looks from a code perspective. So if it works/you're happy, LGTM!

@@ -59,6 +61,22 @@ func MakeRequest(s *State, fullUrl, cookie string) (*int, *int64) {
req.Header.Set("User-Agent", s.UserAgent)
}

if s.RandomUserAgent && s.UserAgent == "" {
b, err := ioutil.ReadFile(s.UserAgentsFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably make this more efficient by reading in the UserAgents file once at the beginning and caching it, rather than on every request. The current way might slow things down a bunch, particularly if there are a lot of agents in the file.

Copy link
Author

Choose a reason for hiding this comment

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

That's true. Where should I put it then?

I thought in SetupDir(), by adding UserAgents []string to the State and using it to store the array of User-Agents.

But in this case, what do we do with the error? Panic? (as I put in the example), and also, this can't be done there (in SetupDir):

s.Printer = PrintDirResult s.Processor = ProcessDirEntry s.Setup = SetupDir

As the ProcessDirEntry is run first in statehelpers.go and it already requires the User-Agent to be set.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think some of the checks belong in ValidateState() or ValidateDirModeState() if they aren't already: https://github.com/OJ/gobuster/blob/master/libgobuster/statehelpers.go

Adding UserAgents []string to the state would be the correct choice I think.

The 'easy' choice is to panic here, the 'better' choice is probably to add support for an 'error' return value to SetupDir as well. Which would also require similar to be done for SetupDns. And then handled somehow wherever they are called from:

Bonus points if you then add an error return to Process: https://github.com/OJ/gobuster/blob/master/libgobuster/state.go#L76

And the associated error check for that at https://github.com/OJ/gobuster/blob/master/main.go#L76

Is ProcessDirEntry actually run in statehelpers? The only line I see is https://github.com/OJ/gobuster/blob/master/libgobuster/statehelpers.go#L53 which I believe is just storing the pointer to the function. It's called elsewhere. So I think this would be fine?

README.md Outdated
* `-r` - follow redirects.
* `-s <status codes>` - comma-separated set of the list of status codes to be deemed a "positive" (default: `200,204,301,302,307`).
* `-suffix <suffix>` - append a custom suffix for directory brute forces.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the RandomUserAgent/UserAgentFile stuff to the README params as well?

lines := strings.Split(string(b), "\n")
var uas []string
for _, line := range lines {
if !strings.HasPrefix(line, "#") && len(line) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not 100%, but you may need to trim pre/post whitespace on here, otherwise a blank line with a few spaces might be counted as a valid UA.

@OJ
Copy link
Owner

OJ commented Feb 19, 2018

Thanks for the efforts you're both putting into this 👍

@@ -159,7 +148,7 @@ func ProcessDirEntry(s *State, word string, resultChan chan<- Result) {
// TODO: Error propagation handling
dirResp, dirSize, err := GoGet(s, s.Url, prefix+word+suffix, s.Cookies)
if err != nil {
panic(err)
//panic(err)
Copy link
Author

Choose a reason for hiding this comment

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

I made this terrible thing here because I realized that the 302 redirect was causing a panic here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which '302 redirect'? Can you give a specific example/site it happens on for debugging.

Definitely will need to actually do something with the error here before it can be merged, otherwise it will fail silently, and that's bad.

Without running it/seeing it in practice I can't really give a good idea of how it should be handled. If it's returning an error where it isn't really an error, then we can probably catch that case and handle it somewhere appropriately.

Copy link
Author

Choose a reason for hiding this comment

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

Funny thing is that now, when trying to reproduce it, I realised that it's happening in the host I was testing it, for a 302 redirect:


Gobuster v1.4.1              OJ Reeves (@TheColonial)
=====================================================
=====================================================
[+] Mode         : dir
[+] Url/Domain   : https://king.com/
[+] Threads      : 10
[+] Wordlist     : /dev/fd/63
[+] Status codes : 200,204,301,302,307
=====================================================
panic: Redirect code: 302

goroutine 15 [running]:
panic(0x22a900, 0xc4203d1ea8)
	/usr/local/go/src/runtime/panic.go:500 +0x1a1
github.com/bberastegui/gobuster/libgobuster.ProcessDirEntry(0xc4200ce000, 0xc4203dc16a, 0x6, 0xc4203109c0)
	/[REDACTED]/go/src/github.com/bberastegui/gobuster/libgobuster/dir.go:151 +0x438
github.com/bberastegui/gobuster/libgobuster.Process.func1(0xc420310960, 0xc4200ce000, 0xc4203109c0, 0xc4203dc1c0)
	/[REDACTED]/go/src/github.com/bberastegui/gobuster/libgobuster/state.go:116 +0x8f
created by github.com/bberastegui/gobuster/libgobuster.Process
	/[REDACTED]/go/src/github.com/bberastegui/gobuster/libgobuster/state.go:122 +0x1a9```

But not on other redirects like 301, for example.

if errorList.ErrorOrNil() == nil && s.RandomUserAgent {
b, err := ioutil.ReadFile(s.UserAgentsFile)
if err != nil {
errorList = multierror.Append(errorList, fmt.Errorf("[!] Error when reading the User-Agents file"))
Copy link
Contributor

Choose a reason for hiding this comment

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

We're masking the cause of the error here by not including it. Could just add the error directly to the errorList, though then you can't add your additional context. At the very least I would include the error message with something like:

fmt.Errorf("[!] Error when reading the User-Agents file: %s", err)

Even that isn't super ideal. Could potentially use %+v to get more context in it? In my own code, I normally would wrap it with another library:

errors.Wrap(err, "[!] Error when reading the User-Agents file")

That would be my best case scenario to do things 'right', but would require adding in another dependency (which personally I don't see as a huge issue).

Copy link
Author

Choose a reason for hiding this comment

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

I like the %+v:


Gobuster v1.4.1              OJ Reeves (@TheColonial)
=====================================================
1 error occurred:

* [!] Error when reading the User-Agents file: open user-agents.txt: no such file or directory```

lines := strings.Split(string(b), "\n")
var uas []string
for _, line := range lines {
if !strings.HasPrefix(line, "#") && len(line) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably call strings.TrimSpace on the line before checking for the prefix:

@@ -170,6 +171,22 @@ func ValidateDirModeState(
}
}

// Read the user agents file
if errorList.ErrorOrNil() == nil && s.RandomUserAgent {
b, err := ioutil.ReadFile(s.UserAgentsFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

ioutil.ReadFile will read the entire file into memory, then you split on newlines, remove comments/etc. It may not matter super much here, but there is a potential that we read in a lot of stuff that is never required, etc.

Following the pattern used for reading the , we could use bufio.NewScanner to read/process the file line by line:

So an implementation in this case might look something like:

for scanner.Scan() {
		if s.Terminate {
			break
		}
		line := strings.TrimSpace(scanner.Text())

		// Skip "comment" (starts with #), as well as empty lines
		if !strings.HasPrefix(word, "#") && len(line) > 0 {
			uas = append(uas, line)
		}
	}

If we wanted to be fancy, we could probably refactor the common functionality between them into a helper function.. But that is probably beyond what is required here.

@OJ
Copy link
Owner

OJ commented Aug 27, 2018

Apologies for the delay on this, can I please request that you rebase your changes on 2.0.0 now that I've finally landed #79? Thank you so much.

@Shaked
Copy link

Shaked commented Mar 2, 2019

Hey @BBerastegui do you need help with this pull request? Seems very useful!

@BBerastegui
Copy link
Author

Hey @Shaked, to be completely honest, I think my knowledge of Go is pretty limited and I think I can't catch up with this now.

Feel free to get my commit and rebase the changes agains the latest version if you want :)

@OJ
Copy link
Owner

OJ commented Mar 22, 2019

I'll see if I can put some time into this later. Sorry for the epic divergence. Dat Open Source Lyfe yo!

@OJ OJ added this to the v3.1 milestone Oct 1, 2019
@firefart
Copy link
Collaborator

This should all be already implemented on the v3.1-working branch

@firefart firefart closed this Jun 20, 2020
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.

None yet

6 participants