Specify arbitrary HTTP headers

[mzpqnxow]

This isn't much more than a rebase of #68 which was briefly discussed. I've tested this and have been using it for some time

The implementation is not the prettiest thing, from a user perspective- there are two command-line flags. Each of them is treated as a CSV.

zgrab2 http ... --custom-headers-names='Cookie,Content-Type' --custom-headers-values='somecookie=1234;,application/json'

It's not difficult to make this panic by not providing a matching amount of names/values. I'm also not entirely sure how to deal properly with values that may contain commas (is that legal in the HTTP RFC? I'm not sure)

So I'm not sure if you want to accept this as is, but it works for simple (practical/useful) cases- things like setting cookies, authorization, content-type, etc.

@zakird, @corny, @justinbastress if you have any comments on what you'd like to see changed, let me know. I can't necessarily commit to spending time on it but I'll do my best. I've been using it for a while so I figured it would be polite to send it upstream, even if it wasn't necessarily meeting requirements you may have

Thanks @zakird for doing the work on this, I mainly just re-based this from a recent master

[zakird]

This looks good except for line 440, which seems to be at odds with the comment above.

…

On Fri, Dec 4, 2020 at 12:28 PM AG ***@***.***> wrote: This isn't much more than a rebase of #68 <#68> which was briefly discussed. I've tested this and have been using it for some time The implementation is not the prettiest thing, from a user perspective- there are two command-line flags. Each of them is treated as a CSV. zgrab2 http ... --custom-headers-names='Cookie,Content-Type' --custom-headers-values='somecookie=1234;,application/json' It's not difficult to make this panic by not providing a matching amount of names/values. I'm also not entirely sure how to deal properly with values that may contain commas (is that legal in the HTTP RFC? I'm not sure) So I'm not sure if you want to accept this as is, but it works for simple (practical/useful) cases- things like setting cookies, authorization, content-type, etc. @zakird <https://github.com/zakird>, @corny <https://github.com/corny>, @justinbastress <https://github.com/justinbastress> if you have any comments on what you'd like to see changed, let me know. I can't necessarily commit to spending time on it but I'll do my best. I've been using it for a while so I figured it would be polite to send it upstream, even if it wasn't necessarily meeting requirements you may have Thanks @zakird <https://github.com/zakird> for doing the work on this, I mainly just re-based this from a recent master ------------------------------ You can view, comment on, or merge this pull request online at: #284 Commit Summary - Merge pull request #1 from zmap/master - Merge pull request #2 from zmap/master - Add support for specifying arbitrary HTTP headers File Changes - *M* modules/http/scanner.go <https://github.com/zmap/zgrab2/pull/284/files#diff-0b818b92a55aa2d7d4d33d9f9519ef9b72519067a9c940f8170b07191eb78090> (56) Patch Links: - https://github.com/zmap/zgrab2/pull/284.patch - https://github.com/zmap/zgrab2/pull/284.diff — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#284>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABREUHGVXSVVT52MPRRR33STFBARANCNFSM4UN5MTLQ> .

[mzpqnxow]

This looks good except for line 440, which seems to be at odds with the comment above.
…
On Fri, Dec 4, 2020 at 12:28 PM AG @.***> wrote: This isn't much more than a rebase of #68 <#68> which was briefly discussed. I've tested this and have been using it for some time The implementation is not the prettiest thing, from a user perspective- there are two command-line flags. Each of them is treated as a CSV. zgrab2 http ... --custom-headers-names='Cookie,Content-Type' --custom-headers-values='somecookie=1234;,application/json' It's not difficult to make this panic by not providing a matching amount of names/values. I'm also not entirely sure how to deal properly with values that may contain commas (is that legal in the HTTP RFC? I'm not sure) So I'm not sure if you want to accept this as is, but it works for simple (practical/useful) cases- things like setting cookies, authorization, content-type, etc. @zakird https://github.com/zakird, @corny https://github.com/corny, @justinbastress https://github.com/justinbastress if you have any comments on what you'd like to see changed, let me know. I can't necessarily commit to spending time on it but I'll do my best. I've been using it for a while so I figured it would be polite to send it upstream, even if it wasn't necessarily meeting requirements you may have Thanks @zakird https://github.com/zakird for doing the work on this, I mainly just re-based this from a recent master ------------------------------ You can view, comment on, or merge this pull request online at: #284 Commit Summary - Merge pull request #1 from zmap/master - Merge pull request #2 from zmap/master - Add support for specifying arbitrary HTTP headers File Changes - M modules/http/scanner.go https://github.com/zmap/zgrab2/pull/284/files#diff-0b818b92a55aa2d7d4d33d9f9519ef9b72519067a9c940f8170b07191eb78090 (56) Patch Links: - https://github.com/zmap/zgrab2/pull/284.patch - https://github.com/zmap/zgrab2/pull/284.diff — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#284>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABREUHGVXSVVT52MPRRR33STFBARANCNFSM4UN5MTLQ .

I actually just glanced at 440 now and you're almost certainly right. I remember when I initially looked at this (it was a while back) I had some confusion about the Accept header and whether it should be kept or not.. I think I got confused because Accept was set somewhere else in certain cases, or maybe in a higher level golang API? Rather than guessing, I should just re-familiarize myself with it and at the very least fix the comment

Then possibly consult on what the correct behavior should be

Thanks for catching this

[mzpqnxow]

all set @zakird

I did something I don't normally like to do, to avoid confusing/mixed up looking commits, but because it was small I just went ahead- I changed 4 variable names and one function declaration to please golint

Aside from fixing the incorrect comment, I also added a handful of additional sanity checking for the custom header names- for the Host and Content-Length headers, which get overwritten by the HTTP library anyway, it will tell the user and bail. For User-Agent header, it will tell you to use --user-agent instead, and bail.

Also, some of the existing sanity checks were returning Errors.new() which doesn't have any effect since there isn't a return value check in bin/bin.go:ZGrab2Main. I think the changes I made make it behave as it should, but correct me if I'm wrong and I can change them back

One other thing- there was a typo if len(headerValues) != len(headerValues) that I fixed to avoid an indexing panic.

I did a bit of testing and things look good. The only comment I would add onto this would be that there may be other headers that are (effectively) immutable, aside from Host and Content-Length, but I didn't have any interesting in trying all of the header values in the golang HTTP library, which I'm sure you can understand, at least for now :)

EDIT: For Content-Length, to clarify the reason I decided to disallow specifying it as a custom header is because there are two behaviors depending on the verb/method, both render it useless (unless more invasive changes are made to the http module)

1. If Content-Length isn't verb appropriate (e.g. GET) the http module will drop it before making the request
2. If Content-Length is verb appropriate (e.g. POST) the http module will override it with the correct value, which until a body is supported, will always be 0

[mzpqnxow]

@zakird, I just updated this branch slightly. Just today I had to use a fairly exotic header value to test something and had a really difficult time trying to properly quote/escape the value. I assumed that simply quoting the value would work, e.g.:

--custom-header-name=Content-Type --custom-header-value='"Has a, comma"'

But had no luck with this. I ended up adding a flag to allow the delimiter to be specified on the command-line as an additional flag, --custom-header-delimeter, and then specifying | as the delimeter (the value I needed to test had commas, single quotes, percent signs, and just about everything else you could imagine- I won't get into why)

Because it's illegal to to have commas in the header names I didn't apply the delimeter to the name argument. So those are still comma delimited. Which, now that I think about it, is either just plain wrong, or requires the delimeter flag to be renamed and more correctly described to only pertain to the custom header value and not the name.

Questions for you are:

1. Are you OK with having this extra flag? I realize that there should be some solution for properly quoting values with commas that doesn't involve hacking around by using an entirely different delimiter, but I couldn't figure it out.
2. If so, is it preferred for the explicitly specified delimeter to apply to both the header name and header value arguments?

Either way this will require a change- either to make it effective on both the name and value, or to update the description of the flag so that it's clear it only operates on the value

EDIT- forgot to show the example usage:

zgrab2 http -p 443 --cipher-suite=portable --custom-headers-names='Content-Type' --custom-headers-values='%{#context['some.java.thing'].addHeader('X-Injection-Test','SomeValue')}.multipart/form-data' --max-redirects=5 --use-https --custom-headers-delimeter='|'

One would think that simply using the following would have handled the comma:

zgrab2 http -p 443 --cipher-suite=portable --custom-headers-names='Content-Type' --custom-headers-values='"%{#context['some.java.thing'].addHeader('X-Injection-Test','SomeValue')}.multipart/form-data"' --max-redirects=5 --use-https

But I had no luck with that. Maybe I'm just being dumb here and someone can demonstrate the correct way to escape or quote this specific example, and I can throw away this flag as unnecessary...

[zakird]

I think that's fine. There isn't an easy clean alternative. I would prefer to have the delimiter apply to both, but that isn't a strongly held opinion.

[mzpqnxow]

I think that's fine. There isn't an easy clean alternative. I would prefer to have the delimiter apply to both, but that isn't a strongly held opinion.

I went ahead and fixed it and updated the branch/PR.. it was actually driving me a bit batty having the names be excluded from the custom separator and it was only a one line change. Tests ok.

[zakird]

Looks good. I'll merge when tests pass.

[mzpqnxow]

FYI- from what I can tell, looks like the pop3 schema validation is failing:

One or more schema validations failed: scan failure(protocol-error)@pop3/banner.json, scan failure(protocol-error)@pop3/banner.quit.json, scan failure(protocol-error)@pop3/help.banner.quit.json, scan failure(protocol-error)@pop3/noop.help.banner.quit.json
Makefile:32: recipe for target 'integration-test' failed
make: *** [integration-test] Error 1

I don't think this is related to this commit (I hope it isn't hah!) but I'll take a look and see if I can save you the time of weeding through it when I get a chance

[engn33r]

First, thanks for this PR. To weigh in with my own data point, I used this PR and got the expected results. One use case I came upon that doesn't currently seem supported is adding an "Origin" header to dynamically match the domain that the request is being sent to. I have a fork where I've tried to implement this feature, but I'll wait until this PR is merged before I introduce a new PR into the mix.

[engn33r]

And for what it's worth, when I tried running make clean zgrab2 test, I didn't get any error on the ExampleAttributeByteString http test - perhaps there is another test failing with this PR that I failed to notice (I'm no pro). It looks like all TravisCI builds since June 2020 have failed with an integration-test error similar to the one that @mzpqnxow mentioned on Dec 18.

[mzpqnxow]

@zakird poke on this :)

I haven't checked the CI again recently, but it seemed to be failing due to something nothing at all related to this change

FYI, I have been using this branch on my own for quite some time now, along with the proposed PR #308 which has been a really useful feature for me. That one is pending feedback

[zakird]

We need to address the sorry state of CI in this project, but it shouldn't block the merging of this.