Fixed crashing bug when parsing invalid messages

parser.js

@@ -36,6 +36,7 @@ Parser.prototype._newPacket = function () {
 }
 
 Parser.prototype.parse = function (buf) {
+
   this._list.append(buf)
 
   while ((this.packet.length != -1 || this._list.length > 0) &&
@@ -163,14 +164,27 @@ Parser.prototype._parseConnect = function () {
   if (protocolId === null)
     return this.emit('error', new Error('cannot parse protocol id'))
 
+  if (protocolId != 'MQTT' && protocolId != 'MQIsdp') {
+
+    return this.emit('error', new Error('invalid protocol id'))
+  }
+
   packet.protocolId = protocolId
 
   // Parse constants version number
-  if(this._pos > this._list.length)
+  if(this._pos >= this._list.length)
     return this.emit('error', new Error('packet too short'))
 
   packet.protocolVersion = this._list.readUInt8(this._pos)
+
+  if(packet.protocolVersion != 3 && packet.protocolVersion != 4) {
+
+    return this.emit('error', new Error('invalid protocol version'))
+  }
+
   this._pos++
+  if(this._pos >= this._list.length)
+    return this.emit('error', new Error('packet too short'))
 
   // Parse connect flags
   flags.username  = (this._list.readUInt8(this._pos) & constants.USERNAME_MASK)
@@ -335,7 +349,7 @@ Parser.prototype._parseString = function(maybeBuffer) {
   var length = this._parseNum()
     , result
 
-  if(length === -1 || length + this._pos > this._list.length)
+  if(length === -1 || length + this._pos > this._list.length || length + this._pos > this.packet.length)
     return null
 
   result = this._list.toString('utf8', this._pos, this._pos + length)

test.js

@@ -879,3 +879,68 @@ test('support cork', function (t) {
 
   dest.end()
 })
+
+// the following test case was designed after experiencing errors
+// when trying to connect with tls on a non tls mqtt port
+// the specific behaviour is:
+// - first byte suggests this is a connect message
+// - second byte suggests message length to be smaller than buffer length
+//   thus payload processing starts
+// - the first two bytes suggest a protocol identifier string length
+//   that leads the parser pointer close to the end of the buffer
+// - when trying to read further connect flags the buffer produces
+//   a "out of range" Error
+//
+testParseError('packet too short', new Buffer([
+  16, 9,
+  0, 6,
+  77, 81, 73, 115, 100, 112,
+  3
+]))
+
+// CONNECT Packets that show other protocol ids than
+// the valid values MQTT and MQIsdp should cause an error
+// those packets are a hint that this is not a mqtt connection
+testParseError('invalid protocol id', new Buffer([
+  16, 18,
+  0, 6,
+  65,65,65,65,65,65, // AAAAAA
+  3, // protocol version
+  0, // connect flags
+  0, 10, // keep alive
+  0, 4, //Client id length
+  116, 101, 115, 116 // Client id
+]))
+
+// CONNECT Packets that contain an unsupported protocol version 
+// flag (i.e. not `3` or `4`) should cause an error
+testParseError('invalid protocol version', new Buffer([
+  16, 18,
+  0, 6,
+  77, 81, 73, 115, 100, 112, // Protocol id
+  1, // protocol version
+  0, // connect flags
+  0, 10, // keep alive
+  0, 4, //Client id length
+  116, 101, 115, 116 // Client id
+]))
+
+// when a packet contains a string in the variable header and the
+// given string length of this exceeds the overall length of the packet that 
+// was specified in the fixed header, parsing must fail.
+// this case simulates this behavior with the protocol id string of the
+// CONNECT packet. The fixed header suggests a remaining length of 8 bytes
+// which would be exceeded by the string length of 15
+// in this case, a protocol id parse error is expected
+testParseError('cannot parse protocol id', new Buffer([
+  16, 8, // fixed header
+  0, 15, // string length 15 --> 15 > 8 --> error!
+  77, 81, 73, 115, 100, 112,
+  77, 81, 73, 115, 100, 112,
+  77, 81, 73, 115, 100, 112,
+  77, 81, 73, 115, 100, 112,
+  77, 81, 73, 115, 100, 112,
+  77, 81, 73, 115, 100, 112,
+  77, 81, 73, 115, 100, 112,
+  77, 81, 73, 115, 100, 112
+]))