Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security fix to prevent allowed_urls Filter Bypass #3082

Merged
merged 7 commits into from
Apr 11, 2024
Merged

Conversation

udaij12
Copy link
Collaborator

@udaij12 udaij12 commented Apr 10, 2024

Description

Security fix to prevent allowed_urls file:// Filter Bypass
Despite the error of Relative path is not allowed in url:, the targeted MAR file is copied from the source to the model-store folder.

Issue example:

jainudai@6cb1339aade6 serve % ls model_store/                      
densenet161.mar resnet-18.mar
jainudai@6cb1339aade6 serve % torchserve --start --model-store model_store --ncs                                                
Torchserve version: 0.10.0
TS Home: /Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages
Current directory: 
Temp directory: /var/folders/l6/xh6snvx50_v90tr7rzxhxdyr0000gr/T/
Metrics config path: /Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/ts/configs/metrics.yaml
Number of GPUs: 16
Number of CPUs: 10
Max heap size: 8192 M
Python executable: /Library/Frameworks/Python.framework/Versions/3.11/bin/python3.11
Config file: N/A
Inference address: http://127.0.0.1:8080
Management address: http://127.0.0.1:8081
Metrics address: http://127.0.0.1:8082
Model Store: .../serve/model_store
Initial Models: N/A
Log dir: .../serve/logs
Metrics dir: .../serve/logs
Netty threads: 0
Netty client threads: 0
Default workers per model: 16
Blacklist Regex: N/A
Maximum Response Size: 6553500
Maximum Request Size: 6553500
Limit Maximum Image Pixels: true
Prefer direct buffer: false
Allowed Urls: [file://.*|http(s)?://.*]
Custom python dependency for model allowed: false
Enable metrics API: true
Metrics mode: LOG
Disable system metrics: false
Workflow Store: .../serve/model_store
CPP log config: N/A
Model config: N/A
System metrics command: default
...
Model server started.
jainudai@6cb1339aade6 serve % curl -X POST 'http://localhost:8081/models?url=file:///../Users/jainudai/Documents/Torchserve/serve/model_store_gen/alexnet.mar'

{
  "code": 404,
  "type": "ModelNotFoundException",
  "message": "Relative path is not allowed in url: file:///../Users/jainudai/Documents/Torchserve/serve/model_store_gen/alexnet.mar"
}
jainudai@6cb1339aade6 serve % ls model_store/                                                                                   
alexnet.mar     densenet161.mar resnet-18.mar

Tests

Added test to check file doesnt exist when using relative path

Logs after fix

jainudai@6cb1339aade6 serve % ls model_store/           
densenet161.mar resnet-18.mar
jainudai@6cb1339aade6 serve % torchserve --start --model-store model_store --ncs
Torchserve version: 0.10.0
TS Home: /Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages
Current directory: .../serve
Temp directory: /var/folders/l6/xh6snvx50_v90tr7rzxhxdyr0000gr/T/
Metrics config path: /Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/ts/configs/metrics.yaml
Number of GPUs: 16
Number of CPUs: 10
Max heap size: 8192 M
Python executable: /Library/Frameworks/Python.framework/Versions/3.11/bin/python3.11
Config file: N/A
Inference address: http://127.0.0.1:8080
Management address: http://127.0.0.1:8081
Metrics address: http://127.0.0.1:8082
Model Store: .../serve/model_store
Initial Models: N/A
Log dir: .../serve/logs
Metrics dir: .../serve/logs
Netty threads: 0
Netty client threads: 0
Default workers per model: 16
Blacklist Regex: N/A
Maximum Response Size: 6553500
Maximum Request Size: 6553500
Limit Maximum Image Pixels: true
Prefer direct buffer: false
Allowed Urls: [file://.*|http(s)?://.*]
Custom python dependency for model allowed: false
Enable metrics API: true
Metrics mode: LOG
Disable system metrics: false
Workflow Store: .../serve/model_store
CPP log config: N/A
Model config: N/A
System metrics command: default
...
Model server started.
jainudai@6cb1339aade6 serve % curl -X POST 'http://localhost:8081/models?url=file:///../Users/jainudai/Documents/Torchserve/serve/model_store_gen/alexnet.mar'
{
  "code": 404,
  "type": "ModelNotFoundException",
  "message": "Relative path is not allowed in url: file:///../Users/jainudai/Documents/Torchserve/serve/model_store_gen/alexnet.mar"
}
jainudai@6cb1339aade6 serve % ls model_store/                                   
densenet161.mar resnet-18.mar

@udaij12 udaij12 changed the title Security fix to prevent allowed_urls file:// Filter Bypass Security fix to prevent allowed_urls Filter Bypass Apr 11, 2024
@udaij12 udaij12 marked this pull request as ready for review April 11, 2024 02:36
Copy link
Collaborator

@agunapal agunapal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@udaij12

In the PR, please add the logs of the new behavior after the fix.

@msaroufim msaroufim self-requested a review April 11, 2024 15:38
Copy link
Collaborator

@agunapal agunapal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@msaroufim msaroufim added this pull request to the merge queue Apr 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 11, 2024
@msaroufim msaroufim added this pull request to the merge queue Apr 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 11, 2024
@msaroufim msaroufim added this pull request to the merge queue Apr 11, 2024
Merged via the queue into master with commit cdba0fd Apr 11, 2024
13 checks passed
@msaroufim msaroufim deleted the security_fix branch April 11, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants