Skip to content

fix: validate symlinks stay within source root during model packaging - #6545

Open
aviruthen wants to merge 1 commit into
aws:masterfrom
aviruthen:fix-packaging-symlink-validation
Open

aviruthen wants to merge 1 commit into
aws:masterfrom
aviruthen:fix-packaging-symlink-validation

Conversation

@aviruthen

Copy link
Copy Markdown
Collaborator

Summary

When ModelBuilder.build() packages local code, repack_model copies source_dir, dependencies, and the inference script into the model archive, and tar_and_upload_dir archives them directly. Both follow nested symlinks, so a symlink inside one of these inputs that points elsewhere on the file system gets packaged as a regular file. This change checks that nested symlinks resolve inside the input they belong to before packaging. It also tightens the path checks those inputs already go through.

Changes

sagemaker-core/src/sagemaker/core/common_utils.py

  • Add _validate_symlinks_within_root(), which walks a directory and raises ValueError if a nested file or directory symlink resolves outside it. Symlinks that resolve inside the directory, relative or absolute, are still supported.
  • _create_or_update_code_dir runs this check on source_directory and on each dependency before copying, and now applies the existing path validation to inference_script.
  • Path validation for source_directory, dependencies, the inference script, and code_dir:
    • compares against both the configured and the symlink-resolved form of each restricted path (for example, /etc is /private/etc on macOS)
    • matches whole path components with os.path.commonpath instead of a string prefix
    • adds /proc, /sys, and /dev to the restricted paths

sagemaker-core/src/sagemaker/core/fw_utils.py

  • tar_and_upload_dir runs the same symlink check on directory (or script when no directory is given) and on each dependency before creating the archive.

create_tar_file is unchanged. It is shared with model artifact uploads, where symlinks outside the source root are expected (for example, Hugging Face cache snapshots that link into ../../blobs).

Behavior change

A symlink under source_dir or a dependency directory that resolves outside that directory now raises ValueError during packaging. To fix this, copy the target into the directory, or point source_dir at a parent directory that contains both. S3 source_dir inputs are not affected.

Testing

  • New unit tests in sagemaker-core/tests/unit/test_common_utils.py cover:
    • nested file and directory symlinks inside and outside the root, relative symlinks, symlinked roots, and sibling directories sharing a name prefix
    • source_dir, dependency directories, and inference_script in _create_or_update_code_dir
    • end-to-end repack_model with local file:// URIs
    • restricted-path matching, including symlinked entries, /etc after resolution, /proc, /sys, /dev, and component-level matching
    • create_tar_file still following symlinks for model uploads
  • New tar_and_upload_dir tests in sagemaker-core/tests/unit/test_fw_utils.py cover rejection for source_dir, dependencies, and directory entry points, and packaging of in-root symlinks.
  • test_common_utils.py and test_fw_utils.py pass, as do sagemaker-serve test_model_builder_build.py and the sagemaker-mlops workflow repack/model-step tests.
  • black, flake8, and pylint pass on the changed files.

@aviruthen
aviruthen deployed to auto-approve October 28, 2026 23:05 — with GitHub Actions Active
@aviruthen
aviruthen deployed to auto-approve October 28, 2026 23:05 — with GitHub Actions Active
@aviruthen
aviruthen deployed to auto-approve October 28, 2026 23:05 — with GitHub Actions Active

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 AI code review

This PR hardens local-code packaging in ModelBuilder.build()/repack_model/tar_and_upload_dir by rejecting nested symlinks that resolve outside the input directory, and tightens the existing sensitive-path denylist (whole-component matching via commonpath, symlink-resolved denylist forms, adding /proc, /sys, /dev). create_tar_file is intentionally left unchanged for model-artifact uploads. The change is well-aligned with prior packaging-side symlink hardening (Source: #5906) and comes with thorough unit tests covering in-root/out-of-root, relative, and symlinked-root cases. I found no correctness, security, or test-coverage issues; the implementation and tests look solid.

Reviewed commit 9c65e35. Automated review; verify before acting.

This branch was successfully deployed

1 active deployment
auto-approve — 9c65e35d Deployed Oct 28, 2026 by aviruthen via wait-for-approval #1863
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.

1 participant