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

RFE: ensure unwritable buildroot during %check #3010

Closed
pmatilai opened this issue Apr 2, 2024 · 11 comments
Closed

RFE: ensure unwritable buildroot during %check #3010

pmatilai opened this issue Apr 2, 2024 · 11 comments
Labels

Comments

@pmatilai
Copy link
Member

pmatilai commented Apr 2, 2024

On the heels of the xz incident, one of the ideas (from @keszybz it seems) to harden against malicious tests is to make buildroot readonly during %check. Picked from #3009 as a clear actionable item.

@pmatilai pmatilai added the RFE label Apr 2, 2024
@pmatilai pmatilai added this to RPM Apr 2, 2024
@github-project-automation github-project-automation bot moved this to Backlog in RPM Apr 2, 2024
@pmatilai pmatilai moved this from Backlog to Todo in RPM Apr 2, 2024
@mlschroe
Copy link
Contributor

mlschroe commented Apr 2, 2024

There's not much you can do against a malicious upstream.

@keszybz
Copy link
Contributor

keszybz commented Apr 2, 2024

An idea was floated on fedora-devel to remove tests from packages altogether. I empathetically disagree with that, but maybe it'd be useful to "sandbox" the tests a bit. The test code is often of lesser quality and less reviewed.

The basic idea is to make sure that the %check section shouldn't affect the packaged payload. If any work affecting the installed filed is done in %check, this is most likely an error. In particular, the %check section can be skipped, and this shouldn't cause the results to be different.

Either $RPM_BUILD_ROOT could be remounted read-only for the duration of %check, or overlayfs with a throw-away upper layer could be overmounted.

In the fedora-devel thread I suggested that mock should do this, because I wasn't aware of rpm using mount namespaces at all.

All that said, I'm not really sure if it's such a great idea… It might help in some cases, so if it's easy to implement, maybe it'd be worth trying. But if you think it's just not worth the effort and close the issue, I'm fine with that too.

@pmatilai
Copy link
Member Author

pmatilai commented Apr 2, 2024

I opened this ticket from the discussion specifically because it's such a no-brainer when you see it: "tests should not be able to affect built binaries". How feasible it is in practise is another story, but it's worth at least investigating.

@xsuchy

This comment was marked as off-topic.

@pmatilai

This comment was marked as off-topic.

@mikhirev
Copy link

mikhirev commented Apr 17, 2024

This issue shows misunderstanding of how the xz backdoor was intended to work. Although its payload was hidden in the test files, it was extracted when running the ./configure script that happens at the %build stage. If you run tests on read only filesystem or even disable them, that wouldn't prevent the backdoor from being injected. So it is incorrect to speak about malicious tests, all tests worked correctly and did what they were expected to do.

Of course, we can imagine a situation when some backdoor is injected when running tests, and proposed changes could prevent such a situation. However this wouldn't help against the particular backdoor and it definitely won't help against hypothetical future backdoors targeted at rpm systems because their developers will take into account protections you are going to implement. I think, it only makes sense to implement them if it is easy to do and won't cause additional overhead by copying or hashing numerous files that can be very large in some cases.

@nmanthey
Copy link

I understand the difference between %build and %check, as well as the problem of this could be worked around by future actors. I would still like to understand the potential as a building blocks for hardening.

Do you see a path for a hashing-like validation in the %check phase that could be enabled by an additional run time parameter of the tool? This way, feature is available to potential users, but not enabled by default?

@mikhirev
Copy link

mikhirev commented Apr 18, 2024

There are simpler ways to ensure that %check stage does not affect files in the build directory. E.g. we could use an overlayed filesystem (overlayfs, aufs etc.) to mount an empty directory on top of the build directory before executing %check but use the original build directory for %install. This would have much lower overhead than hashing, but would be unportable between different OSes and add new dependencies. And I still think this does not solve the real issue because altering binaries would remain possible at %build and %install stages. A completely different approach is required to avoid this.

@nmanthey
Copy link

Yes, this approach will never be complete. Something like the proposed feature is only a building block. For the other stages, there could also be the requirement to not modify files that have been available already. IMHO, other attack vectors should be addressed with other tools.

What data would you need to be more willing to accept a PR the implements the requested idea? While the hashing approach might be more IO heavy, it seems like a portable solution. Furthermore, this approach does not require extra permissions for additional jailing.

@pmatilai
Copy link
Member Author

This is not about "preventing XZ", it's just somewhat inspired by it.
I really don't know why multiple people are arguing against rpm looking to do some extra packaging hygiene enforcement here. In a similar vein, rpm would prefer an unwritable build directory during %install.

Hashing the content is not something this ticket is about.

@pmatilai
Copy link
Member Author

Closing in favor of a more generic #3050

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

No branches or pull requests

6 participants