Skip to content

Commit

Permalink
Prevent path traversal with encoded URL (#10650)
Browse files Browse the repository at this point in the history
Signed-off-by: B-Step62 <yuki.watanabe@databricks.com>
  • Loading branch information
B-Step62 committed Dec 8, 2023
1 parent 9612ef0 commit 1da75df
Show file tree
Hide file tree
Showing 5 changed files with 183 additions and 152 deletions.
5 changes: 4 additions & 1 deletion mlflow/utils/uri.py
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,10 @@ def validate_path_is_safe(path):
"""
from mlflow.utils.file_utils import local_file_uri_to_path

exc = MlflowException(f"Invalid path: {path}", error_code=INVALID_PARAMETER_VALUE)
# We must decode URL before validating it
path = urllib.parse.unquote(path)

exc = MlflowException("Invalid path", error_code=INVALID_PARAMETER_VALUE)
if any((s in path) for s in ("#", "%23")):
raise exc

Expand Down
27 changes: 26 additions & 1 deletion tests/server/test_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,12 @@
UpdateRegisteredModel,
)
from mlflow.protos.service_pb2 import CreateExperiment, SearchRuns
from mlflow.server import BACKEND_STORE_URI_ENV_VAR, app
from mlflow.server import BACKEND_STORE_URI_ENV_VAR, SERVE_ARTIFACTS_ENV_VAR, app
from mlflow.server.handlers import (
_create_experiment,
_create_model_version,
_create_registered_model,
_delete_artifact_mlflow_artifacts,
_delete_model_version,
_delete_model_version_tag,
_delete_registered_model,
Expand Down Expand Up @@ -105,6 +106,11 @@ def mock_model_registry_store():
yield mock_store


@pytest.fixture
def enable_serve_artifacts(monkeypatch):
monkeypatch.setenv(SERVE_ARTIFACTS_ENV_VAR, "true")


def test_health():
with app.test_client() as c:
response = c.get("/health")
Expand Down Expand Up @@ -777,3 +783,22 @@ def test_get_model_version_by_alias(mock_get_request_message, mock_model_registr
_, args = mock_model_registry_store.get_model_version_by_alias.call_args
assert args == {"name": name, "alias": alias}
assert json.loads(resp.get_data()) == {"model_version": jsonify(mvd)}


@pytest.mark.parametrize(
"path",
[
"/path",
"path/../to/file",
"/etc/passwd",
"/etc/passwd%00.jpg",
"/file://etc/passwd",
"%2E%2E%2F%2E%2E%2Fpath",
],
)
def test_delete_artifact_mlflow_artifacts_throws_for_malicious_path(enable_serve_artifacts, path):
response = _delete_artifact_mlflow_artifacts(path)
assert response.status_code == 400
json_response = json.loads(response.get_data())
assert json_response["error_code"] == ErrorCode.Name(INVALID_PARAMETER_VALUE)
assert json_response["message"] == "Invalid path"
4 changes: 2 additions & 2 deletions tests/store/artifact/test_http_artifact_repo.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ def test_list_artifacts(http_artifact_repo):
http_artifact_repo.list_artifacts()


@pytest.mark.parametrize("path", ["/tmp/path", "../../path"])
@pytest.mark.parametrize("path", ["/tmp/path", "../../path", "%2E%2E%2Fpath"])
def test_list_artifacts_malicious_path(http_artifact_repo, path):
with mock.patch(
"mlflow.store.artifact.http_artifact_repo.http_request",
Expand All @@ -259,7 +259,7 @@ def test_list_artifacts_malicious_path(http_artifact_repo, path):
200,
),
):
with pytest.raises(MlflowException, match=f"Invalid path: {path}"):
with pytest.raises(MlflowException, match="Invalid path"):
http_artifact_repo.list_artifacts()


Expand Down
149 changes: 1 addition & 148 deletions tests/tracking/test_rest_tracking.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
)
from mlflow.exceptions import MlflowException
from mlflow.models import Model
from mlflow.server.handlers import validate_path_is_safe
from mlflow.store.tracking.sqlalchemy_store import SqlAlchemyStore
from mlflow.utils import mlflow_tags
from mlflow.utils.file_utils import TempDir, path_to_local_file_uri
Expand Down Expand Up @@ -514,152 +513,6 @@ def assert_bad_request(payload, expected_error_message):
assert response.status_code == 200


@pytest.mark.parametrize(
"path",
[
"path",
"path/",
"path/to/file",
],
)
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
".../...//",
],
)
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",
[
"/path",
"../path",
"../../path",
"./../path",
"path/../to/file",
"path/../../to/file",
"file://a#/..//tmp",
"file://a%23/..//tmp/",
"/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 paths
r"C:path",
r"C:path/",
r"C:path/to/file",
r"C:../path/to/file",
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 All @@ -670,7 +523,7 @@ def assert_response(resp):
assert resp.status_code == 400
assert response.json() == {
"error_code": "INVALID_PARAMETER_VALUE",
"message": f"Invalid path: {invalid_path}",
"message": "Invalid path",
}

response = requests.get(
Expand Down
Loading

0 comments on commit 1da75df

Please sign in to comment.