fixed a bug with array slice index wrapping (#1594)

Author: TomerStarkware

src/libfuncs/array.rs

@@ -1185,24 +1185,26 @@ pub fn build_slice<'ctx, 'this>(
 
     let slice_start = entry.argument(2)?.into();
     let slice_len = entry.argument(3)?.into();
-    let slice_end = entry.append_op_result(arith::addi(slice_start, slice_len, location))?;
 
-    let slice_lhs_bound = entry.append_op_result(arith::cmpi(
+    // Compute `slice_end = slice_start + slice_len` in 64 bits. In 32-bit
+    // arithmetic the sum wraps and lets an attacker-chosen `slice_len`
+    // sneak past the bound below. Since `slice_len` is unsigned (Sierra
+    // u32) it cannot be negative, so once we've checked
+    // `slice_end <= array_len` we also know `slice_start <= array_len`.
+    let wide_ty = IntegerType::new(context, 64).into();
+    let slice_start_wide = entry.extui(slice_start, wide_ty, location)?;
+    let slice_len_wide = entry.extui(slice_len, wide_ty, location)?;
+    let array_len_wide = entry.extui(array_len, wide_ty, location)?;
+    let slice_end_wide =
+        entry.append_op_result(arith::addi(slice_start_wide, slice_len_wide, location))?;
+
+    let slice_bounds = entry.append_op_result(arith::cmpi(
         context,
         CmpiPredicate::Ule,
-        slice_start,
-        array_len,
-        location,
-    ))?;
-    let slice_rhs_bound = entry.append_op_result(arith::cmpi(
-        context,
-        CmpiPredicate::Ule,
-        slice_end,
-        array_len,
+        slice_end_wide,
+        array_len_wide,
         location,
     ))?;
-    let slice_bounds =
-        entry.append_op_result(arith::andi(slice_lhs_bound, slice_rhs_bound, location))?;
 
     let valid_block = helper.append_block(Block::new(&[]));
     let error_block = helper.append_block(Block::new(&[]));

test_data/programs/array_slice_overflow.cairo

@@ -0,0 +1,4 @@
+fn run_test(user_len: u32) {
+    let arr: Array<u64> = array![10_u64, 20_u64];
+    let slice = arr.span().slice(1, user_len);
+}

tests/tests/arrays.rs

@@ -59,3 +59,35 @@ proptest! {
         .unwrap();
     }
 }
+
+#[test]
+fn array_slice_u32_overflow() {
+    let program = &load_program_and_runner("test_data_artifacts/programs/array_slice_overflow");
+    let user_len = u32::MAX;
+
+    let result_vm = run_vm_program(
+        program,
+        "run_test",
+        vec![Arg::Value(Felt::from(user_len))],
+        Some(DEFAULT_GAS as usize),
+    )
+    .unwrap();
+    let result_native = run_native_program(
+        program,
+        "run_test",
+        &[Value::Uint32(user_len)],
+        Some(DEFAULT_GAS),
+        Option::<DummySyscallHandler>::None,
+    );
+
+    compare_outputs(
+        &program.1,
+        &program.2.find_function("run_test").unwrap().id,
+        &result_vm,
+        &result_native,
+    )
+    .expect(
+        "array_slice diverges between VM and native — native let a u32-overflowing \
+         slice through; see build_slice in src/libfuncs/array.rs",
+    );
+}