#3202 Issue open: What to improve in ReaR project as lesson learned from XZ Attack?¶
Labels: discuss / RFC, ReaR Project
jsmeix opened issue at 2024-04-08 12:39:¶
For background information about the XZ Attack see
https://en.wikipedia.org/wiki/XZ_Utils_backdoor
and follow the links therein, in particular see
https://tukaani.org/xz-backdoor/
and
https://gist.github.com/thesamesam/223949d5a074ebc3dce9ee78baad9e27
jsmeix commented at 2024-04-08 12:54:¶
I find in particular this one interesting:
https://mastodon.social/@AndresFreundTec/112208281684931915
From my point of view the most important outcome in that discussion is
this
posting (excerpt):
Elias Probst @eliasp
@jejb so it all comes down to being able
to actually understand and review every contribution,
so it doesn't really matter in the end who contributed,
but what was contributed instead.
I.e. take out the "who" part from finding a solution
how to avoid malicious code changes and
focus on the code changes itself.
Another posting in that discussion also indicates
that the "who" part likely does not help
to avoid malicious code changes (excerpt):
Christoph Petrausch @hikhvar@norden.social
@eliasp @AndresFreundTec But even with the web of trust.
How should that scale in a global way and also
not exclude less privileged people?
A student in a remote country side
wants to start contributing to code.
Welp, nobody there to establish web of trust.
I.e. trying to solve the "who" part via a "web of trust"
does not work in practice.
In contrast understanding what was contributed
(regardless who contributed it)
should work in practice.
From my personal point of view this means:
New code that is not easy to understand should be
rejected at upstream by those upstream maintainers
who review contributions from others
regardless if some other one is an unknown newcomer
or a "well known expert in sophisticated coding ingenuity".
Old code that is known to be OK since ages can stay as is.
But when old code is changed the changes must be easy to understand
(which may mean that bigger parts of old code must be overhauled).
Simply put:
Code that is hard for others to understand must be rejected
because it would be unmaintainable by others in the future.
So in the end from my personal point of view
it means to more enforce what is meant with
https://github.com/rear/rear/wiki/Coding-Style
i.e. not some nitpicking coding syntax style "rules"
(which cause more annoyance than being actually helpful)
but to more enforce the overall idea behind:
Make yourself understood
to make your code maintainable which means
at any time later others still understand your code
so they can properly fix and enhance your code as needed.
gdha commented at 2024-04-08 12:59:¶
An important aspect is to meet the active contributors once personally. A "web of trust" should be a "people of trust" (in our case). All ReaR contributors have met each other in person - that helps.
jsmeix commented at 2024-04-08 13:08:¶
In addition to enforce that code must be easy to understand
I think we should also enforce to review all code changes.
This means that all code changes
(also code changes by us, the upstream maintainers)
must be approved by at least one other upstream maintainer
even if that makes merging code changes into 'master' slower.
But I think that a certain kind of slowness is helpful
because it results from "verify" in "Trust, but verify"
https://en.wikipedia.org/wiki/Trust,_but_verify
In practice enforced code change reviews means
that all code changes must happen via a pull request.
I mean only actual code changes (at least for now).
I.e. all what changes what ReaR does on the user's system.
E.g. typo fixes in comments can still be directly merged
regardless that even a "typo fix" in a comment may
(unintentionally or by evil intent) be a false change
of the meaning of a comment so others get a false
understanding what (some weird looking) code
actually does which could be a first step to do
later a malicious change in the actual code.
jsmeix commented at 2024-04-08 13:13:¶
@gdha
yes, that all ReaR contributors have met each other in person
does help very much.
But:
This would exclude people in far away countries.
Currently I have no good idea how to solve that properly.
And it does not help when the account
of a ReaR upstream maintainer got hijacked
or when a ReaR upstream maintainer was tricked
by a malicious external person.
I think enforced code change reviews
by at least one other upstream maintainer help.
And at least two active upstream maintainers help
against social engineering attrition of single
upstream maintainers (as it happened to the
single upstream maintainer during the XZ Attack).
gdha commented at 2024-04-09 06:34:¶
As mentioned already mentioned before all Pull Requests must be reviewed by another person then the author, and I would like to add an additional third person - the co-called "approver" of the PR. This way there always 3 persons involved before a PR is getting merged.
schlomo commented at 2024-04-09 06:50:¶
We can enable branch protection to help us "remember" the PR rule, although maintainers of course can override that.
I'm against a 6-eyes approval, at least now, because I think that going from "just do it" to "everything via PR" is already a big and probably sometimes inconvenient step for us. As we don't have any actual problem, I'd like to take this slowly, step by step. And inspect and adapt after every change.
jsmeix commented at 2024-04-09 08:26:¶
I also think that 6-eyes approval is too much for us for now.
Often I don't get a single approval for my pull requests and
when I get one I think it is often not after a thorough review.
I totally understand this because in most cases I don't do
a thorough review but only have a look if I spot something
and if not I just trust you.
I always prefer to change things slowly, step by step.
In think for now it is enough to have some kind of "enforced"
approval by at least one other upstream maintainer
for pull requests from an upstream maintainer.
Perhaps we should "enforce" approval by at least
two upstream maintainers for pull requests
from external contributors?
With "enforce" I mean that we set up something in GitHub
which we can of course override but that would at least
remind us each time to not "just merge" something without
approval by at least one other upstream maintainer.
FWIW:
I remember well how @pcahyna asked me
again and again questions during
https://github.com/rear/rear/pull/2961
because he did not understand my description in default.conf.
I even felt somewhat annoyed by his insisting questions
("why the heck doesn't he understand my nice description?")
until finally I saw it that what I described
did not match what I had implemented, cf.
https://github.com/rear/rear/pull/2961#discussion_r1363679930
I was blind regarding my own faulty description
(from 09. Oct. 2023 until 18. Oct. 2023).
I still cannot understand why I was so blind.
So @pcahyna helped me to finally see my fault and
he helped our users to not get my faulty description.
gdha commented at 2024-04-09 08:32:¶
Perhaps, the biggest danger is not in the source code itself, but in the produced ISO images stored on a NAS location where everybody (within a company) can replace it with a malicious code (or whatever evil).
schlomo commented at 2024-04-09 08:39:¶
Yes, that is however by design and we shouldn't worry about it.
Maybe Secure Boot with custom keys can be used to create end-end secured DR boot media
jsmeix commented at 2024-04-09 09:42:¶
With this issue here I meant only our source code,
in particular how to better ensure that our users
actually get what we intended to provide them.
Regarding secure ReaR recovery system images:
I don't remember there had been a user request in the past
for a secured ReaR boot medium so at least currently
this seems to be not an issue for our users.
I assume our users know how to keep e.g. ReaR ISOs secure.
I assume they handle ReaR boot media same as their backups.
One does not want to restore a maliciously modified backup
(e.g. where /usr/sbin/sshd got "additional functionality").
jsmeix commented at 2024-04-12 13:43:¶
FYI:
an openSUSE report about the XZ Backdoor issue:
https://news.opensuse.org/2024/04/12/learn-from-the-xz-backdoor/
[Export of Github issue for rear/rear.]