From bb794753634f887929ebaefcaff1429324b6ad02 Mon Sep 17 00:00:00 2001 From: Adrian Serrano Date: Wed, 14 Feb 2018 22:43:49 +0100 Subject: [PATCH 1/4] AMQP: Fix panic during parse of partial message (#6384) A message with a client header consisting on a partial frame (not all data received for this frame) could result in a panic. --- CHANGELOG.asciidoc | 1 + packetbeat/protos/amqp/amqp_parser.go | 5 ++++- packetbeat/protos/amqp/amqp_test.go | 22 ++++++++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 1a217be37b9..6d5617a84f5 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -308,6 +308,7 @@ https://github.com/elastic/beats/compare/v6.0.1...v6.1.0[View commits] - Fix http parse to allow to parse get request with space in the URI. {pull}5495[5495] - Fix mysql SQL parser to trim `\r` from Windows Server `SELECT\r\n\t1`. {pull}5572[5572] - Fix corruption when parsing repeated headers in an HTTP request or response. {pull}6325[6325] +- Fix panic when parsing partial AMQP messages. {pull}6384[6384] *Winlogbeat* diff --git a/packetbeat/protos/amqp/amqp_parser.go b/packetbeat/protos/amqp/amqp_parser.go index 11ff24e866a..7141f495b50 100644 --- a/packetbeat/protos/amqp/amqp_parser.go +++ b/packetbeat/protos/amqp/amqp_parser.go @@ -72,7 +72,10 @@ func isProtocolHeader(data []byte) (isHeader bool, version string) { //func to read a frame header and check if it is valid and complete func readFrameHeader(data []byte) (ret *amqpFrame, err bool) { var frame amqpFrame - + if len(data) < 8 { + logp.Warn("Partial frame header, waiting for more data") + return nil, false + } frame.size = binary.BigEndian.Uint32(data[3:7]) if len(data) < int(frame.size)+8 { logp.Warn("Frame shorter than declared size, waiting for more data") diff --git a/packetbeat/protos/amqp/amqp_test.go b/packetbeat/protos/amqp/amqp_test.go index 0299ee22334..e435958d15e 100644 --- a/packetbeat/protos/amqp/amqp_test.go +++ b/packetbeat/protos/amqp/amqp_test.go @@ -89,6 +89,28 @@ func TestAmqp_FrameSize(t *testing.T) { } } +// Test that the parser doesn't panic on a partial message that includes +// a client header +func TestAmqp_PartialFrameSize(t *testing.T) { + logp.TestingSetup(logp.WithSelectors("amqp", "amqpdetailed")) + + _, amqp := amqpModForTests() + + //incomplete frame + data, err := hex.DecodeString("414d515000060606010000000000") + assert.Nil(t, err) + + stream := &amqpStream{data: data, message: new(amqpMessage)} + ok, complete := amqp.amqpMessageParser(stream) + + if !ok { + t.Errorf("Parsing should not raise an error") + } + if complete { + t.Errorf("message should not be complete") + } +} + func TestAmqp_WrongShortStringSize(t *testing.T) { logp.TestingSetup(logp.WithSelectors("amqp", "amqpdetailed")) From d397f2dd3b7359f1e3646b057ada5e792b6cf7c5 Mon Sep 17 00:00:00 2001 From: Adrian Serrano Date: Thu, 5 Apr 2018 10:49:02 +0200 Subject: [PATCH 2/4] Fix out of bounds access to slice in MongoDB parser (#6256) * Fix out of bounds access to slice in MongoDB parser Ignore MongoDB message and drop the TCP stream if a malformed query / response is received, instead of logging a panic. Closes #5188 * Update CHANGELOG --- CHANGELOG.asciidoc | 1 + packetbeat/protos/mongodb/mongodb_parser.go | 3 +++ packetbeat/protos/mongodb/mongodb_test.go | 26 +++++++++++++++++++++ 3 files changed, 30 insertions(+) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 6d5617a84f5..b034cd010b8 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -309,6 +309,7 @@ https://github.com/elastic/beats/compare/v6.0.1...v6.1.0[View commits] - Fix mysql SQL parser to trim `\r` from Windows Server `SELECT\r\n\t1`. {pull}5572[5572] - Fix corruption when parsing repeated headers in an HTTP request or response. {pull}6325[6325] - Fix panic when parsing partial AMQP messages. {pull}6384[6384] +- Fix out of bounds access to slice in MongoDB parser. {pull}6256[6256] *Winlogbeat* diff --git a/packetbeat/protos/mongodb/mongodb_parser.go b/packetbeat/protos/mongodb/mongodb_parser.go index 6567c250ee0..4631d518167 100644 --- a/packetbeat/protos/mongodb/mongodb_parser.go +++ b/packetbeat/protos/mongodb/mongodb_parser.go @@ -341,6 +341,9 @@ func (d *decoder) readDocument() (bson.M, error) { start := d.i documentLength, err := d.readInt32() d.i = start + documentLength + if len(d.in) < d.i { + return nil, errors.New("document out of bounds") + } documentMap := bson.M{} diff --git a/packetbeat/protos/mongodb/mongodb_test.go b/packetbeat/protos/mongodb/mongodb_test.go index edbebb2301f..8493e31dc43 100644 --- a/packetbeat/protos/mongodb/mongodb_test.go +++ b/packetbeat/protos/mongodb/mongodb_test.go @@ -333,3 +333,29 @@ func TestMaxDocSize(t *testing.T) { assert.Equal(t, "\"1234 ...\n\"123\"\n\"12\"", res["response"]) } + +// Test for a (recovered) panic parsing document length in request/response messages +func TestDocumentLengthBoundsChecked(t *testing.T) { + logp.TestingSetup(logp.WithSelectors("mongodb", "mongodbdetailed")) + + _, mongodb := mongodbModForTests() + + // request and response from tests/pcaps/mongo_one_row.pcap + reqData, err := hex.DecodeString( + // Request message with out of bounds document + "320000000a000000ffffffffd4070000" + + "00000000746573742e72667374617572" + + "616e7473000000000001000000" + + // Document length (including itself) + "06000000" + + // Document (1 byte instead of 2) + "00") + assert.Nil(t, err) + + tcptuple := testTCPTuple() + req := protos.Packet{Payload: reqData} + private := protos.ProtocolData(new(mongodbConnectionData)) + + private = mongodb.Parse(&req, tcptuple, 0, private) + assert.NotNil(t, private, "mongodb parser recovered from a panic") +} From 9ff054e3bb2f8ee5ff7fed3d324d45261fb83508 Mon Sep 17 00:00:00 2001 From: Adrian Serrano Date: Mon, 19 Mar 2018 16:53:18 +0100 Subject: [PATCH 3/4] Updated dependency `github.com/tsg/gopacket` (#6583) Fixes packetbeat termination problems with both af_packet and pcap captures. Fixes #6535 --- CHANGELOG.asciidoc | 1 + NOTICE.txt | 2 +- .../tsg/gopacket/afpacket/afpacket.go | 16 ++++-- vendor/github.com/tsg/gopacket/layers/lldp.go | 3 +- vendor/github.com/tsg/gopacket/pcap/pcap.go | 18 ++++++- .../tsg/gopacket/pcap/pcap_poll_common.go | 18 +++++++ .../tsg/gopacket/pcap/pcap_poll_linux.go | 52 +++++++++++++++++++ vendor/vendor.json | 4 +- 8 files changed, 105 insertions(+), 9 deletions(-) create mode 100644 vendor/github.com/tsg/gopacket/pcap/pcap_poll_common.go create mode 100644 vendor/github.com/tsg/gopacket/pcap/pcap_poll_linux.go diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index b034cd010b8..7fe97248bad 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -310,6 +310,7 @@ https://github.com/elastic/beats/compare/v6.0.1...v6.1.0[View commits] - Fix corruption when parsing repeated headers in an HTTP request or response. {pull}6325[6325] - Fix panic when parsing partial AMQP messages. {pull}6384[6384] - Fix out of bounds access to slice in MongoDB parser. {pull}6256[6256] +- Fix sniffer hanging on exit under Linux. {pull}6535[6535] *Winlogbeat* diff --git a/NOTICE.txt b/NOTICE.txt index 6b80ef2b4bf..9b968540ef2 100644 --- a/NOTICE.txt +++ b/NOTICE.txt @@ -3259,7 +3259,7 @@ OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. -------------------------------------------------------------------- Dependency: github.com/tsg/gopacket -Revision: 8e703b9968693c15f25cabb6ba8be4370cf431d0 +Revision: f289b3ea3e41a01b2822be9caf5f40c01fdda05c License type (autodetected): BSD-3-Clause ./vendor/github.com/tsg/gopacket/LICENSE: -------------------------------------------------------------------- diff --git a/vendor/github.com/tsg/gopacket/afpacket/afpacket.go b/vendor/github.com/tsg/gopacket/afpacket/afpacket.go index ed0c5ce73a7..fe622918a30 100644 --- a/vendor/github.com/tsg/gopacket/afpacket/afpacket.go +++ b/vendor/github.com/tsg/gopacket/afpacket/afpacket.go @@ -16,14 +16,16 @@ package afpacket import ( "errors" "fmt" - "github.com/tsg/gopacket" - "github.com/tsg/gopacket/layers" - "github.com/tsg/gopacket/pcap" "net" "runtime" "sync" + "syscall" "time" "unsafe" + + "github.com/tsg/gopacket" + "github.com/tsg/gopacket/layers" + "github.com/tsg/gopacket/pcap" ) /* @@ -310,7 +312,13 @@ func (h *TPacket) pollForFirstPacket(hdr header) error { h.pollset.events = C.POLLIN h.pollset.revents = 0 timeout := C.int(h.opts.timeout / time.Millisecond) - _, err := C.poll(&h.pollset, 1, timeout) + n, err := C.poll(&h.pollset, 1, timeout) + if n == 0 { + /* propagate timeout when no packets are available + otherwise it will loop forever until a packet + is received. */ + return syscall.EINTR + } h.stats.Polls++ if err != nil { return err diff --git a/vendor/github.com/tsg/gopacket/layers/lldp.go b/vendor/github.com/tsg/gopacket/layers/lldp.go index c77bd981ab3..613d382084c 100644 --- a/vendor/github.com/tsg/gopacket/layers/lldp.go +++ b/vendor/github.com/tsg/gopacket/layers/lldp.go @@ -9,6 +9,7 @@ package layers import ( "encoding/binary" "fmt" + "github.com/tsg/gopacket" ) @@ -737,7 +738,7 @@ func decodeLinkLayerDiscovery(data []byte, p gopacket.PacketBuilder) error { for len(vData) > 0 { nbit := vData[0] & 0x01 t := LLDPTLVType(vData[0] >> 1) - val := LinkLayerDiscoveryValue{Type: t, Length: uint16(nbit<<8 + vData[1])} + val := LinkLayerDiscoveryValue{Type: t, Length: uint16(nbit)<<8 + uint16(vData[1])} if val.Length > 0 { val.Value = vData[2 : val.Length+2] } diff --git a/vendor/github.com/tsg/gopacket/pcap/pcap.go b/vendor/github.com/tsg/gopacket/pcap/pcap.go index f5612e6ffd2..c869bcc1599 100644 --- a/vendor/github.com/tsg/gopacket/pcap/pcap.go +++ b/vendor/github.com/tsg/gopacket/pcap/pcap.go @@ -131,6 +131,10 @@ type Handle struct { // huge memory hit, so to handle that we store them here instead. pkthdr *C.struct_pcap_pkthdr buf_ptr *C.u_char + // This is required to poll the pcap handle for incoming packets, due to + // newer Linux kernels supporting TPACKET_V3 not starting the timeout until + // the first packet is received. + packetPoller *packetPoll } // Stats contains statistics on how many packets were handled by a pcap handle, @@ -206,10 +210,14 @@ func OpenLive(device string, snaplen int32, promisc bool, timeout time.Duration) dev := C.CString(device) defer C.free(unsafe.Pointer(dev)) - p.cptr = C.pcap_open_live(dev, C.int(snaplen), pro, timeoutMillis(timeout), buf) + timeoutMs := timeoutMillis(timeout) + p.cptr = C.pcap_open_live(dev, C.int(snaplen), pro, timeoutMs, buf) if p.cptr == nil { return nil, errors.New(C.GoString(buf)) } + if !p.blockForever { + p.packetPoller = NewPacketPoll(p.cptr, timeoutMs) + } return p, nil } @@ -316,6 +324,9 @@ func (a activateError) Error() string { func (p *Handle) getNextBufPtrLocked(ci *gopacket.CaptureInfo) error { var result NextError for { + if !p.packetPoller.AwaitForPackets() { + return NextErrorTimeoutExpired + } result = NextError(C.pcap_next_ex(p.cptr, &p.pkthdr, &p.buf_ptr)) if p.blockForever && result == NextErrorTimeoutExpired { continue @@ -544,6 +555,11 @@ func findalladdresses(addresses *_Ctype_struct_pcap_addr) (retval []InterfaceAdd for curaddr := addresses; curaddr != nil; curaddr = (*_Ctype_struct_pcap_addr)(curaddr.next) { var a InterfaceAddress var err error + // In case of a tun device on Linux the link layer has no curaddr.addr. + // Do not crash trying to check the family type. + if curaddr.addr == nil { + continue + } if a.IP, err = sockaddr_to_IP((*syscall.RawSockaddr)(unsafe.Pointer(curaddr.addr))); err != nil { continue } diff --git a/vendor/github.com/tsg/gopacket/pcap/pcap_poll_common.go b/vendor/github.com/tsg/gopacket/pcap/pcap_poll_common.go new file mode 100644 index 00000000000..9d96f485a58 --- /dev/null +++ b/vendor/github.com/tsg/gopacket/pcap/pcap_poll_common.go @@ -0,0 +1,18 @@ +// +build !linux + +package pcap + +/* +#include +*/ +import "C" + +type packetPoll struct{} + +func NewPacketPoll(_ *C.pcap_t, _ C.int) *packetPoll { + return nil +} + +func (t *packetPoll) AwaitForPackets() bool { + return true +} diff --git a/vendor/github.com/tsg/gopacket/pcap/pcap_poll_linux.go b/vendor/github.com/tsg/gopacket/pcap/pcap_poll_linux.go new file mode 100644 index 00000000000..30ec8de72f0 --- /dev/null +++ b/vendor/github.com/tsg/gopacket/pcap/pcap_poll_linux.go @@ -0,0 +1,52 @@ +// +build linux + +package pcap + +/* +#include +#include +#include +*/ +import "C" +import "syscall" + +// packetPoll holds all the parameters required to use poll(2) on the pcap +// file descriptor. +type packetPoll struct { + pollfd C.struct_pollfd + timeout C.int +} + +func captureIsTPacketV3(fildes int) bool { + version, err := syscall.GetsockoptInt(fildes, syscall.SOL_PACKET, C.PACKET_VERSION) + return err == nil && version == C.TPACKET_V3 +} + +// NewPacketPoll returns a new packetPoller if the pcap handle requires it +// in order to timeout effectively when no packets are received. This is only +// necessary when TPACKET_V3 interface is used to receive packets. +func NewPacketPoll(ptr *C.pcap_t, timeout C.int) *packetPoll { + fildes := C.pcap_fileno(ptr) + if !captureIsTPacketV3(int(fildes)) { + return nil + } + return &packetPoll{ + pollfd: C.struct_pollfd{ + fd: fildes, + events: C.POLLIN, + revents: 0, + }, + timeout: timeout, + } +} + +func (t *packetPoll) AwaitForPackets() bool { + if t != nil { + t.pollfd.revents = 0 + // block until the capture file descriptor is readable or a timeout + // happens. + n, err := C.poll(&t.pollfd, 1, t.timeout) + return err != nil || n != 0 + } + return true +} diff --git a/vendor/vendor.json b/vendor/vendor.json index c1f4d7d194d..51423b2afaa 100644 --- a/vendor/vendor.json +++ b/vendor/vendor.json @@ -971,8 +971,8 @@ { "checksumSHA1": "M0S9278lG9Fztu+ZUsLUi40GDJU=", "path": "github.com/tsg/gopacket", - "revision": "8e703b9968693c15f25cabb6ba8be4370cf431d0", - "revisionTime": "2016-08-17T18:24:57Z" + "revision": "f289b3ea3e41a01b2822be9caf5f40c01fdda05c", + "revisionTime": "2018-03-16T21:03:30Z" }, { "checksumSHA1": "STY8i3sZLdZfFcKiyOdpV852nls=", From ad22a6922d5391dfd81dc60dd7b0a1bb4604f5f1 Mon Sep 17 00:00:00 2001 From: Adrian Serrano Date: Thu, 5 Apr 2018 10:52:00 +0200 Subject: [PATCH 4/4] Fix minor panic in http parser (#6750) There was a bounds check error in parsing HTTP responses. A malformed response line in the form "HTTP/1.1\r\n" would cause a panic when parsed. Related to #6409 --- CHANGELOG.asciidoc | 1 + packetbeat/protos/http/http_parser.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 7fe97248bad..e134984fa31 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -311,6 +311,7 @@ https://github.com/elastic/beats/compare/v6.0.1...v6.1.0[View commits] - Fix panic when parsing partial AMQP messages. {pull}6384[6384] - Fix out of bounds access to slice in MongoDB parser. {pull}6256[6256] - Fix sniffer hanging on exit under Linux. {pull}6535[6535] +- Fix bounds check error in http parser causing a panic. {pull}6750[6750] *Winlogbeat* diff --git a/packetbeat/protos/http/http_parser.go b/packetbeat/protos/http/http_parser.go index 1f8938110c7..bb4e2f754fe 100644 --- a/packetbeat/protos/http/http_parser.go +++ b/packetbeat/protos/http/http_parser.go @@ -141,7 +141,7 @@ func (*parser) parseHTTPLine(s *stream, m *message) (cont, ok, complete bool) { var version []byte var err error fline := s.data[s.parseOffset:i] - if len(fline) < 8 { + if len(fline) < 9 { if isDebug { debugf("First line too small") }