Additional precompile fixes (#261)

* Parse and bounds-check entire Modexp length inputs

* Test large modexp computation and fix related typo

* Bounds checks and unit tests for Ed25519Verify precompile

* s/<spaces>/<tabs>/

Author: notlesh

frame/evm/precompile/ed25519/src/lib.rs

@@ -35,10 +35,12 @@ impl LinearCostPrecompile for Ed25519Verify {
 		input: &[u8],
 		_: usize,
 	) -> core::result::Result<(ExitSucceed, Vec<u8>), ExitError> {
-		let len = min(input.len(), 128);
+		if input.len() < 128 {
+			return Err(ExitError::Other("input must contain 128 bytes".into()));
+		};
 
 		let mut i = [0u8; 128];
-		i[..len].copy_from_slice(&input[..len]);
+		i[..128].copy_from_slice(&input[..128]);
 
 		let mut buf = [0u8; 4];
 
@@ -58,3 +60,97 @@ impl LinearCostPrecompile for Ed25519Verify {
 		Ok((ExitSucceed::Returned, buf.to_vec()))
 	}
 }
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+	use ed25519_dalek::{Keypair, SecretKey, Signer};
+
+	#[test]
+	fn test_empty_input() -> std::result::Result<(), ExitError> {
+
+		let input: [u8; 0] = [];
+		let cost: usize = 1;
+
+		match Ed25519Verify::execute(&input, cost) {
+			Ok((_, _)) => {
+				panic!("Test not expected to pass");
+			},
+			Err(e) => {
+				assert_eq!(e, ExitError::Other("input must contain 128 bytes".into()));
+				Ok(())
+			}
+		}
+	}
+
+	#[test]
+	fn test_verify() -> std::result::Result<(), ExitError> {
+
+		let secret_key_bytes: [u8; ed25519_dalek::SECRET_KEY_LENGTH] = [
+			157, 097, 177, 157, 239, 253, 090, 096,
+			186, 132, 074, 244, 146, 236, 044, 196,
+			068, 073, 197, 105, 123, 050, 105, 025,
+			112, 059, 172, 003, 028, 174, 127, 096, ];
+
+		let secret_key = SecretKey::from_bytes(&secret_key_bytes)
+								.expect("Failed to generate secretkey");
+		let public_key = (&secret_key).into();
+
+		let keypair = Keypair { secret: secret_key, public: public_key };
+
+		let msg: &[u8] = b"abcdefghijklmnopqrstuvwxyz123456";
+		assert_eq!(msg.len(), 32);
+		let signature = keypair.sign(msg);
+
+		// input is:
+		// 1) message (32 bytes)
+		// 2) pubkey (32 bytes)
+		// 3) signature (64 bytes)
+		let mut input: Vec<u8> = Vec::with_capacity(128);
+		input.extend_from_slice(msg);
+		input.extend_from_slice(&public_key.to_bytes());
+		input.extend_from_slice(&signature.to_bytes());
+		assert_eq!(input.len(), 128);
+
+		let cost: usize = 1;
+
+		match Ed25519Verify::execute(&input, cost) {
+			Ok((_, output)) => {
+				assert_eq!(output.len(), 4);
+				assert_eq!(output[0], 0u8);
+				assert_eq!(output[1], 0u8);
+				assert_eq!(output[2], 0u8);
+				assert_eq!(output[3], 0u8);
+			},
+			Err(e) => {
+				return Err(e);
+			}
+		};
+
+		// try again with a different message
+		let msg: &[u8] = b"BAD_MESSAGE_mnopqrstuvwxyz123456";
+
+		let mut input: Vec<u8> = Vec::with_capacity(128);
+		input.extend_from_slice(msg);
+		input.extend_from_slice(&public_key.to_bytes());
+		input.extend_from_slice(&signature.to_bytes());
+		assert_eq!(input.len(), 128);
+
+		match Ed25519Verify::execute(&input, cost) {
+			Ok((_, output)) => {
+				assert_eq!(output.len(), 4);
+				assert_eq!(output[0], 0u8);
+				assert_eq!(output[1], 0u8);
+				assert_eq!(output[2], 0u8);
+				assert_eq!(output[3], 1u8); // non-zero indicates error (in our case, 1)
+			},
+			Err(e) => {
+				return Err(e);
+			}
+		};
+
+		Ok(())
+	}
+
+}
+

frame/evm/precompile/modexp/src/lib.rs

@@ -23,10 +23,26 @@ use alloc::vec::Vec;
 use core::cmp::max;
 use fp_evm::LinearCostPrecompile;
 use evm::{ExitSucceed, ExitError};
-use num::{BigUint, Zero, One};
+use num::{BigUint, Zero, One, ToPrimitive, FromPrimitive};
 
 pub struct Modexp;
 
