Skip to content

Commit

Permalink
fix validate_path_is_safe function on windows (#8999)
Browse files Browse the repository at this point in the history
Signed-off-by: Serena Ruan <serena.rxy@gmail.com>
  • Loading branch information
serena-ruan committed Jul 13, 2023
1 parent 83c149f commit 0f2ad02
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 2 deletions.
11 changes: 9 additions & 2 deletions mlflow/server/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -526,11 +526,18 @@ def validate_path_is_safe(path):
"""
Validates that the specified path is safe to join with a trusted prefix. This is a security
measure to prevent path traversal attacks.
A valid path should:
not contain separators other than '/'
not contain .. to navigate to parent dir in path
not be an absolute path
"""
if is_file_uri(path):
path = local_file_uri_to_path(path)
if (
any((s in path) for s in _OS_ALT_SEPS)
or ".." in path.split(posixpath.sep)
or posixpath.isabs(path)
or ".." in path.split("/")
or pathlib.PureWindowsPath(path).is_absolute()
or pathlib.PurePosixPath(path).is_absolute()
):
raise MlflowException(f"Invalid path: {path}", error_code=INVALID_PARAMETER_VALUE)

Expand Down
115 changes: 115 additions & 0 deletions tests/tracking/test_rest_tracking.py
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,22 @@ def test_validate_path_is_safe_good(path):
validate_path_is_safe(path)


@pytest.mark.skipif(not is_windows(), reason="This test only passes on Windows")
@pytest.mark.parametrize(
"path",
[
# relative path from current directory of C: drive
"C:path",
"C:path/",
"C:path/to/file",
".../...//",
],
)
def test_validate_path_is_safe_windows_good(path):
validate_path_is_safe(path)


@pytest.mark.skipif(is_windows(), reason="This test does not pass on Windows")
@pytest.mark.parametrize(
"path",
[
Expand All @@ -537,13 +553,112 @@ def test_validate_path_is_safe_good(path):
"./../path",
"path/../to/file",
"path/../../to/file",
"/etc/passwd",
"/etc/passwd%00.jpg",
"/etc/passwd%00.html",
"/etc/passwd%00.txt",
"/etc/passwd%00.php",
"/etc/passwd%00.asp",
"/file://etc/passwd",
],
)
def test_validate_path_is_safe_bad(path):
with pytest.raises(MlflowException, match="Invalid path"):
validate_path_is_safe(path)


@pytest.mark.skipif(not is_windows(), reason="This test only passes on Windows")
@pytest.mark.parametrize(
"path",
[
r"../path",
r"../../path",
r"./../path",
r"path/../to/file",
r"path/../../to/file",
r"..\path",
r"..\..\path",
r".\..\path",
r"path\..\to\file",
r"path\..\..\to\file",
# Drive-relative absolute paths
r"C:\path",
r"C:/path",
r"C:\path\to\file",
r"C:\path/to/file",
r"C:\path\..\to\file",
r"C:/path/../to/file",
# UNC(Universal Naming Convention) paths
r"\\path\to\file",
r"\\path/to/file",
r"\\.\\C:\path\to\file",
r"\\?\C:\path\to\file",
r"\\?\UNC/path/to/file",
# Other potential attackable paths
r"/etc/password",
r"/path",
r"/etc/passwd%00.jpg",
r"/etc/passwd%00.html",
r"/etc/passwd%00.txt",
r"/etc/passwd%00.php",
r"/etc/passwd%00.asp",
r"/Windows/no/such/path",
r"/file://etc/passwd",
r"/file:c:/passwd",
r"/file://d:/windows/win.ini",
r"/file://./windows/win.ini",
r"file://c:/boot.ini",
r"file://C:path",
r"file://C:path/",
r"file://C:path/to/file",
r"file:///C:/Windows/System32/",
r"file:///etc/passwd",
r"file:///d:/windows/repair/sam",
r"file:///proc/version",
r"file:///inetpub/wwwroot/global.asa",
r"/file://../windows/win.ini",
r"../etc/passwd",
r"..\Windows\System32\\",
r"C:\Windows\System32\\",
r"/etc/passwd",
r"::Windows\System32",
r"..\..\..\..\Windows\System32\\",
r"../Windows/System32",
r"....\\",
r"\\?\C:\Windows\System32\\",
r"\\.\C:\Windows\System32\\",
r"\\UNC\Server\Share\\",
r"\\Server\Share\folder\\",
r"\\127.0.0.1\c$\Windows\\",
r"\\localhost\c$\Windows\\",
r"\\smbserver\share\path\\",
r"..\\?\C:\Windows\System32\\",
r"C:/Windows/../Windows/System32/",
r"C:\Windows\..\Windows\System32\\",
r"../../../../../../../../../../../../Windows/System32",
r"../../../../../../../../../../../../etc/passwd",
r"../../../../../../../../../../../../var/www/html/index.html",
r"../../../../../../../../../../../../usr/local/etc/openvpn/server.conf",
r"../../../../../../../../../../../../Program Files (x86)",
r"/../../../../../../../../../../../../Windows/System32",
r"/Windows\../etc/passwd",
r"/Windows\..\Windows\System32\\",
r"/Windows\..\Windows\System32\cmd.exe",
r"/Windows\..\Windows\System32\msconfig.exe",
r"/Windows\..\Windows\System32\regedit.exe",
r"/Windows\..\Windows\System32\taskmgr.exe",
r"/Windows\..\Windows\System32\control.exe",
r"/Windows\..\Windows\System32\services.msc",
r"/Windows\..\Windows\System32\diskmgmt.msc",
r"/Windows\..\Windows\System32\eventvwr.msc",
r"/Windows/System32/drivers/etc/hosts",
],
)
def test_validate_path_is_safe_windows_bad(path):
with pytest.raises(MlflowException, match="Invalid path"):
validate_path_is_safe(path)


def test_path_validation(mlflow_client):
experiment_id = mlflow_client.create_experiment("tags validation")
created_run = mlflow_client.create_run(experiment_id)
Expand Down

0 comments on commit 0f2ad02

Please sign in to comment.