vmm: Prevent path traveral attack

Author: kvinwang

vmm/rpc/proto/vmm_rpc.proto

@@ -302,7 +302,7 @@ service Vmm {
   rpc BackupDisk(BackupDiskRequest) returns (google.protobuf.Empty);
 
   // List backups for a VM
-  rpc ListBackups(Id) returns (ListBackupsResponse);
+  rpc ListBackups(BackupDiskRequest) returns (ListBackupsResponse);
 
   // Delete a backup
   rpc DeleteBackup(DeleteBackupRequest) returns (google.protobuf.Empty);

vmm/src/app.rs

@@ -115,13 +115,6 @@ pub struct App {
     state: Arc<Mutex<AppState>>,
 }
 
-fn validate_filename(s: &str) -> Result<()> {
-    if s.contains("/") || s.contains("\\") {
-        bail!("Invalid filename");
-    }
-    Ok(())
-}
-
 impl App {
     fn lock(&self) -> MutexGuard<AppState> {
         self.state.lock().unwrap()
@@ -139,15 +132,12 @@ impl App {
         self.config.cvm.backup.path.join(id).join("backups")
     }
 
-    fn backup_dir(&self, id: &str, backup_id: &str) -> Result<PathBuf> {
-        validate_filename(backup_id)?;
-        let backup_dir = self.backups_dir(id).join(backup_id);
-        Ok(backup_dir)
+    fn backup_dir(&self, id: &str, backup_id: &str) -> PathBuf {
+        self.backups_dir(id).join(backup_id)
     }
 
-    fn backup_file(&self, id: &str, backup_id: &str, snapshot_id: &str) -> Result<PathBuf> {
-        validate_filename(snapshot_id)?;
-        Ok(self.backup_dir(id, backup_id)?.join(snapshot_id))
+    fn backup_file(&self, id: &str, backup_id: &str, snapshot_id: &str) -> PathBuf {
+        self.backup_dir(id, backup_id).join(snapshot_id)
     }
 
     pub fn new(config: Config, supervisor: SupervisorClient) -> Self {
@@ -837,7 +827,7 @@ impl App {
         if !self.config.cvm.backup.enabled {
             bail!("Backup is not enabled");
         }
-        let backup_dir = self.backup_dir(vm_id, backup_id)?;
+        let backup_dir = self.backup_dir(vm_id, backup_id);
         if !backup_dir.exists() {
             bail!("Backup does not exist");
         }
@@ -863,7 +853,7 @@ impl App {
             bail!("VM is not stopped: status={}", info.status);
         }
 
-        let backup_file = self.backup_file(vm_id, backup_id, snapshot_id)?;
+        let backup_file = self.backup_file(vm_id, backup_id, snapshot_id);
         if !backup_file.exists() {
             bail!("Backup file not found");
         }
@@ -873,7 +863,7 @@ impl App {
             // Just copy the file
             tokio::fs::copy(&backup_file, &hda_img).await?;
         } else {
-            let backup_dir = self.backup_dir(vm_id, backup_id)?;
+            let backup_dir = self.backup_dir(vm_id, backup_id);
             let snapshot_id = snapshot_id.to_string();
             // Rename the current hda file to *.bak
             let bak_file = hda_img.display().to_string() + ".bak";

vmm/src/main_service.rs

@@ -106,6 +106,30 @@ fn resolve_gpus(gpu_cfg: &rpc::GpuConfig) -> Result<GpuConfig> {
     }
 }
 
+fn check_path_traversal(input: &str) -> Result<()> {
+    if input.is_empty() {
+        bail!("path is empty");
+    }
+
+    if input.len() > 255 {
+        bail!("path is too long");
+    }
+
+    if input.contains('/') {
+        bail!("path contains '/'");
+    }
+
+    if input.contains('\0') {
+        bail!("path contains '\0'");
+    }
+
+    if input == "." || input == ".." {
+        bail!("path is '.' or '..'");
+    }
+
+    Ok(())
+}
+
 impl RpcHandler {
     fn resolve_gpus(&self, gpu_cfg: &rpc::GpuConfig) -> Result<GpuConfig> {
         let gpus = resolve_gpus(gpu_cfg)?;
@@ -209,6 +233,7 @@ impl VmmRpc for RpcHandler {
     }
 
     async fn start_vm(self, request: Id) -> Result<()> {
+        check_path_traversal(&request.id)?;
         self.app
             .start_vm(&request.id)
             .await
@@ -217,6 +242,7 @@ impl VmmRpc for RpcHandler {
     }
 
     async fn stop_vm(self, request: Id) -> Result<()> {
+        check_path_traversal(&request.id)?;
         self.app
             .stop_vm(&request.id)
             .await
@@ -225,6 +251,7 @@ impl VmmRpc for RpcHandler {
     }
 
     async fn remove_vm(self, request: Id) -> Result<()> {
+        check_path_traversal(&request.id)?;
         self.app
             .remove_vm(&request.id)
             .await
@@ -253,6 +280,7 @@ impl VmmRpc for RpcHandler {
     }
 
     async fn upgrade_app(self, request: UpgradeAppRequest) -> Result<Id> {
+        check_path_traversal(&request.id)?;
         let new_id = if !request.compose_file.is_empty() {
             // check the compose file is valid
             let _app_compose: AppCompose =
@@ -322,6 +350,7 @@ impl VmmRpc for RpcHandler {
     }
 
     async fn get_info(self, request: Id) -> Result<GetInfoResponse> {
+        check_path_traversal(&request.id)?;
         if let Some(vm) = self.app.vm_info(&request.id).await? {
             Ok(GetInfoResponse {
                 found: true,
@@ -337,6 +366,7 @@ impl VmmRpc for RpcHandler {
 
     #[tracing::instrument(skip(self, request), fields(id = request.id))]
     async fn resize_vm(self, request: ResizeVmRequest) -> Result<()> {
+        check_path_traversal(&request.id)?;
         info!("Resizing VM: {:?}", request);
         let vm = self
             .app
@@ -397,6 +427,7 @@ impl VmmRpc for RpcHandler {
     }
 
     async fn shutdown_vm(self, request: Id) -> Result<()> {
+        check_path_traversal(&request.id)?;
         self.guest_agent_client(&request.id)?.shutdown().await?;
         Ok(())
     }
@@ -463,21 +494,28 @@ impl VmmRpc for RpcHandler {
     }
 
     async fn backup_disk(self, request: BackupDiskRequest) -> Result<()> {
+        check_path_traversal(&request.vm_id)?;
         self.app.backup_disk(&request.vm_id, &request.level).await
     }
 
-    async fn list_backups(self, request: Id) -> Result<rpc::ListBackupsResponse> {
-        let backups = self.app.list_backups(&request.id).await?;
+    async fn list_backups(self, request: BackupDiskRequest) -> Result<rpc::ListBackupsResponse> {
+        check_path_traversal(&request.vm_id)?;
+        let backups = self.app.list_backups(&request.vm_id).await?;
         Ok(rpc::ListBackupsResponse { backups })
     }
 
     async fn delete_backup(self, request: DeleteBackupRequest) -> Result<()> {
+        check_path_traversal(&request.vm_id)?;
+        check_path_traversal(&request.backup_id)?;
         self.app
             .delete_backup(&request.vm_id, &request.backup_id)
             .await
     }
 
     async fn restore_backup(self, request: RestoreBackupRequest) -> Result<()> {
+        check_path_traversal(&request.vm_id)?;
+        check_path_traversal(&request.backup_id)?;
+        check_path_traversal(&request.snapshot_id)?;
         self.app
             .restore_backup(&request.vm_id, &request.backup_id, &request.snapshot_id)
             .await