Skip to content

Commit

Permalink
Validate file ID for path traversal
Browse files Browse the repository at this point in the history
Signed-off-by: Mateusz Szczygieł <mateusz.szczygiel@nordsec.com>
  • Loading branch information
matszczygiel committed Jul 12, 2024
1 parent c704b34 commit bd24e27
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 1 deletion.
1 change: 1 addition & 0 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
### **Fresh air**
---
* Remove support for deprecated, unsecure protocols V1, V2, V4 and V5
* Sanitaze file ID before downloading a file, to prevent the temporary file from being allowed to traverse parent dirs

---
<br>
Expand Down
42 changes: 41 additions & 1 deletion drop-transfer/src/ws/server/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,7 @@ impl FileXferTask {
) {
let task = async {
validate_subpath_for_download(self.file.subpath())?;
validate_file_id_for_download(self.file.id())?;

let emit_checksum_events = {
if let Some(threshold) = state.config.checksum_events_size_threshold {
Expand Down Expand Up @@ -1164,9 +1165,24 @@ fn validate_subpath_for_download(subpath: &FileSubPath) -> crate::Result<()> {
Ok(())
}

/// Check file ID for illegal characters so that the temp file is not created in
/// parent directories
fn validate_file_id_for_download(file_id: &FileId) -> crate::Result<()> {
// Ideally we would allow only alphanumeric characters but we use URL base64
// which allows for more
const DISALLOWED: &[char] = &['.', '/', '\\', ':'];

let invalid = file_id.as_ref().chars().any(|c| DISALLOWED.contains(&c));
if invalid {
return Err(Error::BadFileId);
}

Ok(())
}

#[cfg(test)]
mod tests {
use crate::file::FileSubPath;
use crate::{file::FileSubPath, FileId};

#[test]
fn validate_subpath() {
Expand All @@ -1188,4 +1204,28 @@ mod tests {
Err(crate::Error::FilenameTooLong)
));
}

#[test]
fn validate_file_id() {
let id = FileId::from("x_ALOz5YRCXZSiS5V5Pd8VN_qqC6abyDyIBnjitzGe8");
assert!(super::validate_file_id_for_download(&id).is_ok());

let id = FileId::from("../../asdfasdf");
assert!(matches!(
super::validate_file_id_for_download(&id),
Err(crate::Error::BadFileId)
));

let id = FileId::from("/asdfasdf");
assert!(matches!(
super::validate_file_id_for_download(&id),
Err(crate::Error::BadFileId)
));

let id = FileId::from("C:\\ABC");
assert!(matches!(
super::validate_file_id_for_download(&id),
Err(crate::Error::BadFileId)
));
}
}

0 comments on commit bd24e27

Please sign in to comment.