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.

Sign up for GitHub

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 support for space in get request #5495

Merged
merged 1 commit into from
Nov 9, 2017
Merged

Add support for space in get request #5495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ https://github.com/elastic/beats/compare/v6.0.0-beta2...master[Check the HEAD di
- Fix http status phrase parsing not allow spaces. {pull}5312[5312]
- Fix missing length check in the PostgreSQL module. {pull}5457[5457]
- Fix panic in ACK handler if event is dropped on blocked queue {issue}5524[5524]
- Fix http parse to allow to parse get request with space in the URI. {pull}5495[5495]

*Winlogbeat*

Expand Down
23 changes: 14 additions & 9 deletions packetbeat/protos/http/http_parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"time"
"unicode"

"github.com/elastic/beats/libbeat/common"
"github.com/elastic/beats/libbeat/common/streambuf"
Expand Down Expand Up @@ -74,8 +75,9 @@ var (

constCRLF = []byte("\r\n")

constClose = []byte("close")
constKeepAlive = []byte("keep-alive")
constClose = []byte("close")
constKeepAlive = []byte("keep-alive")
constHTTPVersion = []byte("HTTP/")

nameContentLength = []byte("content-length")
nameContentType = []byte("content-type")
Expand Down Expand Up @@ -145,7 +147,7 @@ func (*parser) parseHTTPLine(s *stream, m *message) (cont, ok, complete bool) {
}
return false, false, false
}
if bytes.Equal(fline[0:5], []byte("HTTP/")) {
if bytes.Equal(fline[0:5], constHTTPVersion) {
//RESPONSE
m.isRequest = false
version = fline[5:8]
Expand All @@ -160,20 +162,23 @@ func (*parser) parseHTTPLine(s *stream, m *message) (cont, ok, complete bool) {
}
} else {
// REQUEST
slices := bytes.Fields(fline)
if len(slices) != 3 {
afterMethodIdx := bytes.IndexFunc(fline, unicode.IsSpace)
afterRequestURIIdx := bytes.LastIndexFunc(fline, unicode.IsSpace)

// Make sure we have the VERB + URI + HTTP_VERSION
if afterMethodIdx == -1 || afterRequestURIIdx == -1 || afterMethodIdx == afterRequestURIIdx {
if isDebug {
debugf("Couldn't understand HTTP request: %s", fline)
}
return false, false, false
}

m.method = common.NetString(slices[0])
m.requestURI = common.NetString(slices[1])
m.method = common.NetString(fline[:afterMethodIdx])
m.requestURI = common.NetString(fline[afterMethodIdx+1 : afterRequestURIIdx])
Copy link
Contributor

Choose a reason for hiding this comment

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

if there is more than one space between URI and HTTP_VERSION it will be stored into requestURI. I think you should check for that (bytes.Fields did skip consecutive separators)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave some thoughts about it, I don't think we should check for that, if we do it it would mean that we could potentially normalize the query string parameters.

If we look at the following case:

GET /e/b?movie=a brand new world HTTP/1.1 => '/e/b?movie=a brand new world'
GET /e/b?movie=a brand new world  HTTP/1.1 =>  
/elastic/beats?movie=a brand new world '

When we extract the parameters that should gives us 2 distinct values for the movie field: a brand new world and a brand new world (not the space at the end of world)

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I ran a quick check and the server did skip those extra spaces.

Now I was testing again and see that it's more common for servers to return a 400 Bad Request.

So I guess you can just ignore the comment. It's not common at all and spaces are escaped to %20 or + anyway


if bytes.Equal(slices[2][:5], []byte("HTTP/")) {
if bytes.Equal(fline[afterRequestURIIdx+1:afterRequestURIIdx+len(constHTTPVersion)+1], constHTTPVersion) {
m.isRequest = true
version = slices[2][5:]
version = fline[afterRequestURIIdx+len(constHTTPVersion)+1:]
} else {
if isDebug {
debugf("Couldn't understand HTTP version: %s", fline)
Expand Down
39 changes: 39 additions & 0 deletions packetbeat/protos/http/http_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,45 @@ func TestEatBodyChunkedWaitCRLF(t *testing.T) {
}
}

func TestHttpParser_requestURIWithSpace(t *testing.T) {
if testing.Verbose() {
logp.LogInit(logp.LOG_DEBUG, "", false, true, []string{"http", "httpdetailed"})
}

http := httpModForTests(nil)
http.hideKeywords = []string{"password", "pass"}
http.parserConfig.sendHeaders = true
http.parserConfig.sendAllHeaders = true

// Non URL-encoded string, RFC says it should be encoded
data1 := "GET http://localhost:8080/test?password=two secret HTTP/1.1\r\n" +
"Host: www.google.com\r\n" +
"Connection: keep-alive\r\n" +
"User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_4) AppleWebKit/537.1 (KHTML, like Gecko) Chrome/21.0.1180.75 Safari/537.1\r\n" +
"Accept: */*\r\n" +
"X-Chrome-Variations: CLa1yQEIj7bJAQiftskBCKS2yQEIp7bJAQiptskBCLSDygE=\r\n" +
"Referer: http://www.google.com/\r\n" +
"Accept-Encoding: gzip,deflate,sdch\r\n" +
"Accept-Language: en-US,en;q=0.8\r\n" +
"Content-Type: application/x-www-form-urlencoded\r\n" +
"Content-Length: 23\r\n" +
"Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.3\r\n" +
"Cookie: PREF=ID=6b67d166417efec4:U=69097d4080ae0e15:FF=0:TM=1340891937:LM=1340891938:S=8t97UBiUwKbESvVX; NID=61=sf10OV-t02wu5PXrc09AhGagFrhSAB2C_98ZaI53-uH4jGiVG_yz9WmE3vjEBcmJyWUogB1ZF5puyDIIiB-UIdLd4OEgPR3x1LHNyuGmEDaNbQ_XaxWQqqQ59mX1qgLQ\r\n" +
"\r\n" +
"username=ME&pass=twosecret"
tp := newTestParser(http, data1)

msg, ok, complete := tp.parse()
assert.True(t, ok)
assert.True(t, complete)
rawMsg := tp.stream.data[tp.stream.message.start:tp.stream.message.end]
path, params, err := http.extractParameters(msg, rawMsg)
assert.Nil(t, err)
assert.Equal(t, "/test", path)
assert.Equal(t, string(msg.requestURI), "http://localhost:8080/test?password=two secret")
assert.False(t, strings.Contains(params, "two secret"))
}

func TestHttpParser_censorPasswordURL(t *testing.T) {
if testing.Verbose() {
logp.LogInit(logp.LOG_DEBUG, "", false, true, []string{"http", "httpdetailed"})
Expand Down