Make name.decode stricter (#79)

* Add tests for name decoding corner cases

* Modify name.decode to throw an error in the following cases:
 * Not enough data for reading the full label
 * The label is too long (over 253 characters when dots are included)
  * A label must be either <= 63 bytes or a pointer
  * Pointers can only point to prior data (see RFC 1035, section 4.1.4)

In addition pointer jumps don't add extra dots in the names anymore.

* Make name_decoding tests more specific

* Make name.decode non-recursive

* Ensure name.decode can read the label header

* Fix name.decode error messages

Author: jviide

index.js

@@ -46,33 +46,53 @@ name.decode = function (buf, offset) {
   if (!offset) offset = 0
 
   const list = []
-  const oldOffset = offset
-  let len = buf[offset++]
-
-  if (len === 0) {
-    name.decode.bytes = 1
-    return '.'
-  }
-  if (len >= 0xc0) {
-    const res = name.decode(buf, buf.readUInt16BE(offset - 1) - 0xc000)
-    name.decode.bytes = 2
-    return res
-  }
+  let oldOffset = offset
+  let totalLength = 0
+  let consumedBytes = 0
+  let jumped = false
+
+  while (true) {
+    if (offset >= buf.length) {
+      throw new Error('Cannot decode name (buffer overflow)')
+    }
+    const len = buf[offset++]
+    consumedBytes += jumped ? 0 : 1
 
-  while (len) {
-    if (len >= 0xc0) {
-      list.push(name.decode(buf, buf.readUInt16BE(offset - 1) - 0xc000))
-      offset++
+    if (len === 0) {
       break
+    } else if ((len & 0xc0) === 0) {
+      if (offset + len > buf.length) {
+        throw new Error('Cannot decode name (buffer overflow)')
+      }
+      totalLength += len + 1
+      if (totalLength > 254) {
+        throw new Error('Cannot decode name (name too long)')
+      }
+      list.push(buf.toString('utf-8', offset, offset + len))
+      offset += len
+      consumedBytes += jumped ? 0 : len
+    } else if ((len & 0xc0) === 0xc0) {
+      if (offset + 1 > buf.length) {
+        throw new Error('Cannot decode name (buffer overflow)')
+      }
+      const jumpOffset = buf.readUInt16BE(offset - 1) - 0xc000
+      if (jumpOffset >= oldOffset) {
+        // Allow only pointers to prior data. RFC 1035, section 4.1.4 states:
+        // "[...] an entire domain name or a list of labels at the end of a domain name
+        // is replaced with a pointer to a prior occurance (sic) of the same name."
+        throw new Error('Cannot decode name (bad pointer)')
+      }
+      offset = jumpOffset
+      oldOffset = jumpOffset
+      consumedBytes += jumped ? 0 : 1
+      jumped = true
+    } else {
+      throw new Error('Cannot decode name (bad label)')
     }
-
-    list.push(buf.toString('utf-8', offset, offset + len))
-    offset += len
-    len = buf[offset++]
   }
 
-  name.decode.bytes = offset - oldOffset
-  return list.join('.')
+  name.decode.bytes = consumedBytes
+  return list.length === 0 ? '.' : list.join('.')
 }
 
 name.decode.bytes = 0

test.js

@@ -304,6 +304,50 @@ tape('name_encoding', function (t) {
   t.end()
 })
 
+tape('name_decoding', function (t) {
+  // The two most significant bits of a valid label header must be either both zero or both one
+  t.throws(function () { packet.name.decode(Buffer.from([0x80])) }, /Cannot decode name \(bad label\)$/)
+  t.throws(function () { packet.name.decode(Buffer.from([0xb0])) }, /Cannot decode name \(bad label\)$/)
+
+  // Ensure there's enough buffer to read
+  t.throws(function () { packet.name.decode(Buffer.from([])) }, /Cannot decode name \(buffer overflow\)$/)
+  t.throws(function () { packet.name.decode(Buffer.from([0x01, 0x00])) }, /Cannot decode name \(buffer overflow\)$/)
+  t.throws(function () { packet.name.decode(Buffer.from([0x01])) }, /Cannot decode name \(buffer overflow\)$/)
+  t.throws(function () { packet.name.decode(Buffer.from([0xc0])) }, /Cannot decode name \(buffer overflow\)$/)
+
+  // Allow only pointers backwards
+  t.throws(function () { packet.name.decode(Buffer.from([0xc0, 0x00])) }, /Cannot decode name \(bad pointer\)$/)
+  t.throws(function () { packet.name.decode(Buffer.from([0xc0, 0x01])) }, /Cannot decode name \(bad pointer\)$/)
+
+  // A name can be only 253 characters (when connected with dots)
+  const maxLength = Buffer.alloc(255)
+  maxLength.fill(Buffer.from([0x01, 0x61]), 0, 254)
+  t.ok(packet.name.decode(maxLength) === new Array(127).fill('a').join('.'))
+
+  const tooLong = Buffer.alloc(256)
+  tooLong.fill(Buffer.from([0x01, 0x61]))
+  t.throws(function () { packet.name.decode(tooLong) }, /Cannot decode name \(name too long\)$/)
+
+  // Ensure jumps don't reset the total length counter
+  const tooLongWithJump = Buffer.alloc(403)
+  tooLongWithJump.fill(Buffer.from([0x01, 0x61]), 0, 200)
+  tooLongWithJump.fill(Buffer.from([0x01, 0x61]), 201, 401)
+  tooLongWithJump.set([0xc0, 0x00], 401)
+  t.throws(function () { packet.name.decode(tooLongWithJump, 201) }, /Cannot decode name \(name too long\)$/)
+
+  // Ensure a jump to a null byte doesn't add extra dots
+  t.ok(packet.name.decode(Buffer.from([0x00, 0x01, 0x61, 0xc0, 0x00]), 1) === 'a')
+
+  // Ensure deeply nested pointers don't cause "Maximum call stack size exceeded" errors
+  const buf = Buffer.alloc(16386)
+  for (let i = 0; i < 16384; i += 2) {
+    buf.writeUInt16BE(0xc000 | i, i + 2)
+  }
+  t.ok(packet.name.decode(buf, 16384) === '.')
+
+  t.end()
+})
+
 tape('stream', function (t) {
   const val = {
     type: 'query',