Disable '..' in query string for artifact URI

[B-Step62]

🛠 DevTools 🛠

Install mlflow from this PR

pip install git+https://github.com/mlflow/mlflow.git@refs/pull/10653/merge

Checkout with GitHub CLI

gh pr checkout 10653

What changes are proposed in this pull request?

Solving path traversal vulnerability.

Problem

When users create experiment with base artifact URI like http://foo/bar, MLflow appends /{{run_id}}/artifact sub path to it when saving/reading each run's artifact, such as `http://foo/bar/{{run_id}}/artifact". Every GET request for artifacts are validated so that the requested path are under this path, effectively prevents attackers to read any files outside the that run directory.

However, there is one hack to bypass this, which is query string. When MLflow appends /{{run_id}}/artifact, it will be inserted before query string of the specified artifact root. For example, if the artifact root is http://foo/bar?a=a, the run's artifact URI will be ``http://foo/bar{{run_id}}/artifact?a=a`.

This allows path traversal by adding malformed query string like "../../../../etc", which results in run's artifact location to be http://foo/bar{{run_id}}/artifact?../../../../etc, which is then resolved to /etc as a local path.

Solution

This PR resolves this by explicitly validating query string passed as a part of artifact URI. It simply check if the query string contains ".." or not (with decoding).

There were some alternatives considered:

• Ignore query string entirely when appending {{run_id}}/artifacts => I'm afraid that some cases we need query string to access artifact location e.g. S3 ARN can have a region as query string.
• Remove all string after "?" when resolving local path. => "?" is a valid character in some OS.

Currently users can specify any query strings to artifact URI, for example, "http:///?/../path".

How is this PR tested?

• Existing unit/integration tests
• New unit/integration tests
• Manual tests

Validated the fix prevents experiment creation with malformed query string:

curl -X POST -H 'Content-Type: application/json' -d '{"name": "poc", "artifact_location": "http:///??/../../../../../../../../../../../../../../etc/"}' 'http://127.0.0.1:5000/ajax-api/2.0/mlflow/experiments/create'

{"error_code": "INVALID_PARAMETER_VALUE", "message": "Invalid query string"}

Does this PR require documentation update?

• No. You can skip the rest of this section.
• Yes. I've updated:
  • Examples
  • API references
  • Instructions

Release Notes

Is this a user-facing change?

• No. You can skip the rest of this section.
• Yes. Give a description of this change to be included in the release notes for MLflow users.

What component(s), interfaces, languages, and integrations does this PR affect?

Components

• area/artifacts: Artifact stores and artifact logging
• area/build: Build and test infrastructure for MLflow
• area/deployments: MLflow Deployments client APIs, server, and third-party Deployments integrations
• area/docs: MLflow documentation pages
• area/examples: Example code
• area/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registry
• area/models: MLmodel format, model serialization/deserialization, flavors
• area/recipes: Recipes, Recipe APIs, Recipe configs, Recipe Templates
• area/projects: MLproject format, project running backends
• area/scoring: MLflow Model server, model deployment tools, Spark UDFs
• area/server-infra: MLflow Tracking server backend
• area/tracking: Tracking Service, tracking client APIs, autologging

Interface

• area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
• area/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
• area/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registry
• area/windows: Windows support

Language

• language/r: R APIs and clients
• language/java: Java APIs and clients
• language/new: Proposals for new client languages

Integrations

• integrations/azure: Azure and Azure ML integrations
• integrations/sagemaker: SageMaker integrations
• integrations/databricks: Databricks integrations

How should the PR be classified in the release notes? Choose one:

• rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
• rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
• rn/feature - A new user-facing feature worth mentioning in the release notes
• rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
• rn/documentation - A user-facing documentation change worth mentioning in the release notes

[github-actions]

Documentation preview for 3ca7f44 will be available here when this CircleCI job completes successfully.

More info

• Ignore this comment if this PR does not change the documentation.
• It takes a few minutes for the preview to be available.
• The preview is updated when a new commit is pushed to this PR.
• This comment was created by https://github.com/mlflow/mlflow/actions/runs/7138521782.

[harupy]

/ai any chance that this function goes into an infinite loop?

[mlflow]

@harupy

Yes, there is a chance that this function could go into an infinite loop if urllib.parse.unquote(url) continually returns a value that is not equal to the original url. However, this is extremely unlikely due to how urllib.parse.unquote() operates. It decodes percent-encoded characters in a URL into their corresponding characters. In practice, it means that with each loop, the URL should become less percent-encoded, and eventually, it should be fully decoded so that decoded == url and the function would then exit the loop. But if there's a strange behavior with unquote() or the input url constantly changes outside this function in a multithreaded environment it might cause an infinite loop.

Click here to see the usage data

{
  "prompt_tokens": 91,
  "completion_tokens": 149,
  "total_tokens": 240,
  "estimated_cost_in_usd": 0.01167
}

[harupy]

/ai Can we use recursion instead?

[mlflow]

@harupy

Yes, it is possible to re-implement this function using recursion. Here would be a possible solution:

def _decode(url):
    decoded = urllib.parse.unquote(url)
    if decoded == url:
        return url
    else:
        return _decode(decoded)

This version of the function does essentially the same thing as the original - It decodes the URL, and continue to decode the result until decoding does not produce any changes.

Remember though, Python has a maximum recursion depth (usually 1000). While in normal URL decoding cases you would likely never reach near it, malformed or malicious strings may cause a maximum recursion depth exceeded error. Proper error handling or a depth check should be added if this is a concern.

Click here to see the usage data

{
  "prompt_tokens": 256,
  "completion_tokens": 151,
  "total_tokens": 407,
  "estimated_cost_in_usd": 0.01674
}

[harupy]

I'd use this:

for _ in range(100):
    decoded = urllib.parse.unquote(url)
    if decoded == url:
        return url
    url = decoded

raise ValueError("Failed to decode")

# No chance for an infinite loop

[serena-ruan]

I feel like 10 is enough 🤔 Who would encode a path to such complex stuff?

[harupy]

Sounds good. 10 should be enough.

[B-Step62]

Makes sense, will limit it to 10:)

[harupy]

If we reach 10, I think that's some malicious URI.

[serena-ruan]

LGTM!

[harupy]

LGTM