Skip to content

Commit aff7aad

Browse files
garciadiaspre-commit-ci[bot]ericspod
authored
Harden download/deserialize integrity (weights_only, hash checks, path confinement) (#9088)
### Description Harden the download/deserialization chain: - Pass `weights_only=True` to pretrained weight loaders in `senet`, `densenet`, `efficientnet`, and `text_embedding` so a substituted `.pth` cannot unpickle code (GHSA-vm9c-7j6g-c7mm). - `check_hash`: emit a `UserWarning` when no hash value is provided instead of silently passing, and default `check_hash`/`download_url` to `sha256` (GHSA-hhh4-h52m-fqh6). - `download_large_files`: confine large-file targets to the bundle directory, rejecting absolute and `../` traversal (GHSA-x4pc-gj5h-3pq7). Note: SENet pretrained URLs remain `http://` because the upstream host does not serve the files over HTTPS (verified unreachable); `weights_only=True` closes the code-execution vector, leaving only a transport-integrity gap. ### Types of changes - [ ] Non-breaking change - [x] Breaking change (default hash type changes from md5 to sha256 for `check_hash`/`download_url`; callers that relied on the md5 default now pass `hash_type` explicitly) - [x] New tests added to cover the changes. --------- Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
1 parent 434c094 commit aff7aad

8 files changed

Lines changed: 59 additions & 20 deletions

File tree

monai/apps/utils.py

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -166,20 +166,20 @@ def safe_extract_member(member, extract_to):
166166
return full_path
167167

168168

169-
def check_hash(filepath: PathLike, val: str | None = None, hash_type: str = "md5") -> bool:
169+
def check_hash(filepath: PathLike, val: str | None = None, hash_type: str = "sha256") -> bool:
170170
"""
171171
Verify hash signature of specified file.
172172
173173
Args:
174174
filepath: path of source file to verify hash value.
175175
val: expected hash value of the file.
176-
hash_type: type of hash algorithm to use, default is `"md5"`.
176+
hash_type: type of hash algorithm to use, default is `"sha256"`.
177177
The supported hash types are `"md5"`, `"sha1"`, `"sha256"`, `"sha512"`.
178178
See also: :py:data:`monai.apps.utils.SUPPORTED_HASH_TYPES`.
179179
180180
"""
181181
if val is None:
182-
logger.info(f"Expected {hash_type} is None, skip {hash_type} check for file {filepath}.")
182+
warnings.warn(f"No hash value provided for {filepath}; file integrity is NOT verified.", stacklevel=2)
183183
return True
184184
actual_hash_func = look_up_option(hash_type.lower(), SUPPORTED_HASH_TYPES)
185185

@@ -204,7 +204,7 @@ def download_url(
204204
url: str,
205205
filepath: PathLike = "",
206206
hash_val: str | None = None,
207-
hash_type: str = "md5",
207+
hash_type: str = "sha256",
208208
progress: bool = True,
209209
**gdown_kwargs: Any,
210210
) -> None:
@@ -217,7 +217,8 @@ def download_url(
217217
If undefined, `os.path.basename(url)` will be used.
218218
hash_val: expected hash value to validate the downloaded file.
219219
if None, skip hash validation.
220-
hash_type: 'md5' or 'sha1', defaults to 'md5'.
220+
hash_type: type of hash algorithm to use, default is `"sha256"`.
221+
The supported hash types are `"md5"`, `"sha1"`, `"sha256"`, `"sha512"`.
221222
progress: whether to display a progress bar.
222223
gdown_kwargs: other args for `gdown` except for the `url`, `output` and `quiet`.
223224
these args will only be used if download from google drive.
@@ -315,7 +316,7 @@ def extractall(
315316
filepath: PathLike,
316317
output_dir: PathLike = ".",
317318
hash_val: str | None = None,
318-
hash_type: str = "md5",
319+
hash_type: str = "sha256",
319320
file_type: str = "",
320321
has_base: bool = True,
321322
) -> None:
@@ -328,7 +329,7 @@ def extractall(
328329
output_dir: target directory to save extracted files.
329330
hash_val: expected hash value to validate the compressed file.
330331
if None, skip hash validation.
331-
hash_type: 'md5' or 'sha1', defaults to 'md5'.
332+
hash_type: type of hash algorithm to use, default is `"sha256"`.
332333
file_type: string of file type for decompressing. Leave it empty to infer the type from the filepath basename.
333334
has_base: whether the extracted files have a base folder. This flag is used when checking if the existing
334335
folder is a result of `extractall`, if it is, the extraction is skipped. For example, if A.zip is unzipped
@@ -394,7 +395,7 @@ def download_and_extract(
394395
filepath: PathLike = "",
395396
output_dir: PathLike = ".",
396397
hash_val: str | None = None,
397-
hash_type: str = "md5",
398+
hash_type: str = "sha256",
398399
file_type: str = "",
399400
has_base: bool = True,
400401
progress: bool = True,
@@ -410,7 +411,7 @@ def download_and_extract(
410411
default is the current directory.
411412
hash_val: expected hash value to validate the downloaded file.
412413
if None, skip hash validation.
413-
hash_type: 'md5' or 'sha1', defaults to 'md5'.
414+
hash_type: type of hash algorithm to use, default is `"sha256"`.
414415
file_type: string of file type for decompressing. Leave it empty to infer the type from url's base file name.
415416
has_base: whether the extracted files have a base folder. This flag is used when checking if the existing
416417
folder is a result of `extractall`, if it is, the extraction is skipped. For example, if A.zip is unzipped

monai/bundle/scripts.py

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2008,6 +2008,17 @@ def create_workflow(
20082008
return workflow_
20092009

20102010

2011+
def _safe_large_file_path(bundle_path: PathLike, filepath: str) -> str:
2012+
"""Securely resolve a large-file target path to prevent traversal outside the bundle directory."""
2013+
bundle_root = os.path.realpath(bundle_path)
2014+
target = os.path.normpath(os.path.join(bundle_path, filepath))
2015+
target_real = os.path.realpath(target)
2016+
# Ensure the resolved path stays within the bundle root
2017+
if os.path.commonpath([bundle_root, target_real]) != bundle_root:
2018+
raise ValueError(f"Unsafe path: path traversal {filepath} for bundle_path {bundle_path}")
2019+
return target
2020+
2021+
20112022
def download_large_files(bundle_path: str | None = None, large_file_name: str | None = None) -> None:
20122023
"""
20132024
This utility allows you to download large files from a bundle. It supports file suffixes like ".yml", ".yaml", and ".json".
@@ -2042,6 +2053,6 @@ def download_large_files(bundle_path: str | None = None, large_file_name: str |
20422053
lf_data.pop("hash_val")
20432054
if "hash_type" in lf_data and lf_data.get("hash_type", "") == "":
20442055
lf_data.pop("hash_type")
2045-
lf_data["filepath"] = os.path.join(bundle_path, lf_data["path"])
2056+
lf_data["filepath"] = _safe_large_file_path(bundle_path, lf_data["path"])
20462057
lf_data.pop("path")
20472058
download_url(**lf_data)

monai/networks/blocks/text_embedding.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ def __init__(
6767

6868
if pretrained:
6969
model_url = url_map[self.encoding]
70-
pretrain_state_dict = model_zoo.load_url(model_url, map_location="cpu")
70+
pretrain_state_dict = model_zoo.load_url(model_url, map_location="cpu", weights_only=True)
7171
self.text_embedding.data = pretrain_state_dict.float() # type: ignore
7272
else:
7373
print(f"{self.encoding} is not implemented, and can not be downloaded, please load your own")

monai/networks/nets/densenet.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ def _load_state_dict(model: nn.Module, arch: str, progress: bool):
277277
r"^(.*denselayer\d+)(\.(?:norm|relu|conv))\.((?:[12])\.(?:weight|bias|running_mean|running_var))$"
278278
)
279279

280-
state_dict = load_state_dict_from_url(model_url, progress=progress)
280+
state_dict = load_state_dict_from_url(model_url, progress=progress, weights_only=True)
281281
for key in list(state_dict.keys()):
282282
res = pattern.match(key)
283283
if res:

monai/networks/nets/efficientnet.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -793,7 +793,7 @@ def _load_state_dict(model: nn.Module, arch: str, progress: bool, adv_prop: bool
793793
else:
794794
# load state dict from url
795795
model_url = url_map[arch]
796-
pretrain_state_dict = model_zoo.load_url(model_url, progress=progress)
796+
pretrain_state_dict = model_zoo.load_url(model_url, progress=progress, weights_only=True)
797797
model_state_dict = model.state_dict()
798798

799799
pattern = re.compile(r"(.+)\.\d+(\.\d+\..+)")

monai/networks/nets/senet.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ def _load_state_dict(model: nn.Module, arch: str, progress: bool):
304304
download_url(model_url["url"], filepath=model_url["filename"])
305305
state_dict = torch.load(model_url["filename"], map_location=None, weights_only=True)
306306
else:
307-
state_dict = load_state_dict_from_url(model_url, progress=progress)
307+
state_dict = load_state_dict_from_url(model_url, progress=progress, weights_only=True)
308308
for key in list(state_dict.keys()):
309309
new_key = None
310310
if pattern_conv.match(key):

tests/apps/test_check_hash.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
from __future__ import annotations
1313

14+
import hashlib
1415
import os
1516
import tempfile
1617
import unittest
@@ -48,6 +49,23 @@ def test_hash_type_error(self):
4849
with tempfile.TemporaryDirectory() as tempdir:
4950
check_hash(tempdir, "test_hash", "test_type")
5051

52+
def test_warns_when_val_is_none(self):
53+
test_image = np.ones((5, 5, 3))
54+
with tempfile.TemporaryDirectory() as tempdir:
55+
filename = os.path.join(tempdir, "test_file.png")
56+
test_image.tofile(filename)
57+
with self.assertWarns(UserWarning):
58+
result = check_hash(filename, None)
59+
self.assertTrue(result)
60+
61+
def test_default_hash_type_is_sha256(self):
62+
test_image = np.ones((5, 5, 3))
63+
with tempfile.TemporaryDirectory() as tempdir:
64+
filename = os.path.join(tempdir, "test_file.png")
65+
test_image.tofile(filename)
66+
sha256 = hashlib.sha256(test_image.tobytes()).hexdigest()
67+
self.assertTrue(check_hash(filename, sha256))
68+
5169

5270
if __name__ == "__main__":
5371
unittest.main()

tests/bundle/test_bundle_download.py

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
import monai.networks.nets as nets
2727
from monai.apps import check_hash
2828
from monai.bundle import ConfigParser, create_workflow, load, run
29-
from monai.bundle.scripts import _examine_monai_version, _list_latest_versions, download
29+
from monai.bundle.scripts import _examine_monai_version, _list_latest_versions, download, download_large_files
3030
from monai.utils import optional_import
3131
from tests.test_utils import (
3232
assert_allclose,
@@ -166,7 +166,7 @@ def test_github_download_bundle(self, bundle_name, version):
166166
file_path = os.path.join(tempdir, "test_bundle", file)
167167
self.assertTrue(os.path.exists(file_path))
168168
if file == "network.json":
169-
self.assertTrue(check_hash(filepath=file_path, val=hash_val))
169+
self.assertTrue(check_hash(filepath=file_path, val=hash_val, hash_type="md5"))
170170

171171
@parameterized.expand([TEST_CASE_3])
172172
@skip_if_quick
@@ -184,8 +184,8 @@ def test_url_download_bundle(self, bundle_files, bundle_name, url, hash_val):
184184
for file in bundle_files:
185185
file_path = os.path.join(tempdir, bundle_name, file)
186186
self.assertTrue(os.path.exists(file_path))
187-
if file == "network.json":
188-
self.assertTrue(check_hash(filepath=file_path, val=hash_val))
187+
if file == "network.json":
188+
self.assertTrue(check_hash(filepath=file_path, val=hash_val, hash_type="md5"))
189189

190190
@parameterized.expand([TEST_CASE_4])
191191
@skip_if_quick
@@ -445,6 +445,15 @@ def test_load_ts_module(self, bundle_files, bundle_name, version, repo, device,
445445

446446

447447
class TestDownloadLargefiles(unittest.TestCase):
448+
449+
def test_large_files_rejects_path_traversal(self):
450+
with tempfile.TemporaryDirectory() as tempdir:
451+
large_files_path = os.path.join(tempdir, "large_files.yaml")
452+
with open(large_files_path, "w") as f:
453+
f.write("large_files:\n" " - path: ../evil.pt\n" " url: https://example.com/evil.pt\n")
454+
with self.assertRaises(ValueError):
455+
download_large_files(bundle_path=tempdir)
456+
448457
@parameterized.expand([TEST_CASE_10])
449458
@skip_if_quick
450459
def test_url_download_large_files(self, bundle_files, bundle_name, url, hash_val):
@@ -469,7 +478,7 @@ def test_url_download_large_files(self, bundle_files, bundle_name, url, hash_val
469478
command_line_tests(cmd)
470479
for file in ["model.pt", "model.ts"]:
471480
file_path = os.path.join(tempdir, bundle_name, f"models/{file}")
472-
self.assertTrue(check_hash(filepath=file_path, val=hash_val[file]))
481+
self.assertTrue(check_hash(filepath=file_path, val=hash_val[file], hash_type="md5"))
473482

474483

475484
@skip_if_windows
@@ -484,7 +493,7 @@ def test_ngc_download_bundle(self, bundle_name, version, remove_prefix, download
484493
)
485494
full_file_path = os.path.join(tempdir, download_name, file_path)
486495
self.assertTrue(os.path.exists(full_file_path))
487-
self.assertTrue(check_hash(filepath=full_file_path, val=hash_val))
496+
self.assertTrue(check_hash(filepath=full_file_path, val=hash_val, hash_type="md5"))
488497

489498
model = load(
490499
name=bundle_name, source="ngc", version=version, bundle_dir=tempdir, remove_prefix=remove_prefix

0 commit comments

Comments
 (0)