Security audit issues fixed (#1279)

* Restrict network key file permissions

* Check for overflow in str to bigint conversion

* RLP decoder overflow check

parity/signer.rs

@@ -18,7 +18,7 @@ use std::io;
 use std::path::PathBuf;
 use std::sync::Arc;
 use util::panics::{PanicHandler, ForwardPanic};
-use util::keys::directory::restrict_permissions_owner;
+use util::path::restrict_permissions_owner;
 use die::*;
 use rpc_apis;
 

util/bigint/src/uint.rs

@@ -50,6 +50,12 @@ use std::cmp::*;
 use serde;
 use rustc_serialize::hex::{FromHex, FromHexError, ToHex};
 
+/// Conversion from decimal string error
+#[derive(Debug, PartialEq)]
+pub enum FromDecStrErr {
+	/// Value does not fit into type
+	InvalidLength
+}
 
 macro_rules! impl_map_from {
 	($thing:ident, $from:ty, $to:ty) => {
@@ -493,10 +499,8 @@ pub trait Uint: Sized + Default + FromStr + From<u64> + fmt::Debug + fmt::Displa
 	/// Returns the largest value that can be represented by this integer type.
 	fn max_value() -> Self;
 
-	/// Error type for converting from a decimal string.
-	type FromDecStrErr;
 	/// Convert from a decimal string.
-	fn from_dec_str(value: &str) -> Result<Self, Self::FromDecStrErr>;
+	fn from_dec_str(value: &str) -> Result<Self, FromDecStrErr>;
 
 	/// Conversion to u32
 	fn low_u32(&self) -> u32;
@@ -553,17 +557,22 @@ macro_rules! construct_uint {
 		pub struct $name(pub [u64; $n_words]);
 
 		impl Uint for $name {
-			type FromDecStrErr = FromHexError;
 
 			/// TODO: optimize, throw appropriate err
-			fn from_dec_str(value: &str) -> Result<Self, Self::FromDecStrErr> {
+			fn from_dec_str(value: &str) -> Result<Self, FromDecStrErr> {
-				Ok(value.bytes()
+				let mut res = Self::default();
-				   .map(|b| b - 48)
+				for b in value.bytes().map(|b| b - 48) {
-				   .fold($name::from(0u64), | acc, c |
+					let (r, overflow) = res.overflowing_mul_u32(10);
-						 // fast multiplication by 10
+					if overflow {
-						 // (acc << 3) + (acc << 1) => acc * 10
+						return Err(FromDecStrErr::InvalidLength);
-						 (acc << 3) + (acc << 1) + $name::from(c)
+					}
-					))
+					let (r, overflow) = r.overflowing_add(b.into());
+					if overflow {
+						return Err(FromDecStrErr::InvalidLength);
+					}
+					res = r;
+				}
+				Ok(res)
 			}
 
 			#[inline]
@@ -1433,6 +1442,7 @@ known_heap_size!(0, U128, U256);
 mod tests {
 	use uint::{Uint, U128, U256, U512};
 	use std::str::FromStr;
+	use super::FromDecStrErr;
 
 	#[test]
 	pub fn uint256_from() {
@@ -1802,6 +1812,7 @@ mod tests {
 	fn uint256_from_dec_str() {
 		assert_eq!(U256::from_dec_str("10").unwrap(), U256::from(10u64));
 		assert_eq!(U256::from_dec_str("1024").unwrap(), U256::from(1024u64));
+		assert_eq!(U256::from_dec_str("115792089237316195423570985008687907853269984665640564039457584007913129639936"), Err(FromDecStrErr::InvalidLength));
 	}
 
 	#[test]

util/src/keys/directory.rs

@@ -18,6 +18,7 @@
 
 use common::*;
 use std::path::{PathBuf};
+use path::restrict_permissions_owner;
 
 const CURRENT_DECLARED_VERSION: u64 = 3;
 const MAX_KEY_FILE_LEN: u64 = 1024 * 80;
@@ -465,23 +466,6 @@ pub struct KeyDirectory {
 	cache_usage: RwLock<VecDeque<Uuid>>,
 }
 
-/// Restricts the permissions of given path only to the owner.
-#[cfg(not(windows))]
-pub fn restrict_permissions_owner(file_path: &Path) -> Result<(), i32>  {
-	let cstr = ::std::ffi::CString::new(file_path.to_str().unwrap()).unwrap();
-	match unsafe { ::libc::chmod(cstr.as_ptr(), ::libc::S_IWUSR | ::libc::S_IRUSR) } {
-		0 => Ok(()),
-		x => Err(x),
-	}
-}
-
-/// Restricts the permissions of given path only to the owner.
-#[cfg(windows)]
-pub fn restrict_permissions_owner(_file_path: &Path) -> Result<(), i32>  {
-	//TODO: implement me
-	Ok(())
-}
-
 impl KeyDirectory {
 	/// Initializes new cache directory context with a given `path`
 	pub fn new(path: &Path) -> KeyDirectory {

util/src/network/host.rs

@@ -41,6 +41,7 @@ use network::stats::NetworkStats;
 use network::error::{NetworkError, DisconnectReason};
 use network::discovery::{Discovery, TableUpdates, NodeEntry};
 use network::ip_utils::{map_external_address, select_public_address};
+use path::restrict_permissions_owner;
 
 type Slab<T> = ::slab::Slab<T, usize>;
 
@@ -946,13 +947,17 @@ fn save_key(path: &Path, key: &Secret) {
 		return;
 	};
 	path_buf.push("key");
-	let mut file = match fs::File::create(path_buf.as_path()) {
+	let path = path_buf.as_path();
+	let mut file = match fs::File::create(&path) {
 		Ok(file) => file,
 		Err(e) => {
 			warn!("Error creating key file: {:?}", e);
 			return;
 		}
 	};
+	if let Err(e) = restrict_permissions_owner(&path) {
+		warn!(target: "network", "Failed to modify permissions of the file (chmod: {})", e);
+	}
 	if let Err(e) = file.write(&key.hex().into_bytes()) {
 		warn!("Error writing key file: {:?}", e);
 	}

util/src/path.rs

@@ -15,6 +15,7 @@
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
 //! Path utilities
+use std::path::Path;
 
 /// Default ethereum paths
 pub mod ethereum {
@@ -62,3 +63,21 @@ pub mod ethereum {
 		pth
 	}
 }
+
+/// Restricts the permissions of given path only to the owner.
+#[cfg(not(windows))]
+pub fn restrict_permissions_owner(file_path: &Path) -> Result<(), i32>  {
+	let cstr = ::std::ffi::CString::new(file_path.to_str().unwrap()).unwrap();
+	match unsafe { ::libc::chmod(cstr.as_ptr(), ::libc::S_IWUSR | ::libc::S_IRUSR) } {
+		0 => Ok(()),
+		x => Err(x),
+	}
+}
+
+/// Restricts the permissions of given path only to the owner.
+#[cfg(windows)]
+pub fn restrict_permissions_owner(_file_path: &Path) -> Result<(), i32>  {
+	//TODO: implement me
+	Ok(())
+}
+

util/src/rlp/tests.rs

@@ -429,3 +429,10 @@ fn test_rlp_nested_empty_list_encode() {
 	assert_eq!(stream.drain()[..], [0xc2u8, 0xc0u8, 40u8][..]);
 }
 
+#[test]
+fn test_rlp_list_length_overflow() {
+	let data: Vec<u8> = vec![0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00];
+	let rlp = UntrustedRlp::new(&data);
+	let as_val: Result<String, DecoderError> = rlp.val_at(0);
+	assert_eq!(Err(DecoderError::RlpIsTooShort), as_val);
+}

util/src/rlp/untrusted_rlp.rs

@@ -334,9 +334,9 @@ impl<'a> BasicDecoder<'a> {
 	/// Return first item info.
 	fn payload_info(bytes: &[u8]) -> Result<PayloadInfo, DecoderError> {
 		let item = try!(PayloadInfo::from(bytes));
-		match item.header_len + item.value_len <= bytes.len() {
+		match item.header_len.checked_add(item.value_len) { 
-			true => Ok(item),
+			Some(x) if x <= bytes.len() => Ok(item), 
-			false => Err(DecoderError::RlpIsTooShort),
+			_ => Err(DecoderError::RlpIsTooShort), 
 		}
 	}
 }