ECOPROJECT-4719 | fix: Prevent symlink-based path traversal in VDDK tarball extraction by AvielSegev · Pull Request #256 · kubev2v/assisted-migration-agent

AvielSegev · 2026-06-01T09:29:55Z

Summary

Fixes critical path traversal vulnerability in VDDK tarball extraction (CVE-level security issue)
Blocks chained symlink attacks that could write arbitrary files outside extraction directory
Preserves support for legitimate VDDK .so version symlinks

Security Impact

Severity: High - Unauthenticated LAN-adjacent arbitrary file write as UID 1001

Vulnerability Details

The PUT /inspector/vddk endpoint extracts gzip'd tarballs using extractTarGz(). The existing path traversal protection used lexical checks (filepath.Clean + HasPrefix) that don't resolve symlinks already on disk.

Attack scenario:

Attacker crafts tarball with symlink: a/x → .. (passes lexical validation)
Then writes: a/x/evil.sh which follows the live symlink
File actually lands at: dest/../evil.sh (outside destDir)

Real impact:

Write to /var/lib/agent/config.json (config manipulation)
Write to /app/.cache/malicious.py (persistent code execution)
Agent runs with vCenter admin credentials → full infrastructure compromise

Fix Implementation

Before creating files/directories, resolve parent directory symlinks with filepath.EvalSymlinks and verify the resolved path is still inside destDir. This prevents the attack while maintaining support for legitimate VDDK internal symlinks (e.g., libvixDiskLib.so → libvixDiskLib.so.8.0.3).

Summary by CodeRabbit

Bug Fixes
- Improved archive extraction validation to detect and reject symlink-based path-escape and path-traversal attempts (including absolute and relative targets, and traversals using ../), preventing files from being written outside the intended extraction root while still permitting legitimate internal symlinks that resolve within the extraction root.
Tests
- Added comprehensive tests validating chained symlink escapes, absolute/relative traversal cases, ../ usage, and acceptance of valid internal symlinks.

coderabbitai · 2026-06-01T09:30:03Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 240dec9d-8672-4bae-898b-657e2fd835fd

📥 Commits

Reviewing files that changed from the base of the PR and between ef99c9b and 08de5b2.

📒 Files selected for processing (2)

internal/services/vddk.go
internal/services/vddk_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

internal/services/vddk.go
internal/services/vddk_test.go

📝 Walkthrough

Walkthrough

This PR hardens tar.gz extraction security by adding symlink-escape detection to extractTarGz. Before creating tar entries, the function resolves symlinks in parent directories and rejects extraction if the resolved parent would escape the destination directory. Tests validate rejection of multiple traversal patterns and acceptance of legitimate internal symlinks.

Changes

Symlink Escape Detection

Layer / File(s)	Summary
Symlink escape detection in tar extraction `internal/services/vddk.go`	`extractTarGz` resolves symlinks in entry parent directories and returns explicit `symlink escape detected` errors when resolution would place the parent outside `destDir`, blocking creation before the existing switch handling.
Path traversal prevention test coverage `internal/services/vddk_test.go`	A new `Security: Path Traversal Prevention` block adds five test cases verifying `Upload` rejects chained symlinks, absolute targets, relative escapes, and `../../` traversal, and accepts legitimate internal VDDK symlinks while checking created link targets.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

tupyy
ronenav
nirarg

Poem

🐰 I hop through tarball threads with care,
I sniff each symlink, check each lair.
No secret path will let me stray,
Extraction stays the honest way.
Cheer the code — keep dangers bare.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main security fix: preventing symlink-based path traversal in VDDK tarball extraction, which aligns perfectly with the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with 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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

ronenav · 2026-06-03T12:00:55Z

/approve

…arball extraction Resolves CVE vulnerability where chained symlinks could write files outside the extraction directory. An attacker could craft a tarball with: 1. a/x → .. (symlink passes lexical validation) 2. a/x/evil.sh (follows symlink, writes to ../evil.sh outside destDir) This enables arbitrary file write as UID 1001, allowing: - Config file overwrites in /var/lib/agent/ - Persistent code execution via /app/.cache - Exfiltration of vCenter admin credentials Fix: Before creating files/directories, resolve parent path symlinks with filepath.EvalSymlinks and verify the resolved path remains inside destDir. Legitimate VDDK .so version symlinks are still supported. Signed-off-by: Aviel Segev <asegev@redhat.com>

amalimov · 2026-06-04T11:22:03Z

+		// to prevent chained symlink attacks (e.g., a/x -> .., then a/x/evil)
+		if header.Typeflag == tar.TypeDir || header.Typeflag == tar.TypeReg || header.Typeflag == tar.TypeSymlink {
+			parentDir := filepath.Dir(targetPath)
+			if parentDir != destDir {


do we need to clean these directories from slashes?

maybe with this: https://pkg.go.dev/path/filepath#Clean

If you scroll up a bit, you will see:

targetPath := filepath.Clean(filepath.Join(destDir, header.Name))
and parentDir := filepath.Dir(targetPath)

Regarding destDir, expected from the caller to pass clean path.

amalimov · 2026-06-04T11:26:42Z

/lgtm

AvielSegev · 2026-06-07T06:13:37Z

/lgtm

can you please approve the PR using the regular Approve pull request?

amalimov

lgtm

AvielSegev requested a review from a team as a code owner June 1, 2026 09:29

AvielSegev requested review from ronenav and tupyy and removed request for a team June 1, 2026 09:29

AvielSegev force-pushed the fix/ECOPROJECT-4719-vddk-symlink-path-traversal branch 3 times, most recently from f54d7dc to ef99c9b Compare June 3, 2026 09:29

AvielSegev force-pushed the fix/ECOPROJECT-4719-vddk-symlink-path-traversal branch from ef99c9b to 08de5b2 Compare June 4, 2026 06:58

amalimov reviewed Jun 4, 2026

View reviewed changes

AvielSegev enabled auto-merge (rebase) June 7, 2026 06:12

amalimov approved these changes Jun 7, 2026

View reviewed changes

AvielSegev merged commit bcae043 into kubev2v:main Jun 7, 2026
11 checks passed

coderabbitai Bot mentioned this pull request Jun 11, 2026

test: Fix macOS compatibility in VDDK path traversal security tests #269

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ECOPROJECT-4719 | fix: Prevent symlink-based path traversal in VDDK tarball extraction#256

ECOPROJECT-4719 | fix: Prevent symlink-based path traversal in VDDK tarball extraction#256
AvielSegev merged 1 commit into
kubev2v:mainfrom
AvielSegev:fix/ECOPROJECT-4719-vddk-symlink-path-traversal

AvielSegev commented Jun 1, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 1, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

Uh oh!

ronenav commented Jun 3, 2026

Uh oh!

amalimov Jun 4, 2026

Uh oh!

amalimov Jun 4, 2026

Uh oh!

AvielSegev Jun 4, 2026

Uh oh!

amalimov commented Jun 4, 2026

Uh oh!

AvielSegev commented Jun 7, 2026 •

edited

Loading

Uh oh!

amalimov left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

AvielSegev commented Jun 1, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Security Impact

Vulnerability Details

Fix Implementation

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

Uh oh!

ronenav commented Jun 3, 2026

Uh oh!

amalimov Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

amalimov Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

AvielSegev Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

amalimov commented Jun 4, 2026

Uh oh!

AvielSegev commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amalimov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AvielSegev commented Jun 1, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 1, 2026 •

edited

Loading

AvielSegev commented Jun 7, 2026 •

edited

Loading