CVE-2023-6975: Path Traversal: '\..\filename' (CWE-29)
value: A malicious user could use this issue to get command execution on the vulnerable machine and get access to data & models information.
CWE description: The product uses external input to construct a pathname that should be within a restricted directory, but it does not properly neutralize '\..\filename' (leading backslash dot dot) sequences that can resolve to a location that is outside of that directory.
CVSS v3 base score: 9.8
Published: 2023-12-20T06:15Z
Repository: https://github.com/mlflow/mlflow
Language: Python
Files changed: mlflow/store/artifact/ftp_artifact_repo.py, tests/store/artifact/test_ftp_artifact_repo.py
Commit date: 2023-12-12 17:35:51 +0900
Commit message: Apply `os.path.basename` before `.` or `..` check in `FTPArtifactRepository` (#10657)

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Diff: diff --git a/mlflow/store/artifact/ftp_artifact_repo.py b/mlflow/store/artifact/ftp_artifact_repo.py
index 9f925e880f..36dcb35b90 100644
--- a/mlflow/store/artifact/ftp_artifact_repo.py
+++ b/mlflow/store/artifact/ftp_artifact_repo.py
@@ -104,14 +104,14 @@ class FTPArtifactRepository(ArtifactRepository):
             artifact_dir = self.path
             list_dir = posixpath.join(artifact_dir, path) if path else artifact_dir
             if not self._is_dir(ftp, list_dir):
                 return []
             artifact_files = ftp.nlst(list_dir)
-            artifact_files = list(filter(lambda x: x != "." and x != "..", artifact_files))
             # Make sure artifact_files is a list of file names because ftp.nlst
             # may return absolute paths.
             artifact_files = [os.path.basename(f) for f in artifact_files]
+            artifact_files = list(filter(lambda x: x != "." and x != "..", artifact_files))
             infos = []
             for file_name in artifact_files:
                 file_path = file_name if path is None else posixpath.join(path, file_name)
                 full_file_path = posixpath.join(list_dir, file_name)
                 if self._is_dir(ftp, full_file_path):
diff --git a/tests/store/artifact/test_ftp_artifact_repo.py b/tests/store/artifact/test_ftp_artifact_repo.py
index 898430cd0b..628ff601a7 100644
--- a/tests/store/artifact/test_ftp_artifact_repo.py
+++ b/tests/store/artifact/test_ftp_artifact_repo.py
@@ -65,10 +65,22 @@ def test_list_artifacts(ftp_mock):
     assert artifacts[1].path == dir_path
     assert artifacts[1].is_dir is True
     assert artifacts[1].file_size is None
 
 
+def test_list_artifacts_malicious_path(ftp_mock):
+    artifact_root_path = "/experiment_id/run_id/"
+    repo = FTPArtifactRepository("ftp://test_ftp" + artifact_root_path)
+    repo.get_ftp_client = MagicMock()
+    call_mock = MagicMock(return_value=ftp_mock)
+    repo.get_ftp_client.return_value = MagicMock(__enter__=call_mock)
+    ftp_mock.nlst = MagicMock(return_value=[".", "/.", "/..", "//.."])
+
+    artifacts = repo.list_artifacts(path=None)
+    assert artifacts == []
+
+
 def test_list_artifacts_when_ftp_nlst_returns_absolute_paths(ftp_mock):
     artifact_root_path = "/experiment_id/run_id/"
     repo = FTPArtifactRepository("ftp://test_ftp" + artifact_root_path)
 
     repo.get_ftp_client = MagicMock()

Vulnerable code:             artifact_files = list(filter(lambda x: x != "." and x != "..", artifact_files))
Fixed code:             artifact_files = list(filter(lambda x: x != "." and x != "..", artifact_files))
def test_list_artifacts_malicious_path(ftp_mock):
    artifact_root_path = "/experiment_id/run_id/"
    repo = FTPArtifactRepository("ftp://test_ftp" + artifact_root_path)
    repo.get_ftp_client = MagicMock()
    call_mock = MagicMock(return_value=ftp_mock)
    repo.get_ftp_client.return_value = MagicMock(__enter__=call_mock)
    ftp_mock.nlst = MagicMock(return_value=[".", "/.", "/..", "//.."])

    artifacts = repo.list_artifacts(path=None)
    assert artifacts == []