Check for attempted path traversal on import

This commit is contained in:
Damien Elmes 2022-03-17 16:01:23 +10:00
parent c2e8d89fc6
commit 57a4495d92

View File

@ -5,7 +5,7 @@ use std::{
collections::HashMap, collections::HashMap,
fs::{self, File}, fs::{self, File},
io::{self, Read, Write}, io::{self, Read, Write},
path::{Path, PathBuf}, path::{Component, Path, PathBuf},
}; };
use prost::Message; use prost::Message;
@ -74,7 +74,8 @@ pub fn import_colpkg(
check_collection(tempfile.path())?; check_collection(tempfile.path())?;
progress_fn(ImportProgress::Collection)?; progress_fn(ImportProgress::Collection)?;
let media_import_result = restore_media(&meta, progress_fn, &mut archive, target_media_folder); let media_folder = Path::new(target_media_folder);
let media_import_result = restore_media(&meta, progress_fn, &mut archive, media_folder);
// Proceed with replacing collection, regardless of media import result // Proceed with replacing collection, regardless of media import result
tempfile.as_file().sync_all()?; tempfile.as_file().sync_all()?;
@ -104,7 +105,7 @@ fn restore_media(
meta: &Meta, meta: &Meta,
mut progress_fn: impl FnMut(ImportProgress) -> Result<()>, mut progress_fn: impl FnMut(ImportProgress) -> Result<()>,
archive: &mut ZipArchive<File>, archive: &mut ZipArchive<File>,
media_folder: &str, media_folder: &Path,
) -> Result<()> { ) -> Result<()> {
let media_entries = extract_media_entries(meta, archive)?; let media_entries = extract_media_entries(meta, archive)?;
std::fs::create_dir_all(media_folder)?; std::fs::create_dir_all(media_folder)?;
@ -117,7 +118,9 @@ fn restore_media(
} }
if let Ok(mut zip_file) = archive.by_name(&archive_file_name.to_string()) { if let Ok(mut zip_file) = archive.by_name(&archive_file_name.to_string()) {
let file_path = Path::new(&media_folder).join(normalize_to_nfc(&entry.name).as_ref()); check_filename_safe(&entry.name)?;
let nfc_name = normalize_to_nfc(&entry.name);
let file_path = media_folder.join(nfc_name.as_ref());
let size_in_colpkg = if meta.media_list_is_hashmap() { let size_in_colpkg = if meta.media_list_is_hashmap() {
zip_file.size() zip_file.size()
} else { } else {
@ -148,6 +151,20 @@ fn restore_media(
Ok(()) Ok(())
} }
/// Return an error if name contains any path separators.
fn check_filename_safe(name: &str) -> Result<()> {
let mut components = Path::new(name).components();
let first_element_normal = components
.next()
.map(|component| matches!(component, Component::Normal(_)))
.unwrap_or_default();
if !first_element_normal || components.next().is_some() {
Err(AnkiError::ImportError(ImportError::Corrupt))
} else {
Ok(())
}
}
fn extract_media_entries(meta: &Meta, archive: &mut ZipArchive<File>) -> Result<Vec<MediaEntry>> { fn extract_media_entries(meta: &Meta, archive: &mut ZipArchive<File>) -> Result<Vec<MediaEntry>> {
let mut file = archive.by_name("media")?; let mut file = archive.by_name("media")?;
let mut buf = Vec::new(); let mut buf = Vec::new();
@ -201,3 +218,24 @@ fn copy_collection(
Ok(()) Ok(())
} }
#[cfg(test)]
mod test {
use super::*;
#[test]
fn path_traversal() {
assert!(check_filename_safe("foo").is_ok(),);
assert!(check_filename_safe("..").is_err());
assert!(check_filename_safe("foo/bar").is_err());
assert!(check_filename_safe("/foo").is_err());
assert!(check_filename_safe("../foo").is_err());
if cfg!(windows) {
assert!(check_filename_safe("foo\\bar").is_err());
assert!(check_filename_safe("c:\\foo").is_err());
assert!(check_filename_safe("\\foo").is_err());
}
}
}