+// ModExp expects the following as inputs:
+// 1) 32 bytes expressing the length of base
+// 2) 32 bytes expressing the length of exponent
+// 3) 32 bytes expressing the length of modulus
+// 4) base, size as described above
+// 5) exponent, size as described above
+// 6) modulus, size as described above
+//
+//
+// NOTE: input sizes are arbitrarily large (up to 256 bits), with the expectation
+//       that gas limits would be applied before actual computation.
+//
+//       maximum stack size will also prevent abuse.
+//
+//       see: https://eips.ethereum.org/EIPS/eip-198
+
 impl LinearCostPrecompile for Modexp {
 	const BASE: usize = 15;
 	const WORD: usize = 3;
@@ -39,23 +55,39 @@ impl LinearCostPrecompile for Modexp {
 			return Err(ExitError::Other("input must contain at least 96 bytes".into()));
 		};
 
+		// reasonable assumption: this must fit within the Ethereum EVM's max stack size
+		let max_size_big = BigUint::from_u32(1024).expect("can't create BigUint");
+
 		let mut buf = [0; 32];
 		buf.copy_from_slice(&input[0..32]);
-		let mut len_bytes = [0u8; 8];
-		len_bytes.copy_from_slice(&buf[24..]);
-		let base_len = u64::from_be_bytes(len_bytes) as usize;
+		let base_len_big = BigUint::from_bytes_be(&buf);
+		if base_len_big > max_size_big {
+			return Err(ExitError::Other("unreasonably large base length".into()));
+		}
 
-		buf = [0; 32];
 		buf.copy_from_slice(&input[32..64]);
-		len_bytes = [0u8; 8];
-		len_bytes.copy_from_slice(&buf[24..]);
-		let exp_len = u64::from_be_bytes(len_bytes) as usize;
+		let exp_len_big = BigUint::from_bytes_be(&buf);
+		if exp_len_big > max_size_big {
+			return Err(ExitError::Other("unreasonably large exponent length".into()));
+		}
 
-		buf = [0; 32];
 		buf.copy_from_slice(&input[64..96]);
-		len_bytes = [0u8; 8];
-		len_bytes.copy_from_slice(&buf[24..]);
-		let mod_len = u64::from_be_bytes(len_bytes) as usize;
+		let mod_len_big = BigUint::from_bytes_be(&buf);
+		if mod_len_big > max_size_big {
+			return Err(ExitError::Other("unreasonably large exponent length".into()));
+		}
+
+		// bounds check handled above
+		let base_len = base_len_big.to_usize().expect("base_len out of bounds");
+		let exp_len = exp_len_big.to_usize().expect("exp_len out of bounds");
+		let mod_len = mod_len_big.to_usize().expect("mod_len out of bounds");
+
+		// input length should be at least 96 + user-specified length of base + exp + mod
+		let total_len = base_len + exp_len + mod_len + 96;
+		if input.len() < total_len {
+			let err_msg = format!("expected at least {} bytes but received {}", total_len, input.len());
+			return Err(ExitError::Other(err_msg.into()));
+		}
 
 		// Gas formula allows arbitrary large exp_len when base and modulus are empty, so we need to handle empty base first.
 		let r = if base_len == 0 && mod_len == 0 {
@@ -72,6 +104,8 @@ impl LinearCostPrecompile for Modexp {
 			let mod_start = exp_start + exp_len;
 			let modulus = BigUint::from_bytes_be(&input[mod_start..mod_start + mod_len]);
 
+			// TODO: computation should only proceed if sufficient gas has been provided
+
 			if modulus.is_zero() || modulus.is_one() {
 				BigUint::zero()
 			} else {
@@ -120,15 +154,51 @@ mod tests {
 	}
 
 	#[test]
-	fn test_simple_inputs() {
+	fn test_insufficient_input() -> std::result::Result<(), ExitError> {
+
+		let input = hex::decode(
+			"0000000000000000000000000000000000000000000000000000000000000001\
+			0000000000000000000000000000000000000000000000000000000000000001\
+			0000000000000000000000000000000000000000000000000000000000000001")
+			.expect("Decode failed");
+
+		let cost: usize = 1;
+
+		match Modexp::execute(&input, cost) {
+			Ok((_, _)) => {
+				panic!("Test not expected to pass");
+			},
+			Err(e) => {
+				assert_eq!(e, ExitError::Other("expected at least 99 bytes but received 96".into()));
+				Ok(())
+			}
+		}
+	}
+
+	#[test]
+	fn test_excessive_input() -> std::result::Result<(), ExitError> {
+
+		let input = hex::decode(
+			"1000000000000000000000000000000000000000000000000000000000000001\
+			0000000000000000000000000000000000000000000000000000000000000001\
+			0000000000000000000000000000000000000000000000000000000000000001")
+			.expect("Decode failed");
 
-		// ModExp expects the following as inputs:
-		// 1) 32 bytes expressing the length of base
-		// 2) 32 bytes expressing the length of exponent
-		// 3) 32 bytes expressing the length of modulus
-		// 4) base, size as described above
-		// 5) exponent, size as described above
-		// 6) modulus, size as described above
+		let cost: usize = 1;
+
+		match Modexp::execute(&input, cost) {
+			Ok((_, _)) => {
+				panic!("Test not expected to pass");
+			},
+			Err(e) => {
+				assert_eq!(e, ExitError::Other("unreasonably large base length".into()));
+				Ok(())
+			}
+		}
+	}
+
+	#[test]
+	fn test_simple_inputs() {
 
 		let input = hex::decode(
 			"0000000000000000000000000000000000000000000000000000000000000001\
@@ -183,4 +253,31 @@ mod tests {
 			}
 		}
 	}
+
+	#[test]
+	fn test_large_computation() {
+
+		let input = hex::decode(
+			"0000000000000000000000000000000000000000000000000000000000000001\
+			0000000000000000000000000000000000000000000000000000000000000020\
+			0000000000000000000000000000000000000000000000000000000000000020\
+			03\
+			fffffffffffffffffffffffffffffffffffffffffffffffffffffffefffffc2e\
+			fffffffffffffffffffffffffffffffffffffffffffffffffffffffefffffc2f")
+			.expect("Decode failed");
+
+		let cost: usize = 1;
+
+		match Modexp::execute(&input, cost) {
+			Ok((_, output)) => {
+				assert_eq!(output.len(), 32); // should be same length as mod
+				let result = BigUint::from_bytes_be(&output[..]);
+				let expected = BigUint::parse_bytes(b"1", 10).unwrap();
+				assert_eq!(result, expected);
+			},
+			Err(_) => {
+				panic!("Modexp::execute() returned error"); // TODO: how to pass error on?
+			}
+		}
+	}
 }