ssz: fix buffer overflows, explicit error reporting (#7)

* ssz: fix buffer overflows, explicit error reporting

* ssz: harden against unaligned data

beacon_chain/ssz.nim

@@ -9,7 +9,7 @@
 # See https://github.com/ethereum/beacon_chain/issues/100
 # and https://github.com/ethereum/beacon_chain/tree/master/ssz
 
-import ./datatypes, eth_common, endians, typetraits
+import ./datatypes, eth_common, endians, typetraits, options
 
 # ################### Helper functions ###################################
 func `+`[T](p: ptr T, offset: int): ptr T {.inline.}=
@@ -17,81 +17,91 @@ func `+`[T](p: ptr T, offset: int): ptr T {.inline.}=
   const size = sizeof T
   cast[ptr T](cast[ByteAddress](p) +% offset * size)
 
-func checkSize[T: not seq](x: T, pos, len: int) {.inline.}=
+func eat(x: var auto, data: ptr byte, pos: var int, len: int): bool =
-  # This assumes that T is packed
+  if pos + x.sizeof > len: return
-  doAssert pos + T.sizeof < len, "Deserialization overflow"
+  copyMem(x.addr, data + pos, x.sizeof)
+  inc pos, x.sizeof
+  return true
 
-func checkSize[T](x: seq[T], pos, len: int) {.inline.}=
+func eatInt[T: SomeInteger or byte](x: var T, data: ptr byte, pos: var int, len: int):
-  # seq length is stored in an uint32 (4 bytes) for SSZ
+    bool =
-  doAssert pos + 4 + x.len * T.sizeof < len, "Deserialization overflow"
+  if pos + x.sizeof > len: return
+
+  # XXX: any better way to get a suitably aligned buffer in nim???
+  # see also: https://github.com/nim-lang/Nim/issues/9206
+  var tmp: uint64
+  var alignedBuf = cast[ptr byte](tmp.addr)
+  copyMem(alignedBuf, data + pos, x.sizeof)
 
-template deserInt(x: var SomeInteger or byte, data: ptr byte, pos: var int) =
   when x.sizeof == 8:
-    bigEndian64(x.addr, data + pos)
+    bigEndian64(x.addr, alignedBuf)
-    inc pos, 8
   elif x.sizeof == 4:
-    bigEndian32(x.addr, data + pos)
+    bigEndian32(x.addr, alignedBuf)
-    inc pos, 4
   elif x.sizeof == 2:
-    bigEndian16(x.addr, data + pos)
+    bigEndian16(x.addr, alignedBuf)
-    inc pos, 2
+  elif x.sizeof == 1:
+    x = cast[ptr type x](alignedBuf)[]
   else:
-    x = cast[ptr type x](data + pos)[]
+    {.fatal: "Unsupported type deserialization: " & $(type(x)).name.}
-    inc pos
 
-func deserSeq[T](dest: var seq[T], len: int, src: ptr byte, pos: var int) =
+  inc pos, x.sizeof
-  dest = newSeqUninitialized[T](len)
+  return true
-  for val in dest.mitems:
-    val.deserInt(src, pos)
 
-func serInt[T: SomeInteger or byte](dest: var seq[byte], src: T, buffer: var array[sizeof(T), byte]) {.inline.}=
+func eatSeq[T: SomeInteger or byte](x: var seq[T], data: ptr byte, pos: var int,
-  when T.sizeof == 8:
+    len: int): bool =
-    bigEndian64(buffer.addr, src.unsafeAddr)
+  var items: int32
-  elif T.sizeof == 4:
+  if not eatInt(items, data, pos, len): return
-    bigEndian32(buffer.addr, src.unsafeAddr)
+  if pos + T.sizeof * items > len: return
-  elif T.sizeof == 2:
+
-    bigEndian16(buffer.addr, src.unsafeAddr)
+  x = newSeqUninitialized[T](items)
-  else:
+  for val in x.mitems:
-    dest.add byte(src)
+    discard eatInt(val, data, pos, len) # Bounds-checked above
-    return
+  return true
-  dest.add buffer
 
 func serInt[T: SomeInteger or byte](dest: var seq[byte], src: T) {.inline.}=
-  var buffer: array[T.sizeof, byte]
+  # XXX: any better way to get a suitably aligned buffer in nim???
-  dest.serInt(src, buffer)
+  var tmp: T
+  var alignedBuf = cast[ptr array[src.sizeof, byte]](tmp.addr)
+  when src.sizeof == 8:
+    bigEndian64(alignedBuf, src.unsafeAddr)
+  elif src.sizeof == 4:
+    bigEndian32(alignedBuf, src.unsafeAddr)
+  elif src.sizeof == 2:
+    bigEndian16(alignedBuf, src.unsafeAddr)
+  elif src.sizeof == 1:
+    copyMem(alignedBuf, src.unsafeAddr, src.sizeof) # careful, aliasing..
+  else:
+    {.fatal: "Unsupported type deserialization: " & $(type(x)).name.}
+
+  dest.add alignedBuf[]
 
 func serSeq[T: SomeInteger or byte](dest: var seq[byte], src: seq[T]) =
   dest.serInt src.len.uint32
-  var buffer: array[T.sizeof, byte]
   for val in src:
-    dest.serInt(val, buffer)
+    dest.serInt(val)
 
 # ################### Core functions ###################################
-func deserialize(data: ptr byte, pos: var int, len: int, typ: typedesc[object]): typ =
+func deserialize(data: ptr byte, pos: var int, len: int, typ: typedesc[object]):
-  for field in result.fields:
+    auto =
-    checkSize field, pos, len
+  var t: typ
-    when field is EthAddress:
+
-      copyMem(field.addr, data + pos, 20)
+  for field in t.fields:
-      inc pos, 20
+    when field is EthAddress | MDigest:
-    elif field is MDigest:
+      if not eat(field, data, pos, len): return
-      const size = field.bits div 8
-      copyMem(field.addr, data + pos, size)
-      inc pos, size
     elif field is (SomeInteger or byte):
-      field.deserInt(data, pos)
+      if not eatInt(field, data, pos, len): return
     elif field is seq[SomeInteger or byte]:
-      var length: int32
+      if not eatSeq(field, data, pos, len): return
-      bigEndian32(length.addr, data + pos)
-      inc pos, 4
-      deserSeq(field, length, data, pos)
     else: # TODO: deserializing subtypes (?, depends on final spec)
       {.fatal: "Unsupported type deserialization: " & $typ.name.}
+  return some(t)
 
 func deserialize*(
       data: seq[byte or uint8] or openarray[byte or uint8] or string,
-      typ: typedesc[object]): typ {.inline.}=
+      typ: typedesc[object]): auto {.inline.} =
+  # XXX: returns Option[typ]: https://github.com/nim-lang/Nim/issues/9195
   var pos = 0
