feat: cannot extract files outside the target directory #205

letFunny · 2025-02-14T14:18:52Z

Have you signed the CLA?

This commits adds several security controls and tests so that files
cannot be extracted outside of the target directory, and hardlinks
cannot point to files outside of the target directory.

This commits adds several security controls and tests so that files cannot be extracted outside of the target directory, and hardlinks cannot point to files outside of the target directory.

github-actions · 2025-02-14T14:22:02Z

Command	Mean [s]	Min [s]	Max [s]	Relative
`BASE`	8.235 ± 0.042	8.189	8.333	1.00
`HEAD`	8.551 ± 0.027	8.516	8.598	1.04 ± 0.01

rebornplusplus · 2025-03-18T15:18:51Z

internal/deb/extract_test.go

+			}},
+		},
+	},
+	error: `cannot extract from package "test-package": cannot create path /[a-z0-9\-\/]*/file outside of root /[a-z0-9\-\/]*`,


/[a-z0-9-/]*/file

What is this path? Is it to hide the root?

To be clear, this is not hiding the root. This is a regular expression that is used for matching. The actual message will hold the real paths.

rebornplusplus · 2025-03-18T15:20:03Z

internal/fsutil/create.go

+	// Root defaults to "/" if empty.
+	Root string


Won't that be a bit risky to have / as the default? What if Chisel overwrites the files in the system?

How about /var/lib/chisel or $PWD? The snap may not be able to write to the former.

This is the root of the filesystem, I am not sure we can provide a good default as users may choose whatever folder they wish. But, more importantly, I put the root as / because this is a general API and, in my opinion, the default shouldn't correspond to one use-case.

I agree having the default in an arbitrary place doesn't seem natural, but it does seem safer to force it to be defined. This way we'll avoid errors created by missing that separation of concerns, which actually could result in security bugs. All it takes is a Root: "/" to get through it when that's what's desired.

Done in da20063, now Root is required.

niemeyer

Very nicely done, thanks. Only minor tuning suggested:

niemeyer · 2025-03-31T09:55:17Z

internal/fsutil/create.go

+	// Root defaults to "/" if empty.
+	Root string


I agree having the default in an arbitrary place doesn't seem natural, but it does seem safer to force it to be defined. This way we'll avoid errors created by missing that separation of concerns, which actually could result in security bugs. All it takes is a Root: "/" to get through it when that's what's desired.

niemeyer · 2025-03-31T09:58:21Z

internal/fsutil/create.go

+	if o.Root != "/" {
+		o.Root = filepath.Clean(o.Root) + "/"
+	}
+	o.Path = filepath.Clean(filepath.Join(o.Root, o.Path))


This is unexpected as it means we're mutating data to be able to use the feature at all. Worse: if we happen to "clean" this same options object twice we'll get a corrupt value. I suggest having a method like abspath() (string, error) which returns the absolute path after doing the required verifications.

Agree that a clean function that avoids mutation and can be called twice is preferred. If I understood your suggestion correctly I have done it as part of da20063.

letFunny · 2025-03-31T10:52:34Z

internal/scripts/scripts.go

 	// No mode parameter for now as slices are supposed to list files
 	// explicitly instead.
 	entry, err := fsutil.Create(&fsutil.CreateOptions{
+		Root: "/",


[Comment for reviewer]: There is some complexity in how we handle paths above for Starlark. The function gets the fpath and follows symlinks among other things, I am hesitant to change it without a more comprehensive refactor which I don't think should happen in this PR.

internal/fsutil/create.go

niemeyer · 2025-03-31T13:54:15Z

internal/fsutil/create.go

+	if err != nil {
+		return nil, err
+	}
+	o.Path = path


This is still doing the same thing that was raised in the prior review: mutating the options in place, and changing the meaning of Root and Path. If we change what Path points to so that it includes Root, and we pass this same Options to this same function again, we'll then have the Root joined twice on the same path.

The suggestion was to use an actual abspath() method that would do that, and return the final path after error checking. If that's bad or inconvenient for the actual use cases, let's please talk about it.

Using the method instead of changing the options means we have to push abspath and the validation to each leaf function individually (create file, create symlink, create directory, etc.) I will do the changes and see if there is redundancy. The biggest sticking point for me is about moving the logic in the other comment, i.e. sanitize the root and add the trailing /, to each function but I understand that is not what we want. We just want to push the path function and we can leave the validation outside.

Discussed offline, this is about changing the meaning of Root and Path. I have incorporated changes in 8046cf5 so that Root and Path are respected and the helper is used absPath, this also makes the transformations idempotent.

niemeyer · 2025-03-31T13:54:44Z

internal/fsutil/create.go

+		return nil, nil, fmt.Errorf("root cannot be empty")
+	}
+	if o.Root != "/" {
+		o.Root = filepath.Clean(o.Root) + "/"


And this is repeating the same logic.

niemeyer

Thanks, looking good now!

letFunny added 3 commits February 3, 2025 10:23

feat: cannot extract files outside the target directory

b214b76

This commits adds several security controls and tests so that files cannot be extracted outside of the target directory, and hardlinks cannot point to files outside of the target directory.

Merge branch 'main' into security-check-root-creating-files

ddd53d9

unify error messages

a560fb5

letFunny added 3 commits February 25, 2025 17:02

fix prefix logic

8ce2947

clean typo

bfaa9eb

helper function

44c5fd6

rebornplusplus reviewed Mar 18, 2025

View reviewed changes

niemeyer requested changes Mar 31, 2025

View reviewed changes

make Root mandatory

da20063

letFunny commented Mar 31, 2025

View reviewed changes

niemeyer requested changes Mar 31, 2025

View reviewed changes

letFunny added 2 commits March 31, 2025 16:12

change error message

65d820b

do not change the path and getValidOptions

8046cf5

niemeyer approved these changes Mar 31, 2025

View reviewed changes

niemeyer merged commit 9ac6134 into canonical:main Mar 31, 2025
18 checks passed

letFunny deleted the security-check-root-creating-files branch April 30, 2025 08:07

feat: cannot extract files outside the target directory #205

feat: cannot extract files outside the target directory #205

Uh oh!

Conversation

letFunny commented Feb 14, 2025

Uh oh!

github-actions bot commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Feb 14, 2025 •

edited

Loading