Create SECURITY.md #8546
Create SECURITY.md #8546
Conversation
Signed-off-by: h3rrr <[email protected]>
WalkthroughAdds a new SECURITY.md containing MONAI's security policy (reporting via GitHub Security Advisories, required report details, 72‑hour acknowledgement, coordinated disclosure, researcher credit, acknowledgements). In monai/apps/utils.py a new helper safe_extract_member(member, extract_to) was added and extractall was reworked to perform per-member secure extraction for ZIP and TAR archives (path normalization, rejection of absolute or up‑level paths, handling of symlinks/hardlinks, ensuring extraction stays inside the target, raises ValueError on unsafe paths), with added os and shutil imports. No public API signatures were removed or changed. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
SECURITY.md (5)
3-3: Normalize heading levels; keep a single H1.Subsections should be H2 under “Security Policy”.
-# Reporting a Vulnerability +## Reporting a Vulnerability -# Disclosure Policy +## Disclosure Policy -# Acknowledgements +## AcknowledgementsAlso applies to: 13-13, 17-17
8-11: Clarify reporting flow; add docs link and optional fallback channel.Link to GH docs; consider a fallback email (if maintained).
-* To report a security issue, please use the GitHub Security Advisories tab to "Open a draft security advisory". +* To report a security issue, please use the repository's Security tab to "Open a draft security advisory" (see: https://docs.github.com/en/code-security/security-advisories/repository-security-advisories/creating-a-repository-security-advisory). * Include a detailed description of the issue, steps to reproduce, potential impact, and any possible mitigations. * If applicable, please also attach proof-of-concept code or screenshots. * We will acknowledge your report within 72 hours and provide a status update as we investigate. +* If you cannot use GitHub, contact us via <security-contact@your-domain> (PGP preferred). Replace with the correct address.
13-16: Make coordinated disclosure explicit (timeline, CVE/OSV).State target fix/disclosure window and CVE/OSV process.
* We follow a coordinated disclosure approach. -* We will not publicly disclose vulnerabilities until a fix has been developed and released. +* We will not publicly disclose vulnerabilities until a fix has been developed and released. Our target window is [X] days from triage for critical issues (adjust as policy). +* We request a CVE ID via GitHub Security Advisories and publish an OSV entry upon release. * Credit will be given to researchers who responsibly disclose vulnerabilities, if requested.
1-2: Add Supported Versions and Scope.Helps reporters focus on maintained releases and first/third‑party boundaries.
# Security Policy +## Supported Versions +The following versions receive security fixes: + +| Version | Supported | +|--------:|:---------:| +| X.Y | Yes/No | + +## Scope +Report vulnerabilities affecting MONAI code and officially maintained extensions. Third‑party issues should be reported upstream (you may still notify us if MONAI is impacted).
3-16: Add Safe Harbor and testing constraints (healthcare context).Set expectations: no DoS, no data exfiltration, synthetic data only, etc.
## Reporting a Vulnerability @@ Please do not create public issues for security-related reports. @@ * We will acknowledge your report within 72 hours and provide a status update as we investigate. +## Testing Guidelines and Safe Harbor +Please: +* Avoid actions that degrade service (no DoS/stress testing against shared infra). +* Never access, exfiltrate, or use real patient/PHI data; use synthetic or sample data only. +* Do not attempt social engineering or physical intrusion. +If you follow this policy in good faith, MONAI will not initiate legal action solely for your testing activities.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (1)
SECURITY.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.md
⚙️ CodeRabbit configuration file
Remember that documentation must be updated with the latest information.
Files:
SECURITY.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: packaging
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (codeformat)
🔇 Additional comments (2)
SECURITY.md (2)
11-11: Confirm the 72‑hour SLA is realistic for maintainers.If not, adjust before publishing.
1-2: Handle GHSA access requests outside this doc.The PR’s ask to view GHSA-x6ww-pf9m-m73m, GHSA-6vm5-6jv9-rjpj, GHSA-p8cm-mm2v-gwjm should be processed via GitHub Security Advisories permissions, not encoded in SECURITY.md.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: h3rrr <[email protected]>
|
/build |
1 similar comment
|
/build |
There was a problem hiding this comment.
Thanks for the contribution. I have suggested a minor rewrite but broadly it's the same policy you suggest. I think this is fine as a policy, it follows mostly what's given in the example one from Github's documentation. I would like @Nic-Ma to weigh in however as well if possible.
Co-authored-by: Eric Kerfoot <[email protected]> Signed-off-by: h3rrr <[email protected]>
|
Hi @ericspod , I submitted three related vulnerabilities before. Can you please review them? |
I have now requested CVEs for your submissions, would you be able to submit PRs to fix each? I had planned to do so once we'd sorted a CICD issue. |
|
I can try to submit a PR to fix it, but it might take some time... |
If it's a lot of work we can consider only one vulnerability at a time, for example just a PR fixing the zip file one. Any contribution is appreciated, thanks! |
|
OK, I'll submit the corresponding fix, probably tomorrow |
I've encountered some issues that may be slowing down submissions. It's possible that my environment is causing test cases to repeatedly fail, even with unmodified code. |
|
Hi @h3rrr we do want to get the fixes for the PyTorch loading and unpickling issues merged soon so please get to the path traversal one when you can. I will next work on the PyTorch load and credit you with having raised the advisory. This PR can go ahead shortly with the requested changes. Thanks! |
Path traversal security issue fix Zip Slip Signed-off-by: h3rrr <[email protected]>
for more information, see https://pre-commit.ci
|
A fix has been submitted to address path traversal issues with zip and tar. @ericspod |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
monai/apps/utils.py (3)
123-131: Consider adding type hints and improving docstring.The function lacks type annotations and the docstring doesn't follow Google style.
-def safe_extract_member(member, extract_to): - """Securely verify compressed package member paths to prevent path traversal attacks""" +def safe_extract_member(member: Any, extract_to: PathLike) -> str: + """Securely verify compressed package member paths to prevent path traversal attacks. + + Args: + member: Archive member object from zipfile or tarfile. + extract_to: Target directory for extraction. + + Returns: + The safe, normalized full path for extraction. + + Raises: + ValueError: If the member path contains unsafe elements like '..' or absolute paths. + """
314-322: ZIP extraction may lose file permissions.The manual extraction doesn't preserve file permissions from the ZIP archive.
safe_path = safe_extract_member(member, output_dir) os.makedirs(os.path.dirname(safe_path), exist_ok=True) with zip_file.open(member) as source: with open(safe_path, 'wb') as target: shutil.copyfileobj(source, target) + # Preserve file permissions if available + if hasattr(member, 'external_attr'): + mode = member.external_attr >> 16 + if mode: + os.chmod(safe_path, mode)
333-335: Missing None check is redundant.The
if source:check at line 333 is unnecessary sinceextractfilereturns None for non-file members, but we already filter those at line 327.with tar_file.extractfile(member) as source: - if source: - with open(safe_path, 'wb') as target: - shutil.copyfileobj(source, target) + with open(safe_path, 'wb') as target: + shutil.copyfileobj(source, target)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (1)
monai/apps/utils.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/apps/utils.py
🪛 Ruff (0.12.2)
monai/apps/utils.py
136-136: Avoid specifying long messages outside the exception class
(TRY003)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: packaging
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: build-docs
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (pytype)
🔇 Additional comments (1)
monai/apps/utils.py (1)
123-147: Good security fix for path traversal vulnerability.The implementation correctly prevents directory traversal attacks in archive extraction.
Hi @h3rrr thanks for this, I think it looks correct though it'll need a test. What I suggest instead of submitting it here is to put the change into a new PR with tests that demonstrate a malicious zip file can be safely opened. In this PR we'll just add the SECURITY.md file but with the duplication I mentioned earlier removed. Does that work for you? What I would suggest doing is creating a new branch on your fork and then putting your changes to |
OK, I'll submit a new PR tomorrow. |
Signed-off-by: h3rrr <[email protected]>
Changed to previous content, the fix will be filed in a new PR Signed-off-by: h3rrr <[email protected]>
|
Hi @ericspod , I'm not sure if this is the correct solution. I've verified the fix locally and posted the results. PR:#8568 Sorry again for the frequent file changes caused by my unfamiliarity with GitHub's pull operations. I hope this is correct this time. |
No worries we can help with this. I'm going to approve this PR now and hopefully merge soon. What I think we should do for the next PR for the zip fix is have a test where you construct a zip file with the |
The path traversal issue has been fixed, and detection of soft links and hard links has been added to prevent bypass. Signed-off-by: h3rrr <[email protected]>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
monai/apps/utils.py (2)
331-343: TAR: tar.gz won’t extract with mode 'r'; also preserve directories.
- Use mode 'r:*' to autodetect compression (fixes .tar.gz).
- Create directories for member.isdir(); skip non-files explicitly.
- This aligns with earlier feedback on directories/special files.
- with tarfile.open(filepath, 'r') as tar_file: - for member in tar_file.getmembers(): - safe_path = safe_extract_member(member, output_dir) - if not member.isfile(): - continue - os.makedirs(os.path.dirname(safe_path), exist_ok=True) - source = tar_file.extractfile(member) - if source is not None: - with source: - with open(safe_path, 'wb') as target: - shutil.copyfileobj(source, target) + with tarfile.open(filepath, 'r:*') as tar_file: + for member in tar_file.getmembers(): + if member.isdir(): + safe_path = safe_extract_member(member, output_dir) + os.makedirs(safe_path, exist_ok=True) + continue + if not member.isfile(): + continue + safe_path = safe_extract_member(member, output_dir) + os.makedirs(os.path.dirname(safe_path), exist_ok=True) + source = tar_file.extractfile(member) + if source is not None: + with source: + with open(safe_path, 'wb') as target: + shutil.copyfileobj(source, target)
319-329: ZIP: don’t drop directory entries; create them.Empty directories are currently lost.
with zipfile.ZipFile(filepath, 'r') as zip_file: for member in zip_file.infolist(): safe_path = safe_extract_member(member, output_dir) - if member.is_dir(): - continue + if member.is_dir(): + os.makedirs(safe_path, exist_ok=True) + continue os.makedirs(os.path.dirname(safe_path), exist_ok=True) with zip_file.open(member) as source: with open(safe_path, 'wb') as target: shutil.copyfileobj(source, target)
🧹 Nitpick comments (3)
monai/apps/utils.py (3)
318-331: Make extension checks case-insensitive and avoid false positives.Use a lower-cased name and explicit “.zip”/“.tar[.gz|.tgz]” suffixes.
- if filepath.name.endswith("zip") or _file_type == "zip": + name_lower = filepath.name.lower() + if name_lower.endswith(".zip") or _file_type == "zip": with zipfile.ZipFile(filepath, 'r') as zip_file: ... return - if filepath.name.endswith("tar") or filepath.name.endswith("tar.gz") or "tar" in _file_type: + if name_lower.endswith(".tar") or name_lower.endswith(".tar.gz") or name_lower.endswith(".tgz") or "tar" in _file_type: with tarfile.open(filepath, 'r') as tar_file: ... return
319-343: Optional: guard against zip/tar bombs.Consider bounding per-member size and total extracted bytes, and reject archives exceeding limits.
I can add size limits and tests if you want.
274-344: Add tests for the new extraction behavior.
- ZIP: empty directory preserved; traversal blocked; ZIP symlink rejected/treated as regular file per policy.
- TAR/TAR.GZ: directories preserved; traversal blocked; link policy enforced.
I can draft unit tests that build small archives on the fly to verify these cases. Want me to open a PR with tests?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
monai/apps/utils.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/apps/utils.py
🪛 Ruff (0.12.2)
monai/apps/utils.py
134-134: Avoid specifying long messages outside the exception class
(TRY003)
136-136: Avoid specifying long messages outside the exception class
(TRY003)
141-141: Avoid specifying long messages outside the exception class
(TRY003)
150-150: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: packaging
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: build-docs
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: flake8-py3 (pytype)
🔇 Additional comments (2)
monai/apps/utils.py (2)
14-15: Imports look fine.Needed for path checks and streaming copies.
123-152: Decide: fail or skip on link members.Current behavior raises on any symlink/hardlink (tar) and will abort extraction if present. If you prefer “best effort,” consider skipping link members instead of raising. Confirm desired policy; I can adjust accordingly.
rollback Signed-off-by: h3rrr <[email protected]>
|
/build |
Help the project add a more detailed VDP (Vulnerability Disclosure Program) description, and request to view the three security vulnerabilities I submitted some time ago: GHSA-x6ww-pf9m-m73m, GHSA-6vm5-6jv9-rjpj, GHSA-p8cm-mm2v-gwjm