Skip to content
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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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)

…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.

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.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@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

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.

1 participant