fix: update all CDI hooks to fail for invalid container root paths

[cdesiniotis]

No description provided.

[cdesiniotis]

/cherry-pick release-1.20

[coveralls]

Coverage Report for CI Build 33128543976

Coverage decreased (-0.05%) to 43.585%

Details

• Coverage decreased (-0.05%) from the base build.
• Patch coverage: 18 uncovered changes across 6 files (0 of 18 lines covered, 0.0%).
• 1 coverage regression across 1 file.

Uncovered Changes

File	Changed	Covered	%
cmd/nvidia-cdi-hook/update-ldcache/update-ldcache.go	4	0	0.0%
cmd/nvidia-cdi-hook/create-symlinks/create-symlinks.go	3	0	0.0%
cmd/nvidia-cdi-hook/cudacompat/cudacompat.go	3	0	0.0%
cmd/nvidia-cdi-hook/disable-device-node-modification/disable-device-node-modification.go	3	0	0.0%
cmd/nvidia-cdi-hook/update-application-profile/update-application-profile.go	3	0	0.0%
cmd/nvidia-cdi-hook/chmod/chmod.go	2	0	0.0%

Coverage Regressions

1 previously-covered line in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
cmd/nvidia-cdi-hook/chmod/chmod.go	1	0.0%

---

Coverage Stats

Relevant Lines:	13445
Covered Lines:	5860
Line Coverage:	43.58%
Coverage Strength:	0.44 hits per line

---

💛 - Coveralls

[henry118]

What about the following?

1. "//", "/.", "/foo/.."
2. /container/root -> / (link)
3. mount --bind / /container/root (bind mount)

also runc validates the rootfs too, but it does not fail on root though.
https://github.com/opencontainers/runc/blob/cb470207c6f48904e11418a8fef3c37aeb7e482e/libcontainer/configs/validate/validator.go#L55-L72

[cdesiniotis]

What about the following?

1. "//", "/.", "/foo/.."
2. /container/root -> / (link)
3. mount --bind / /container/root (bind mount)

also runc validates the rootfs too, but it does not fail on root though. https://github.com/opencontainers/runc/blob/cb470207c6f48904e11418a8fef3c37aeb7e482e/libcontainer/configs/validate/validator.go#L55-L72

Fair point. If runc does not fail on / then one could make the argument we shouldn't either. If anything, we should update

nvidia-container-toolkit/internal/oci/state.go

Lines 82 to 95 in 7df27d0

	func (s *State) GetContainerRoot() (string, error) {
	containerRoot, err := s.getRoot()
	if err != nil {
	return "", err
	}
	if filepath.IsAbs(containerRoot) {
	return containerRoot, nil
	}
	return filepath.Join(s.Bundle, containerRoot), nil
	}
	// loadMinimalSpec loads a reduced OCI spec associated with the container state.

to check for symlinks (like runc does in https://github.com/opencontainers/runc/blob/cb470207c6f48904e11418a8fef3c37aeb7e482e/libcontainer/configs/validate/validator.go#L55-L72. WDYT?

The "" case we already handle when checking for an absolute vs relative path.

[henry118]

runc performs the validation when a container is created, before invoking oci hooks.

but i am not against the extra validation either, esp if we don't want to depend on runtime's implementation details.

[henry118]

If runc does not fail on / then one could make the argument we shouldn't either.

Agreed

[cdesiniotis]

Closing as no longer planned. May open a follow up based on the discussion here.