-  deserialize((ptr byte)(data[0].unsafeAddr), pos, data.len, typ)
+  return deserialize((ptr byte)(data[0].unsafeAddr), pos, data.len, typ)
 
 func serialize*[T](value: T): seq[byte] =
   for field in value.fields:

tests/test_ssz.nim

@@ -6,7 +6,7 @@
 # at your option. This file may not be copied, modified, or distributed except according to those terms.
 
 import
-  unittest, nimcrypto, eth_common, sequtils,
+  unittest, nimcrypto, eth_common, sequtils, options,
   ../beacon_chain/ssz
 
 func filled[N: static[int], T](typ: type array[N, T], value: T): array[N, T] =
@@ -47,9 +47,14 @@ suite "Simple serialization":
   expected_ser &= [byte 0, 0, 0, 3, 'c'.ord, 'o'.ord, 'w'.ord]
 
   test "Deserialization":
-    let deser = expected_ser.deserialize(Foo)
+    let deser = expected_ser.deserialize(Foo).get()
     check: expected_deser == deser
 
   test "Serialization":
     let ser = expected_deser.serialize()
     check: expected_ser == ser
+
+  test "Overflow":
+    check:
+      expected_ser[0..^2].deserialize(Foo).isNone()
+      expected_ser[1..^1].deserialize(Foo).isNone()