From 20d0f2ee2607a1eecf095223250ca07307b9dbb0 Mon Sep 17 00:00:00 2001 From: Smit Soni Date: Tue, 20 Jul 2021 22:27:33 -0700 Subject: [PATCH] Move shim argument checks before isolation check This allows catching extremely incorrect arguments before rejecting due to isolation. --- src/shims/posix/fs.rs | 171 +++++++++++++++++++++--------------------- 1 file changed, 85 insertions(+), 86 deletions(-) diff --git a/src/shims/posix/fs.rs b/src/shims/posix/fs.rs index 2693dc09628..ac6e878da96 100644 --- a/src/shims/posix/fs.rs +++ b/src/shims/posix/fs.rs @@ -304,28 +304,6 @@ fn insert_fd_with_min_fd(&mut self, file_handle: Box, min_fd impl<'mir, 'tcx: 'mir> EvalContextExtPrivate<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} trait EvalContextExtPrivate<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { - /// Emulate `stat` or `lstat` on `macos`. This function is not intended to be - /// called directly from `emulate_foreign_item_by_name`, so it does not check if isolation is - /// disabled or if the target OS is the correct one. Please use `macos_stat` or - /// `macos_lstat` instead. - fn macos_stat_or_lstat( - &mut self, - follow_symlink: bool, - path_op: &OpTy<'tcx, Tag>, - buf_op: &OpTy<'tcx, Tag>, - ) -> InterpResult<'tcx, i32> { - let this = self.eval_context_mut(); - - let path_scalar = this.read_pointer(path_op)?; - let path = this.read_path_from_c_str(path_scalar)?.into_owned(); - - let metadata = match FileMetadata::from_path(this, &path, follow_symlink)? { - Some(metadata) => metadata, - None => return Ok(-1), - }; - this.macos_stat_write_buf(metadata, buf_op) - } - fn macos_stat_write_buf( &mut self, metadata: FileMetadata, @@ -504,13 +482,6 @@ fn open( ) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); - // Reject if isolation is enabled. - if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { - this.reject_in_isolation("`open`", reject_with)?; - this.set_last_error_from_io_error(ErrorKind::PermissionDenied)?; - return Ok(-1); - } - let flag = this.read_scalar(flag_op)?.to_i32()?; // Get the mode. On macOS, the argument type `mode_t` is actually `u16`, but @@ -588,6 +559,13 @@ fn open( let path = this.read_path_from_c_str(this.read_pointer(path_op)?)?; + // Reject if isolation is enabled. + if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { + this.reject_in_isolation("`open`", reject_with)?; + this.set_last_error_from_io_error(ErrorKind::PermissionDenied)?; + return Ok(-1); + } + let fd = options.open(&path).map(|file| { let fh = &mut this.machine.file_handler; fh.insert_fd(Box::new(FileHandle { file, writable })) @@ -599,13 +577,6 @@ fn open( fn fcntl(&mut self, args: &[OpTy<'tcx, Tag>]) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); - // Reject if isolation is enabled. - if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { - this.reject_in_isolation("`fcntl`", reject_with)?; - this.set_last_error_from_io_error(ErrorKind::PermissionDenied)?; - return Ok(-1); - } - if args.len() < 2 { throw_ub_format!( "incorrect number of arguments for fcntl: got {}, expected at least 2", @@ -614,6 +585,14 @@ fn fcntl(&mut self, args: &[OpTy<'tcx, Tag>]) -> InterpResult<'tcx, i32> { } let fd = this.read_scalar(&args[0])?.to_i32()?; let cmd = this.read_scalar(&args[1])?.to_i32()?; + + // Reject if isolation is enabled. + if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { + this.reject_in_isolation("`fcntl`", reject_with)?; + this.set_last_error_from_io_error(ErrorKind::PermissionDenied)?; + return Ok(-1); + } + // We only support getting the flags for a descriptor. if cmd == this.eval_libc_i32("F_GETFD")? { // Currently this is the only flag that `F_GETFD` returns. It is OK to just return the @@ -795,6 +774,8 @@ fn lseek64( fn unlink(&mut self, path_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); + let path = this.read_path_from_c_str(this.read_pointer(path_op)?)?; + // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`unlink`", reject_with)?; @@ -802,8 +783,6 @@ fn unlink(&mut self, path_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { return Ok(-1); } - let path = this.read_path_from_c_str(this.read_pointer(path_op)?)?; - let result = remove_file(path).map(|_| 0); this.try_unwrap_io_result(result) } @@ -825,6 +804,8 @@ fn create_link(src: &Path, dst: &Path) -> std::io::Result<()> { } let this = self.eval_context_mut(); + let target = this.read_path_from_c_str(this.read_pointer(target_op)?)?; + let linkpath = this.read_path_from_c_str(this.read_pointer(linkpath_op)?)?; // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { @@ -833,9 +814,6 @@ fn create_link(src: &Path, dst: &Path) -> std::io::Result<()> { return Ok(-1); } - let target = this.read_path_from_c_str(this.read_pointer(target_op)?)?; - let linkpath = this.read_path_from_c_str(this.read_pointer(linkpath_op)?)?; - let result = create_link(&target, &linkpath).map(|_| 0); this.try_unwrap_io_result(result) } @@ -848,6 +826,9 @@ fn macos_stat( let this = self.eval_context_mut(); this.assert_target_os("macos", "stat"); + let path_scalar = this.read_pointer(path_op)?; + let path = this.read_path_from_c_str(path_scalar)?.into_owned(); + // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`stat`", reject_with)?; @@ -857,7 +838,12 @@ fn macos_stat( } // `stat` always follows symlinks. - this.macos_stat_or_lstat(true, path_op, buf_op) + let metadata = match FileMetadata::from_path(this, &path, true)? { + Some(metadata) => metadata, + None => return Ok(-1), + }; + + this.macos_stat_write_buf(metadata, buf_op) } // `lstat` is used to get symlink metadata. @@ -869,6 +855,9 @@ fn macos_lstat( let this = self.eval_context_mut(); this.assert_target_os("macos", "lstat"); + let path_scalar = this.read_pointer(path_op)?; + let path = this.read_path_from_c_str(path_scalar)?.into_owned(); + // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`lstat`", reject_with)?; @@ -877,7 +866,12 @@ fn macos_lstat( return Ok(-1); } - this.macos_stat_or_lstat(false, path_op, buf_op) + let metadata = match FileMetadata::from_path(this, &path, false)? { + Some(metadata) => metadata, + None => return Ok(-1), + }; + + this.macos_stat_write_buf(metadata, buf_op) } fn macos_fstat( @@ -889,6 +883,8 @@ fn macos_fstat( this.assert_target_os("macos", "fstat"); + let fd = this.read_scalar(fd_op)?.to_i32()?; + // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`fstat`", reject_with)?; @@ -896,8 +892,6 @@ fn macos_fstat( return this.handle_not_found(); } - let fd = this.read_scalar(fd_op)?.to_i32()?; - let metadata = match FileMetadata::from_fd(this, fd)? { Some(metadata) => metadata, None => return Ok(-1), @@ -973,8 +967,10 @@ fn linux_statx( // relative to CWD, `EACCES` is the most relevant. this.eval_libc("EACCES")? } else { - // `dirfd` is set to target file, and `path` is - // empty. `EACCES` would violate the spec. + // `dirfd` is set to target file, and `path` is empty + // (or we would have hit the `throw_unsup_format` + // above). `EACCES` would violate the spec. + assert!(empty_path_flag); this.eval_libc("EBADF")? }; this.set_last_error(ecode)?; @@ -1089,13 +1085,6 @@ fn rename( ) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); - // Reject if isolation is enabled. - if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { - this.reject_in_isolation("`rename`", reject_with)?; - this.set_last_error_from_io_error(ErrorKind::PermissionDenied)?; - return Ok(-1); - } - let oldpath_ptr = this.read_pointer(oldpath_op)?; let newpath_ptr = this.read_pointer(newpath_op)?; @@ -1108,6 +1097,13 @@ fn rename( let oldpath = this.read_path_from_c_str(oldpath_ptr)?; let newpath = this.read_path_from_c_str(newpath_ptr)?; + // Reject if isolation is enabled. + if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { + this.reject_in_isolation("`rename`", reject_with)?; + this.set_last_error_from_io_error(ErrorKind::PermissionDenied)?; + return Ok(-1); + } + let result = rename(oldpath, newpath).map(|_| 0); this.try_unwrap_io_result(result) @@ -1120,13 +1116,6 @@ fn mkdir( ) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); - // Reject if isolation is enabled. - if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { - this.reject_in_isolation("`mkdir`", reject_with)?; - this.set_last_error_from_io_error(ErrorKind::PermissionDenied)?; - return Ok(-1); - } - #[cfg_attr(not(unix), allow(unused_variables))] let mode = if this.tcx.sess.target.os == "macos" { u32::from(this.read_scalar(mode_op)?.to_u16()?) @@ -1136,6 +1125,13 @@ fn mkdir( let path = this.read_path_from_c_str(this.read_pointer(path_op)?)?; + // Reject if isolation is enabled. + if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { + this.reject_in_isolation("`mkdir`", reject_with)?; + this.set_last_error_from_io_error(ErrorKind::PermissionDenied)?; + return Ok(-1); + } + #[cfg_attr(not(unix), allow(unused_mut))] let mut builder = DirBuilder::new(); @@ -1155,6 +1151,8 @@ fn mkdir( fn rmdir(&mut self, path_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); + let path = this.read_path_from_c_str(this.read_pointer(path_op)?)?; + // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`rmdir`", reject_with)?; @@ -1162,8 +1160,6 @@ fn rmdir(&mut self, path_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { return Ok(-1); } - let path = this.read_path_from_c_str(this.read_pointer(path_op)?)?; - let result = remove_dir(path).map(|_| 0i32); this.try_unwrap_io_result(result) @@ -1172,6 +1168,8 @@ fn rmdir(&mut self, path_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { fn opendir(&mut self, name_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); + let name = this.read_path_from_c_str(this.read_pointer(name_op)?)?; + // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`opendir`", reject_with)?; @@ -1180,8 +1178,6 @@ fn opendir(&mut self, name_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, Scalar) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); + let dirp = this.read_scalar(dirp_op)?.to_machine_usize(this)?; + // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`closedir`", reject_with)?; @@ -1410,8 +1408,6 @@ fn closedir(&mut self, dirp_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { return this.handle_not_found(); } - let dirp = this.read_scalar(dirp_op)?.to_machine_usize(this)?; - if let Some(dir_iter) = this.machine.dir_handler.streams.remove(&dirp) { drop(dir_iter); Ok(0) @@ -1427,6 +1423,9 @@ fn ftruncate64( ) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); + let fd = this.read_scalar(fd_op)?.to_i32()?; + let length = this.read_scalar(length_op)?.to_i64()?; + // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`ftruncate64`", reject_with)?; @@ -1434,8 +1433,6 @@ fn ftruncate64( return this.handle_not_found(); } - let fd = this.read_scalar(fd_op)?.to_i32()?; - let length = this.read_scalar(length_op)?.to_i64()?; if let Some(file_descriptor) = this.machine.file_handler.handles.get_mut(&fd) { // FIXME: Support ftruncate64 for all FDs let FileHandle { file, writable } = file_descriptor.as_file_handle()?; @@ -1467,6 +1464,8 @@ fn fsync(&mut self, fd_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); + let fd = this.read_scalar(fd_op)?.to_i32()?; + // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`fsync`", reject_with)?; @@ -1474,7 +1473,6 @@ fn fsync(&mut self, fd_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { return this.handle_not_found(); } - let fd = this.read_scalar(fd_op)?.to_i32()?; if let Some(file_descriptor) = this.machine.file_handler.handles.get(&fd) { // FIXME: Support fsync for all FDs let FileHandle { file, writable } = file_descriptor.as_file_handle()?; @@ -1488,6 +1486,8 @@ fn fsync(&mut self, fd_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { fn fdatasync(&mut self, fd_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); + let fd = this.read_scalar(fd_op)?.to_i32()?; + // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`fdatasync`", reject_with)?; @@ -1495,7 +1495,6 @@ fn fdatasync(&mut self, fd_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { return this.handle_not_found(); } - let fd = this.read_scalar(fd_op)?.to_i32()?; if let Some(file_descriptor) = this.machine.file_handler.handles.get(&fd) { // FIXME: Support fdatasync for all FDs let FileHandle { file, writable } = file_descriptor.as_file_handle()?; @@ -1515,13 +1514,6 @@ fn sync_file_range( ) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); - // Reject if isolation is enabled. - if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { - this.reject_in_isolation("`sync_file_range`", reject_with)?; - // Set error code as "EBADF" (bad fd) - return this.handle_not_found(); - } - let fd = this.read_scalar(fd_op)?.to_i32()?; let offset = this.read_scalar(offset_op)?.to_i64()?; let nbytes = this.read_scalar(nbytes_op)?.to_i64()?; @@ -1541,6 +1533,13 @@ fn sync_file_range( return Ok(-1); } + // Reject if isolation is enabled. + if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { + this.reject_in_isolation("`sync_file_range`", reject_with)?; + // Set error code as "EBADF" (bad fd) + return this.handle_not_found(); + } + if let Some(file_descriptor) = this.machine.file_handler.handles.get(&fd) { // FIXME: Support sync_data_range for all FDs let FileHandle { file, writable } = file_descriptor.as_file_handle()?; @@ -1559,6 +1558,10 @@ fn readlink( ) -> InterpResult<'tcx, i64> { let this = self.eval_context_mut(); + let pathname = this.read_path_from_c_str(this.read_pointer(pathname_op)?)?; + let buf = this.read_pointer(buf_op)?; + let bufsize = this.read_scalar(bufsize_op)?.to_machine_usize(this)?; + // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`readlink`", reject_with)?; @@ -1567,10 +1570,6 @@ fn readlink( return Ok(-1); } - let pathname = this.read_path_from_c_str(this.read_pointer(pathname_op)?)?; - let buf = this.read_pointer(buf_op)?; - let bufsize = this.read_scalar(bufsize_op)?.to_machine_usize(this)?; - let result = std::fs::read_link(pathname); match result { Ok(resolved) => { -- 2.44.0