Clam 2269 & 2271: CDIFF: unlink, close issues. Freshclam: download whole CVD if failure applying CDIFF

libclamav_rust/build.rs

@@ -55,7 +55,13 @@ const BINDGEN_FUNCTIONS: &[&str] = &[
 ];
 
 // Generate bindings for these types (structs, enums):
-const BINDGEN_TYPES: &[&str] = &["cli_matcher", "cli_ac_data", "cli_ac_result", "css_image_extractor_t", "css_image_handle_t"];
+const BINDGEN_TYPES: &[&str] = &[
+    "cli_matcher",
+    "cli_ac_data",
+    "cli_ac_result",
+    "css_image_extractor_t",
+    "css_image_handle_t",
+];
 
 // Find the required functions and types in these headers:
 const BINDGEN_HEADERS: &[&str] = &[
@@ -186,7 +192,7 @@ fn detect_clamav_build() -> Result<(), &'static str> {
 
         // LLVM is optional, and don't have a path to each library like we do with the other libs.
         let llvm_libs = env::var("LLVM_LIBS").unwrap_or("".into());
-        if llvm_libs != "" {
+        if !llvm_libs.is_empty() {
             match env::var("LLVM_DIRS") {
                 Err(env::VarError::NotPresent) => eprintln!("LLVM_DIRS not set"),
                 Err(env::VarError::NotUnicode(_)) => return Err("environment value not unicode"),
@@ -203,7 +209,7 @@ fn detect_clamav_build() -> Result<(), &'static str> {
 
             llvm_libs
                 .split(',')
-                .for_each(|filepath_str| match parse_lib_path(&filepath_str) {
+                .for_each(|filepath_str| match parse_lib_path(filepath_str) {
                     Ok(parsed_path) => {
                         println!("cargo:rustc-link-search={}", parsed_path.dir);
                         eprintln!("  - requesting that rustc link {:?}", &parsed_path.libname);
@@ -282,7 +288,7 @@ struct ParsedLibraryPath {
 
 // Parse a library path, returning the portion expected after the `-l`, and the
 // directory containing the library
-fn parse_lib_path<'a>(path: &'a str) -> Result<ParsedLibraryPath, &'static str> {
+fn parse_lib_path(path: &str) -> Result<ParsedLibraryPath, &'static str> {
     let path = PathBuf::from(path);
     let file_name = path
         .file_name()

libclamav_rust/src/cdiff.rs

@@ -25,8 +25,8 @@ use std::{
     io::{prelude::*, BufReader, BufWriter, Read, Seek, SeekFrom, Write},
     iter::*,
     os::raw::c_char,
-    path::PathBuf,
-    str,
+    path::{Path, PathBuf},
+    str::{self, FromStr},
 };
 
 use crate::sys;
@@ -97,8 +97,12 @@ pub enum CdiffError {
     ///
     /// This error *may* wrap a processing error if the command has side effects
     /// (e.g., MOVE or CLOSE)
-    #[error("{1} on line {0}")]
-    Input(usize, InputError),
+    #[error("{err} on line {line}: {operation}")]
+    Input {
+        line: usize,
+        err: InputError,
+        operation: String,
+    },
 
     /// An error encountered while handling a particular CDiff command
     #[error("processing {1} command on line {2}: {0}")]
@@ -155,14 +159,20 @@ pub enum InputError {
     #[error("No DB open for action {0}")]
     NoDBForAction(&'static str),
 
+    #[error("Invalid DB \"{0}\" open for action {1}")]
+    InvalidDBForAction(String, &'static str),
+
     #[error("File {0} not closed before opening {1}")]
     NotClosedBeforeOpening(String, String),
 
     #[error("{0} not Unicode")]
     NotUnicode(&'static str),
 
-    #[error("Forbidden characters found in database name {0}")]
-    ForbiddenCharactersInDB(String),
+    #[error("Invalid database name {0}. Characters must be alphanumeric or '.'")]
+    InvalidDBNameForbiddenCharacters(String),
+
+    #[error("Invalid database name {0}. Must not specify parent directory.")]
+    InvalidDBNameNoParentDirectory(String),
 
     #[error("{0} missing for {1}")]
     MissingParameter(&'static str, &'static str),
@@ -183,6 +193,9 @@ pub enum InputError {
 
     #[error("no final newline")]
     MissingNL,
+
+    #[error("Database file is still open: {0}")]
+    DBStillOpen(String),
 }
 
 /// Errors encountered while processing
@@ -286,6 +299,43 @@ impl<'a> DelOp<'a> {
     }
 }
 
+#[derive(Debug)]
+pub struct UnlinkOp<'a> {
+    db_name: &'a str,
+}
+
+/// Method to parse the cdiff line describing an unlink operation
+impl<'a> UnlinkOp<'a> {
+    pub fn new(data: &'a [u8]) -> Result<Self, InputError> {
+        let mut iter = data.split(|b| *b == b' ');
+        let db_name = str::from_utf8(
+            iter.next()
+                .ok_or(InputError::MissingParameter("UNLINK", "db_name"))?,
+        )
+        .map_err(|_| InputError::NotUnicode("database name"))?;
+
+        if !db_name
+            .chars()
+            .all(|x: char| x.is_alphanumeric() || x == '.')
+        {
+            // DB Name contains invalid characters.
+            return Err(InputError::InvalidDBNameForbiddenCharacters(
+                db_name.to_owned(),
+            ));
+        }
+
+        let db_path = PathBuf::from_str(db_name).unwrap();
+        if db_path.parent() != Some(Path::new("")) {
+            // DB Name must be not include a parent directory.
+            return Err(InputError::InvalidDBNameNoParentDirectory(
+                db_name.to_owned(),
+            ));
+        }
+
+        Ok(UnlinkOp { db_name })
+    }
+}
+
 #[derive(Debug)]
 pub struct MoveOp<'a> {
     src: PathBuf,
@@ -448,7 +498,7 @@ pub fn script2cdiff(script_file_name: &str, builder: &str, server: &str) -> Resu
         .map_err(|e| CdiffError::FileCreate(cdiff_file_name.to_owned(), e))?;
 
     // Open the original script file for reading
-    let script_file: File = File::open(&script_file_name)
+    let script_file: File = File::open(script_file_name)
         .map_err(|e| CdiffError::FileOpen(script_file_name.to_owned(), e))?;
 
     // Get file length
@@ -570,7 +620,7 @@ pub fn cdiff_apply(file: &mut File, mode: ApplyMode) -> Result<(), CdiffError> {
             let dsig = read_dsig(file)?;
             debug!("cdiff_apply() - final dsig length is {}", dsig.len());
             if is_debug_enabled() {
-                print_file_data(dsig.clone(), dsig.len() as usize);
+                print_file_data(dsig.clone(), dsig.len());
             }
 
             // Get file length
@@ -644,10 +694,22 @@ fn cmd_open(ctx: &mut Context, db_name: Option<&[u8]>) -> Result<(), InputError>
 
     if !db_name
         .chars()
-        .all(|x: char| x.is_alphanumeric() || x == '\\' || x == '/' || x == '.')
+        .all(|x: char| x.is_alphanumeric() || x == '.')
     {
-        return Err(InputError::ForbiddenCharactersInDB(db_name.to_owned()));
+        // DB Name contains invalid characters.
+        return Err(InputError::InvalidDBNameForbiddenCharacters(
+            db_name.to_owned(),
+        ));
     }
+
+    let db_path = PathBuf::from_str(db_name).unwrap();
+    if db_path.parent() != Some(Path::new("")) {
+        // DB Name must be not include a parent directory.
+        return Err(InputError::InvalidDBNameNoParentDirectory(
+            db_name.to_owned(),
+        ));
+    }
+
     ctx.open_db = Some(db_name.to_owned());
 
     Ok(())
@@ -912,6 +974,7 @@ fn cmd_close(ctx: &mut Context) -> Result<(), InputError> {
     // Test for lines to add
     if !ctx.additions.is_empty() {
         let mut db_file = OpenOptions::new()
+            .create(true)
             .append(true)
             .open(&open_db)
             .map_err(ProcessingError::from)?;
@@ -927,12 +990,16 @@ fn cmd_close(ctx: &mut Context) -> Result<(), InputError> {
 }
 
 /// Set up Context structure with data parsed from command unlink
-fn cmd_unlink(ctx: &mut Context) -> Result<(), InputError> {
+fn cmd_unlink(ctx: &mut Context, unlink_op: UnlinkOp) -> Result<(), InputError> {
     if let Some(open_db) = &ctx.open_db {
-        fs::remove_file(open_db).map_err(ProcessingError::from)?;
-    } else {
-        return Err(InputError::NoDBForAction("UNLINK"));
+        return Err(InputError::DBStillOpen(open_db.clone()));
     }
+
+    // We checked that the db_name doesn't have any '/' or '\\' in it before
+    // adding to the UnlinkOp struct, so it's safe to say the path is just a local file and
+    // won't accidentally delete something in a different directory.
+    fs::remove_file(unlink_op.db_name).map_err(ProcessingError::from)?;
+
     Ok(())
 }
 
@@ -960,9 +1027,12 @@ fn process_line(ctx: &mut Context, line: &[u8]) -> Result<(), InputError> {
             cmd_move(ctx, move_op)
         }
         b"CLOSE" => cmd_close(ctx),
-        b"UNLINK" => cmd_unlink(ctx),
+        b"UNLINK" => {
+            let unlink_op = UnlinkOp::new(remainder.unwrap())?;
+            cmd_unlink(ctx, unlink_op)
+        }
         _ => Err(InputError::UnknownCommand(
-            String::from_utf8(cmd.to_owned()).unwrap(),
+            String::from_utf8_lossy(&cmd).to_string(),
         )),
     }
 }
@@ -986,10 +1056,14 @@ where
             0 => break,
             n_read => {
                 decompressed_bytes = decompressed_bytes + n_read + 1;
-                match linebuf.get(0) {
+                match linebuf.first() {
                     // Skip comment lines
                     Some(b'#') => continue,
-                    _ => process_line(ctx, &linebuf).map_err(|e| CdiffError::Input(line_no, e))?,
+                    _ => process_line(ctx, &linebuf).map_err(|e| CdiffError::Input {
+                        line: line_no,
+                        err: e,
+                        operation: String::from_utf8_lossy(&linebuf).to_string(),
+                    })?,
                 }
             }
         }
@@ -1035,7 +1109,7 @@ fn read_dsig(file: &mut File) -> Result<Vec<u8>, SignatureError> {
 // as the offset in the file that the header ends.
 fn read_size(file: &mut File) -> Result<(u32, usize), HeaderError> {
     // Seek to beginning of file.
-    file.seek(SeekFrom::Start(0))?;
+    file.rewind()?;
 
     // File should always start with "ClamAV-Diff".
     let prefix = b"ClamAV-Diff";
@@ -1080,7 +1154,7 @@ fn get_hash(file: &mut File, len: usize) -> Result<[u8; 32], CdiffError> {
     let mut hasher = Sha256::new();
 
     // Seek to beginning of file
-    file.seek(SeekFrom::Start(0))?;
+    file.rewind()?;
 
     let mut sum: usize = 0;
 

libclamav_rust/src/css_image_extract.rs

@@ -22,7 +22,7 @@
 
 use std::{ffi::CStr, mem::ManuallyDrop, os::raw::c_char};
 
-use base64::{Engine as _, engine::general_purpose as base64_engine_standard};
+use base64::{engine::general_purpose as base64_engine_standard, Engine as _};
 use log::{debug, error, warn};
 use thiserror::Error;
 
@@ -194,7 +194,7 @@ impl<'a> CssImageExtractor<'a> {
             // So we'll just skip until after the next ';'.
 
             // Find contents after ";"
-            if let Some(pos) = url_parameter.find(";") {
+            if let Some(pos) = url_parameter.find(';') {
                 (_, url_parameter) = url_parameter.split_at(pos + ";".len());
                 // Found ";"
             } else {
@@ -267,7 +267,7 @@ impl<'a> Iterator for CssImageExtractor<'a> {
             // Decode the base64 encoded image
             base64_engine_standard::STANDARD.decode(base64_image).ok()
         } else {
-            return None;
+            None
         }
     }
 }
@@ -297,9 +297,9 @@ pub unsafe extern "C" fn new_css_image_extractor(
     };
 
     if let Ok(extractor) = CssImageExtractor::new(css_input) {
-        return Box::into_raw(Box::new(extractor)) as sys::css_image_extractor_t;
+        Box::into_raw(Box::new(extractor)) as sys::css_image_extractor_t
     } else {
-        return 0 as sys::css_image_extractor_t;
+        0 as sys::css_image_extractor_t
     }
 }
 
@@ -334,11 +334,9 @@ pub unsafe extern "C" fn css_image_extract_next(
             *image_out = image.as_ptr();
             *image_out_len = image.len();
             *image_out_handle = Box::into_raw(Box::new(image)) as sys::css_image_handle_t;
-            return true;
-        }
-        None => {
-            return false;
+            true
         }
+        None => false,
     }
 }
 

libclamav_rust/src/evidence.rs

@@ -70,7 +70,7 @@ pub struct IndicatorMeta {
 /// Initialize a match vector
 #[no_mangle]
 pub extern "C" fn evidence_new() -> sys::evidence_t {
-    Box::into_raw(Box::new(Evidence::default())) as sys::evidence_t
+    Box::into_raw(Box::<Evidence>::default()) as sys::evidence_t
 }
 
 /// Free the evidence
@@ -147,15 +147,15 @@ pub unsafe extern "C" fn _evidence_get_indicator(
                 return meta.last().unwrap().static_virname as *const c_char;
             } else {
                 // no alert at that index. return NULL
-                return std::ptr::null();
+                std::ptr::null()
             }
         }
         IndicatorType::PotentiallyUnwanted => {
             if let Some(meta) = evidence.pua.values().nth(index) {
                 return meta.last().unwrap().static_virname as *const c_char;
             } else {
                 // no alert at that index. return NULL
-                return std::ptr::null();
+                std::ptr::null()
             }
         }
     }

libclamav_rust/src/fuzzy_hash.rs

@@ -123,7 +123,7 @@ pub struct FuzzyHashMeta {
 /// Initialize the hashmap
 #[no_mangle]
 pub extern "C" fn fuzzy_hashmap_new() -> sys::fuzzyhashmap_t {
-    Box::into_raw(Box::new(FuzzyHashMap::default())) as sys::fuzzyhashmap_t
+    Box::into_raw(Box::<FuzzyHashMap>::default()) as sys::fuzzyhashmap_t
 }
 
 /// Free the hashmap

libclamav_rust/src/logging.rs

@@ -88,8 +88,12 @@ mod tests {
 /// API exported for C code to log to standard error using Rust.
 /// This would be be an alternative to fputs, and reliably prints
 /// non-ASCII UTF8 characters on Windows, where fputs does not.
+///
+/// # Safety
+///
+/// This function dereferences the c_buff raw pointer. Pointer must be valid.
 #[no_mangle]
-pub extern "C" fn clrs_eprint(c_buf: *const c_char) -> () {
+pub unsafe extern "C" fn clrs_eprint(c_buf: *const c_char) {
     if c_buf.is_null() {
         return;
     }

libfreshclam/dns.c

@@ -71,12 +71,12 @@ dnsquery(const char *domain, int qtype, unsigned int *ttl)
         if (qtype == T_TXT)
             qtype = T_ANY;
         if ((len = res_query(domain, C_IN, qtype, answer, PACKETSZ)) < 0) {
-            logg(LOGG_INFO, "%cCan't query %s\n",
-                 (qtype == T_TXT || qtype == T_ANY) ? '^' : '*', domain);
+            logg((qtype == T_TXT || qtype == T_ANY) ? LOGG_WARNING : LOGG_DEBUG, "Can't query %s\n",
+                 domain);
             return NULL;
         }
 #else
-        logg(LOGG_INFO, "%cCan't query %s\n", (qtype == T_TXT) ? '^' : '*', domain);
+        logg((qtype == T_TXT) ? LOGG_WARNING : LOGG_DEBUG, "Can't query %s\n", domain);
         return NULL;
 #endif
     }