Skip to content

pkg/archive: nosysFileInfo: implement tar.FileInfoNames to prevent lookups #49152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

thaJeztah
Copy link
Member

relates to:

commit e9bbc41 removed our fork of pkg/archive that was in place to mitigate CVE-2019-14271. As part of that change, a nosysFileInfo type was added to prevent tar.FileInfoHeader from looking up user- and group-names.

A proposal was pending in go https://go.dev/issue/50102 to define an interface for implementing custom lookup functions to be implemented, and disable go's builtin lookup. That proposal was accepted, and is now implemented in go1.23.

Thia patch makes the nosysFileInfo implement the tar.FileInfoNames interface to prevent tar.FileInfoHeader from performing its own lookups. While the mitigation implemented in e9bbc41 should already prevent this from happening, implementing the interface does not cost us much and is complementary to the existing mitigation.

With this patch in place, we can consider removing the mitigation added in a316b10, which was discussed to be ineffective, but left in place for the time being.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Dec 20, 2024
@thaJeztah thaJeztah added this to the 28.0.0 milestone Dec 20, 2024
@thaJeztah thaJeztah self-assigned this Dec 20, 2024
@thaJeztah
Copy link
Member Author

I'm now considering if this part is accurate;

With this patch in place, we can consider removing the mitigation added in a316b10, which was discussed to be ineffective, but left in place for the time being.

Reading back some comments from @corhere in #42402 (comment) and some following that, there were still concerns about running go code inside the ("hostile") chrooted environment. The user-lookups addressed here (and patches before this) were the known cases where libraries could be loaded, but may not be comprehensive. So perhaps the init still has some value?

func init() {
// initialize nss libraries in Glibc so that the dynamic libraries are loaded in the host
// environment not in the chroot from untrusted files.
_, _ = user.Lookup("docker")
_, _ = net.LookupHost("localhost")
}

I recall from some discussion though that there was doubt whether that init was actually effective, but I'm trying really hard to recall the details around that (maybe @corhere recalls); if not, it may be good to remove it to not give a false sense of additional security provided

…okups

commit e9bbc41 removed our fork of
pkg/archive that was in place to mitigate CVE-2019-14271. As part of that
change, a nosysFileInfo type was added to prevent tar.FileInfoHeader from
looking up user- and group-names.

A proposal was pending in go https://go.dev/issue/50102 to define an
interface for implementing custom lookup functions to be implemented,
and disable go's builtin lookup. That proposal was accepted, and is now
implemented in go1.23.

Thia patch makes the nosysFileInfo implement the tar.FileInfoNames interface
to prevent tar.FileInfoHeader from performing its own lookups. While the
mitigation implemented in e9bbc41 should
already prevent this from happening, implementing the interface does not
cost us much and is complementary to the existing mitigation.

This patch keeps the mitigation added in a316b10
in place for any unforeseen other code.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the pkg_archive_nolookups branch from b15a72c to 2b4db93 Compare January 10, 2025 15:16
@thaJeztah
Copy link
Member Author

For posterity; we discussed this PR in the maintainers call, and decided to keep the existing mitigations in place; while some bits are possibly redundant, defense in depth won't hurt, and they don't bring real maintenance cost.

I'll bring this patch in, thanks Cory!

@thaJeztah thaJeztah merged commit d34a5f5 into moby:master Jan 10, 2025
140 checks passed
@thaJeztah thaJeztah deleted the pkg_archive_nolookups branch January 10, 2025 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/refactor PR's that refactor, or clean-up code status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants