Skip to content

Commit

Permalink
Merge pull request #338 from NordSecurity/msz/FILE-608-windows-path-t…
Browse files Browse the repository at this point in the history
…raversal-via-websocket-v-4

Validate file ID for path traversal
  • Loading branch information
matszczygiel authored Jul 15, 2024
2 parents c704b34 + f4c4d4f commit ffd54c9
Show file tree
Hide file tree
Showing 2 changed files with 50 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
* Sanitize file ID before downloading a file, to prevent the temporary file from being allowed to traverse parent dirs

---
<br>
Expand Down
50 changes: 49 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,26 @@ 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<()> {
const ALLOWED_BESIDE_ALPHANUMERIC: &[char] = &['_', '-', '=']; // Add '=' just in case the padding is added

fn is_char_valid(c: char) -> bool {
c.is_ascii_alphanumeric() || ALLOWED_BESIDE_ALPHANUMERIC.contains(&c)
}

let valid = file_id.as_ref().chars().all(is_char_valid);
if !valid {
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 +1206,34 @@ 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("..");
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 ffd54c9

Please sign in to comment.