fix: insert critical overflow checks preventing heap corruption (#3077)

crusso · web-flow · commit 0657017d685d · 2022-01-28T20:24:43.000Z
Fixes two classes of `u32`-undetected overflow bugs that could corrupt heap, stable variables, or both: * ic.rs: `alloc_words` wasn't checking validity of addresses, leading to possible (and observed) heap corruption. * compile.ml: `buffer_size` wasn't checking overflow of 32-bit size pre-computation. The first is fixed using an overflow detecting add function of Rust. The second is checked by using a local `i64` in each type-directed size function. These functions are MV and return two `i32`s (for the data buffer and (vestigial) reference buffer sizes. I did not modify the functions to return 2 `i64`s because our MultiValue emulation only support `i32` at the moment. Fixes #3068.
diff --git a/rts/motoko-rts/src/memory/ic.rs b/rts/motoko-rts/src/memory/ic.rs
@@ -76,7 +76,11 @@ impl Memory for IcMemory {
 
         // Update heap pointer
         let old_hp = HP;
-        let new_hp = old_hp + bytes.as_u32();
+        let (new_hp, overflow) = old_hp.overflowing_add(bytes.as_u32());
+        // Check for overflow
+        if overflow {
+            rts_trap_with("Out of memory");
+        }
         HP = new_hp;
 
         // Grow memory if needed
diff --git a/src/codegen/compile.ml b/src/codegen/compile.ml
@@ -4386,14 +4386,15 @@ module Serialization = struct
     (fun env get_x ->
 
       (* Some combinators for writing values *)
-      let (set_data_size, get_data_size) = new_local env "data_size" in
+      let (set_data_size, get_data_size) = new_local64 env "data_size" in
       let (set_ref_size, get_ref_size) = new_local env "ref_size" in
-      compile_unboxed_const 0l ^^ set_data_size ^^
+      compile_const_64 0L ^^ set_data_size ^^
       compile_unboxed_const 0l ^^ set_ref_size ^^
 
       let inc_data_size code =
-        get_data_size ^^ code ^^
-        G.i (Binary (Wasm.Values.I32 I32Op.Add)) ^^
+        get_data_size ^^
+        code ^^ G.i (Convert (Wasm.Values.I64 I64Op.ExtendUI32)) ^^
+        G.i (Binary (Wasm.Values.I64 I64Op.Add)) ^^
         set_data_size
       in
 
@@ -4404,9 +4405,10 @@ module Serialization = struct
       in
 
       let size env t =
+        let (set_inc, get_inc) = new_local env "inc" in
         buffer_size env t ^^
         get_ref_size ^^ G.i (Binary (Wasm.Values.I32 I32Op.Add)) ^^ set_ref_size ^^
-        get_data_size ^^ G.i (Binary (Wasm.Values.I32 I32Op.Add)) ^^ set_data_size
+        set_inc ^^ inc_data_size get_inc
       in
 
       let size_alias size_thing =
@@ -4508,7 +4510,14 @@ module Serialization = struct
         size_alias (fun () -> get_x ^^ Heap.load_field MutBox.field ^^ size env t)
       | _ -> todo "buffer_size" (Arrange_ir.typ t) G.nop
       end ^^
+      (* Check 32-bit overflow of buffer_size *)
+      get_data_size ^^
+      compile_shrU64_const 32L ^^
+      G.i (Test (Wasm.Values.I64 I64Op.Eqz)) ^^
+      E.else_trap_with env "buffer_size overflow" ^^
+      (* Convert to 32-bit *)
       get_data_size ^^
+      G.i (Convert (Wasm.Values.I32 I32Op.WrapI64)) ^^
       get_ref_size
     )
 
@@ -5343,9 +5352,14 @@ module Serialization = struct
       get_x ^^
       buffer_size env (Type.seq ts) ^^
       set_refs_size ^^
-
+      (* add tydesc_len *)
       compile_add_const tydesc_len  ^^
       set_data_size ^^
+      (* check for overflow *)
+      get_data_size ^^
+      compile_unboxed_const tydesc_len ^^
+      G.i (Compare (Wasm.Values.I32 I32Op.LtU)) ^^
+      E.then_trap_with env "serialization overflow" ^^
 
       let (set_data_start, get_data_start) = new_local env "data_start" in
       let (set_refs_start, get_refs_start) = new_local env "refs_start" in
diff --git a/test/run-drun/inc-oom.mo b/test/run-drun/inc-oom.mo
@@ -0,0 +1,22 @@
+// test incremental oom by allocating 5 GB, one GB at a time!
+import P "mo:⛔";
+actor {
+
+  var c = 5;
+
+  while(c > 0) {
+    let a : [var Nat8] = P.Array_init<Nat8>(1024*1024*1024/4, 0xFF);
+    c -= 1;
+  };
+
+}
+
+//SKIP run
+//SKIP run-low
+//SKIP run-ir
+// too slow on ic-ref-run:
+//SKIP comp-ref
+// too resource heavy on GH:
+//SKIP comp
+
+
diff --git a/test/run-drun/ok/oom-query.drun-run.ok b/test/run-drun/ok/oom-query.drun-run.ok
@@ -0,0 +1,3 @@
+ingress Completed: Reply: 0x4449444c016c01b3c4b1f204680100010a00000000000000000101
+ingress Completed: Reply: 0x4449444c0000
+Err: IC0503: Canister rwlgt-iiaaa-aaaaa-aaaaa-cai trapped explicitly: RTS error: Out of memory
diff --git a/test/run-drun/ok/oom-update.drun-run.ok b/test/run-drun/ok/oom-update.drun-run.ok
@@ -0,0 +1,3 @@
+ingress Completed: Reply: 0x4449444c016c01b3c4b1f204680100010a00000000000000000101
+ingress Completed: Reply: 0x4449444c0000
+Err: IC0503: Canister rwlgt-iiaaa-aaaaa-aaaaa-cai trapped explicitly: RTS error: Out of memory
diff --git a/test/run-drun/ok/query-size-overflow.drun-run.ok b/test/run-drun/ok/query-size-overflow.drun-run.ok
@@ -0,0 +1,4 @@
+ingress Completed: Reply: 0x4449444c016c01b3c4b1f204680100010a00000000000000000101
+ingress Completed: Reply: 0x4449444c0000
+Err: IC0503: Canister rwlgt-iiaaa-aaaaa-aaaaa-cai trapped explicitly: buffer_size overflow
+Err: IC0503: Canister rwlgt-iiaaa-aaaaa-aaaaa-cai trapped explicitly: buffer_size overflow
diff --git a/test/run-drun/ok/query-size-overflow.ic-ref-run.ok b/test/run-drun/ok/query-size-overflow.ic-ref-run.ok
@@ -0,0 +1,8 @@
+→ update create_canister(record {settings = null})
+← replied: (record {hymijyo = principal "cvccv-qqaaq-aaaaa-aaaaa-c"})
+→ update install_code(record {arg = blob ""; kca_xin = blob "\00asm\01\00\00\00\0…
+← replied: ()
+→ query overflow()
+← rejected (RC_CANISTER_ERROR): canister trapped: EvalTrapError region:0xXXX-0xXXX "canister trapped explicitly: buffer_size overflow"
+→ query overflowOpt()
+← rejected (RC_CANISTER_ERROR): canister trapped: EvalTrapError region:0xXXX-0xXXX "canister trapped explicitly: buffer_size overflow"
diff --git a/test/run-drun/ok/stable-size-overflow.drun-run.ok b/test/run-drun/ok/stable-size-overflow.drun-run.ok
@@ -0,0 +1,4 @@
+ingress Completed: Reply: 0x4449444c016c01b3c4b1f204680100010a00000000000000000101
+ingress Completed: Reply: 0x4449444c0000
+debug.print: upgrading...
+ingress Err: IC0503: Canister rwlgt-iiaaa-aaaaa-aaaaa-cai trapped explicitly: buffer_size overflow
diff --git a/test/run-drun/ok/stable-size-overflow.ic-ref-run.ok b/test/run-drun/ok/stable-size-overflow.ic-ref-run.ok
@@ -0,0 +1,7 @@
+→ update create_canister(record {settings = null})
+← replied: (record {hymijyo = principal "cvccv-qqaaq-aaaaa-aaaaa-c"})
+→ update install_code(record {arg = blob ""; kca_xin = blob "\00asm\01\00\00\00\0…
+← replied: ()
+→ update install_code(record {arg = blob ""; kca_xin = blob "\00asm\01\00\00\00\0…
+debug.print: upgrading...
+← rejected (RC_CANISTER_ERROR): Pre-upgrade trapped: EvalTrapError region:0xXXX-0xXXX "canister trapped explicitly: buffer_size overflow"
diff --git a/test/run-drun/oom-query.mo b/test/run-drun/oom-query.mo
@@ -0,0 +1,28 @@
+// test incremental oom
+import P "mo:⛔";
+actor {
+
+  public query func go() : async () {
+    // allocate 3GB
+    var c = 3;
+    while(c > 0) {
+      let a : [var Nat8] = P.Array_init<Nat8>(1024*1024*1024/4, 0xFF);
+      c -= 1;
+    };
+
+    // allocate 1GB in 1k chunks and trigger Oom
+    var d = 1024*1024;
+    while(d > 0) {
+      let a : [var Nat8] = P.Array_init<Nat8>(1024/4, 0xFF);
+      d -= 1;
+    };
+  }
+}
+
+//SKIP run
+//SKIP run-low
+//SKIP run-ir
+// too slow on ic-ref-run:
+//SKIP comp-ref
+
+//CALL query go "DIDL\x00\x00"
diff --git a/test/run-drun/oom-update.mo b/test/run-drun/oom-update.mo
@@ -0,0 +1,29 @@
+// test incremental oom
+import P "mo:⛔";
+actor {
+
+  public query func go() : async () {
+    // allocate 3GB
+    var c = 3;
+    while(c > 0) {
+      let a : [var Nat8] = P.Array_init<Nat8>(1024*1024*1024/4, 0xFF);
+      c -= 1;
+    };
+
+    // allocate 1GB in 1k chunks and trigger Oom
+    var d = 1024*1024;
+    while(d > 0) {
+      let a : [var Nat8] = P.Array_init<Nat8>(1024/4, 0xFF);
+      d -= 1;
+    };
+
+  }
+}
+
+//SKIP run
+//SKIP run-low
+//SKIP run-ir
+// too slow on ic-ref-run:
+//SKIP comp-ref
+//CALL query go "DIDL\x00\x00"
+
diff --git a/test/run-drun/query-size-overflow.mo b/test/run-drun/query-size-overflow.mo
@@ -0,0 +1,28 @@
+import P "mo:⛔";
+import SM "stable-mem/StableMemory";
+
+// Our users may not thank us that we only preserve sharing for mutable data, but nothing else.
+actor {
+
+  ignore SM.grow(1);
+
+  let page : Blob = SM.loadBlob(0,65536);
+  assert (page.size() == 65536);
+
+  public query func overflow() : async [Blob] {
+    P.Array_tabulate<Blob>(65536,func _ { page });
+  };
+
+  public query func overflowOpt() : async (?[Blob]){
+    ? P.Array_tabulate<Blob>(65536,func _ { page });
+  };
+
+}
+
+//SKIP run
+//SKIP run-low
+//SKIP run-ir
+
+//OR-CALL query overflow "DIDL\x00\x00"
+//OR-CALL query overflowOpt "DIDL\x00\x00"
+
diff --git a/test/run-drun/stable-size-overflow.mo b/test/run-drun/stable-size-overflow.mo
@@ -0,0 +1,24 @@
+import P "mo:⛔";
+import SM "stable-mem/StableMemory";
+
+actor {
+
+  ignore SM.grow(1);
+
+  let page : Blob = SM.loadBlob(0,65536);
+  assert (page.size() == 65536);
+
+  stable
+  let a : [Blob] = P.Array_tabulate<Blob>(65536,func _ { page });
+
+  system func preupgrade() {
+   P.debugPrint("upgrading...");
+  };
+}
+
+//SKIP run
+//SKIP run-low
+//SKIP run-ir
+
+//CALL upgrade ""
+
diff --git a/test/run-drun/upgrade-bug-immut.mo b/test/run-drun/upgrade-bug-immut.mo
@@ -0,0 +1,19 @@
+import P "mo:⛔";
+
+actor {
+  stable var a : [Nat] = P.Array_tabulate<Nat>(268435456 / 4, func _ { 0x0F } ); // 0.25 GB array (I think)
+
+  system func preupgrade() { P.debugPrint("pre"); };
+
+  system func postupgrade() { P.debugPrint("post"); }
+}
+
+//SKIP run
+//SKIP run-low
+//SKIP run-ir
+// too slow on ic-ref-run:
+//SKIP comp-ref
+// too resource heavy on GH:
+//SKIP comp
+
+//CALL upgrade ""
diff --git a/test/run-drun/upgrade-bug.mo b/test/run-drun/upgrade-bug.mo
@@ -0,0 +1,19 @@
+import P "mo:⛔";
+
+actor {
+  stable var a : [var Nat] = P.Array_init(268435456 / 4, 0x0F); // 0.25 GB array (I think)
+
+  system func preupgrade() { P.debugPrint("pre"); };
+
+  system func postupgrade() { P.debugPrint("post"); }
+}
+
+//SKIP run
+//SKIP run-low
+//SKIP run-ir
+// too slow on ic-ref-run:
+//SKIP comp-ref
+// too resource heavy on GH:
+//SKIP comp
+
+//CALL upgrade ""
diff --git a/test/run.sh b/test/run.sh
@@ -86,6 +86,8 @@ function normalize () {
     sed 's/\[Canister [0-9a-z\-]*\]/debug.print:/g' |
     # Normalize instruction locations on traps, added by ic-ref ad6ea9e
     sed 's/region:0x[0-9a-fA-F]\+-0x[0-9a-fA-F]\+/region:0xXXX-0xXXX/g' |
+    # Delete everything after Oom
+    sed '/RTS error: Out of memory/q' |
     cat > $1.norm
     mv $1.norm $1
   fi
diff --git a/test/run/inc-oom.mo b/test/run/inc-oom.mo
@@ -0,0 +1,18 @@
+// test incremental oom by allocating 5 GB, one GB at a time
+import P "mo:⛔";
+do {
+
+  var c = 5;
+
+  while(c > 0) {
+    let a : [var Nat8] = P.Array_init<Nat8>(1024*1024*1024/4, 0xFF);
+    c -= 1;
+  };
+
+}
+
+//SKIP run
+//SKIP run-low
+//SKIP run-ir
+
+
diff --git a/test/run/ok/inc-oom.wasm-run.ok b/test/run/ok/inc-oom.wasm-run.ok
@@ -0,0 +1 @@
+RTS error: Out of memory
diff --git a/test/run/ok/inc-oom.wasm-run.ret.ok b/test/run/ok/inc-oom.wasm-run.ret.ok
@@ -0,0 +1 @@
+Return code 134

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+ingress Completed: Reply: 0x4449444c016c01b3c4b1f204680100010a00000000000000000101`
	`2`	`+ingress Completed: Reply: 0x4449444c0000`
	`3`	`+Err: IC0503: Canister rwlgt-iiaaa-aaaaa-aaaaa-cai trapped explicitly: RTS error: Out of memory`