Fix relative symlink support in TarFile

[am11]

Fixes #77303

Depends on dotnet/runtime-assets#280

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #77303

Depends on dotnet/runtime-assets#280

Author:	am11
Assignees:	-
Labels:	area-System.IO
Milestone:	-

[am11]

@tmds, this gets us closer to tar utilities.

File attributes: I noticed with GNU and BSD tar(1), the file layer.tar in extraction dir has these permissions: rwxrwxrwx and date is of unix epoch's (Jan 1 1970), while we assign rwxr-xr-x and current date.

[tmds]

If we're changing this code, I think it would be good to make a decision about #74140.
I think the behavior is restrictive without a good reason.

[tmds]

File attributes: I noticed with GNU and BSD tar(1), the file layer.tar in extraction dir has these permissions: rwxrwxrwx and date is of unix epoch's (Jan 1 1970), while we assign rwxr-xr-x and current date.

Can you create an issue about this?

[am11]

I think the behavior is restrictive without a good reason.

There are two things which throw different IOExceptions:

• file being extracted has destination outside the extraction directory. Throws: IOException(TarExtractingResultsFileOutside)
• symlink is pointing to something outside the extraction directory (e.g. /usr/bin/foo). Throws: IOException(TarExtractingResultsLinkOutside)

the security concerns mentioned in #74140 apply to the first one, but the second one seems harmless given the target of symlink are normally not validated upon symlink creation (it can point to a non-existing path).

[tmds]

the security concerns mentioned in #74140 apply to the first one, but the second one seems harmless given the target of symlink are normally not validated upon symlink creation (it can point to a non-existing path).

Yes, #74140 is about the behavior being too restrictive in the second case.

@jozkee @carlossanlop @jeffhandley @adamsitnik can you make a decision about the desired behavior?

[am11]

@adamsitnik, currently, System.Formats.Tar allows symlink target to be inside the archive and disallows if the target is pointing to path outside the archive. So far, this PR hasn't changed the behavior. The bug appears when the symlink target is pointing to the path inside the archive with relative paths, the symlink validation (TarExtractingResultsLinkOutside) fails. In addition to fixing the bug, this PR is also restoring the original symlink path post-validation, so we don't turn a relative target into absolute one in symlink's target after extracting the archive (matches GNU and BSD tar utilities).

Allowing the target of symlink to path outside the archive is a separate discussion (#74140, awaiting decision) and it can be fixed in a separate PR.

[tmds]

Allowing the target of symlink to path outside the archive is a separate discussion (#74140, awaiting decision) and it can be fixed in a separate PR.

Rather than changing this code once more, review it once more, and maybe backport it once more too, why don't we make a decision?

[am11]

Hmm, because they are separate issues: #77303 (comment)..

[tmds]

Yes, and still very related. #77303 is a bug in the code written for #74140.

[am11]

Right, and this is a bug fix with accompanying tests. If decision to allow link target outside the extraction directory will take longer (#74140 is awaiting decision for some time now), we should fix this bug regardless. The code change for 74140 is straightforward, so I am fine to fix it here as well. Lets see what the area owners think about next steps.

[am11]

FWIW, it will take this change to allow target of symlinks (and hardlinks) outside the archive: am11@fae7cd2 (remove validation of link target, update two existing tests and add two additional tests for non-existing paths).

[tmds]

FWIW, it will take this change to allow target of symlinks (and hardlinks) outside the archive: am11@fae7cd2 (remove validation of link target, update two existing tests and add two additional tests for non-existing paths).

You must also add the symlink check (see #74140) to prevent 'using' a symlink to place files outside the output directory.

[am11]

You must also add the symlink check (see #74140) to prevent 'using' a symlink to place files outside the output directory.

I am not sure how to produce that tar file. Do you have a test case handy which isn't failing with my change, while it should?

[tmds]

This is about introducing a new check that prevents an 'evil' tar file to write files outside the output directory. You should be able to create the tar file using the .NET API.

For example:

link: SymbolicLink to /tmp
link/file: RegularFile

Extracting this without the check, would lead to writing to /tmp/file independent of the output directory.

[am11]

You should be able to create the tar file using the .NET API.

How exactly?

[tmds]

You can use the TarWriter API. Take a look at the tests for some inspiration.

[am11]

Have you actually found that kind of tar file before? I am not sure how do you think it is possible to construct that tar file using the .NET API (or any other producer for that matter). That's why I asked for the test repro (or the tar file) you are talking about.

[tmds]

This is a specially crafted file meant to trigger a security problem. You can't create it from file system. The TarWriter API allows you to create it because it will write whatever entries you ask it to.

[am11]

Ok. Since you have ideas what needs to be done for #74140, I am not pursuing it. As it stands, this PR is only fixing the bug reported in #77303.

@jozkee, can you take a look?

[jozkee]

Rather than changing this code once more, review it once more, and maybe backport it once more too, why don't we make a decision?

Between #77303 and #74140, is one of the issues worth backporting while the other is not?

Without the answer I think keeping them as separate PRs would allow us to nicely cherry-pick in case we choose to backport just one.

[am11]

is one of the issues worth backporting while the other is not?

IMO, this one is worth backporting since we currently allow symlinks within the extraction directory. The validation logic is wrong when the symlink is created with relative path; the bug this PR is fixing. It is not uncommon, e.g. in https://github.com/libunwind/libunwind/archive/refs/tags/v1.6.2.tar.gz, README is a relative symlink to README.md.

I think keeping them as separate PRs would allow us to nicely cherry-pick

I agree. Keeping bugfixes separate from changing the (intentional) behavior is pretty normal; if not recommended.

[jozkee]

Thanks.

[am11]

Thanks for the reviews @tmds and @jozkee.

[jozkee]

/backport to release/7.0

[github-actions]

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3480654764