#3293 Issue closed: BACKUP=DUPLICITY may source third party code¶
Labels: enhancement, bug, cleanup, fixed / solved / done,
critical / security / legal
jsmeix opened issue at 2024-07-23 09:46:¶
See
https://github.com/rear/rear/issues/3285#issuecomment-2244545130
The current code related to the find_duply_profile function
and the DUPLY_PROFILE* variables looks rather messy.
At least at first glance I cannot make sense of it.
@rear/contributors
it should be verified before the ReaR 2.8 release
if third party code could be sourced here
or if all is reasonably safe.
gdha commented at 2024-08-13 09:08:¶
Details can be found at
https://www.thomas-krenn.com/en/wiki/Backup_on_Linux_with_duply#conf_File
Duply is a wrapper around duplicity.
It is up to the end-user to enter his/her password in the configuration
file of duply. We advise never to write passwords in plain text
files, but rather use secure key pairs.
github-actions commented at 2024-11-12 02:33:¶
Stale issue message
jsmeix commented at 2024-11-13 07:52:¶
In
https://github.com/rear/rear/issues/3285#issuecomment-2244545130
I wrote
I won't try to mitigate that now
based on guesses about a software
that I neither have nor know about
i.e. DUPLICITY with BACKUP=DUPLICITY.
So I won't make a decision here whether or not
BACKUP=DUPLICITY may source third party code
because I won't reverse engineer code
where I never had something to do with and
where I neither have nor know about the software
that is used by that code.
When we have obscure code in ReaR that deals with
third-party (backup) software which we do not have
and/or where we do not know about,
i.e. when we have code in ReaR where we do not or cannot
understand with reasonable effort what it actually does,
i.e. when we have code in ReaR which we cannot
maintain with reasonable effort,
then
I would rather remove that code to keep ReaR maintainable
instead of keeping unmaintainable (old) code in ReaR,
in particular when such code could be a security issue.
In this case it would mean to deprecate BACKUP=DUPLICITY
(unless someone can and does maintain its code in ReaR).
schlomo commented at 2024-11-13 09:10:¶
Maybe we should introduce a list of backup methods with maintainers and others who "care" about it, or are knowledgable. Based on that we can then also declare a maturity for every backup method, or a level of support and commitment to it.
I'd not deprecate and remove it just because it sources something, as that problem affects only user of Duplicity and not everybody. I'd remove code where we have no way to know if it works and where we can't find anybody to care about it.
For that "find somebody to care" we could add a shoutout for help to
BACKUP=DUPLICITY to give users of that a chance to come to us and
help. ATM that seems the only way to contact our users (and yes, this is
almost like a useless warning, but maybe OK in this case)
loyeyoung commented at 2024-11-13 23:36:¶
I do care about BACKUP=DUPLICITY. I have it running on clients' servers.
I do not yet understand the problem that's being discussed here or there.
Do you have something specific you'd like me to run down or to test?
jsmeix commented at 2024-11-14 09:30:¶
@loyeyoung
great that you responded!
FIY first my generic "boilerplate"
regarding third-party backup tools:
In general regarding third-party backup tools:
Usually we at ReaR upstream do not have or use
third-party backup tools so usually we cannot
reproduce issues with third-party backup tools
(in particular not if a third-party backup tool
is proprietary software).
In case of issues with third-party backup tools
we at ReaR upstream can usually do nothing
but totally depend on contributions and help
from those specific users who use and know
about each specific third-party backup tool.
This case here is one specific case
of the generic issue
https://github.com/rear/rear/issues/3259
I fail to understand what the BACKUP=DUPLICITY code
which I mentioned in
https://github.com/rear/rear/issues/3285#issuecomment-2244545130
actually does.
I think the intent of that code is to read
all or some needed values from DUPLY_PROFILE_FILE.
I think this is done by executing DUPLY_PROFILE_FILE
via 'source' as source $CONF.
So any command in DUPLY_PROFILE_FILE gets executed
as 'root' (because usr/sbin/rear runs as 'root').
This means any user who can modify DUPLY_PROFILE_FILE
can get any command executed as 'root' when
'root' runs usr/sbin/rear with BACKUP=DUPLICITY
So we need to carefully check if
executing DUPLY_PROFILE_FILE is really needed
of if it is possible to improve security
by only parsing it (without executing it), cf.
https://github.com/rear/rear/issues/3292
@loyeyoung
when you can test if ReaR with BACKUP=DUPLICITY works
for your particular use case, you could help me a lot
because then I could try to clean up and improve the
current "read DUPLY_PROFILE_FILE" code in ReaR
and you would test and verify that my modifications
don't break things - at least not for your particular
use case - which is very much more than no test at all.
gdha commented at 2024-11-14 10:20:¶
@jsmeix I just remember about an old blog https://www.it3.be/2015/09/02/rear-using-duply/ which showed an example of DUPLY_PROFILE usage.
jsmeix commented at 2024-11-20 12:59:¶
For ReaR 2.8 I will now clean up things with
sourcing DUPLY_PROFILE_FILE for BACKUP=DUPLICITY
to get that code reasonably simple and straightforward
which will make that stuff reasonably secure to use
and by the way may make it reasonably maintainable.
loyeyoung commented at 2024-11-20 18:40:¶
I expect to be able to test the changes @jsmeix makes.
code reasonably simple and straightforward
Much appreciated.
reasonably secure to use
Methinks the issue is not so much root privileges as orthogonality.
Bare metal backups require root privileges, both for ReaR and for whatever third party solution is used for encryption, rotation, etc. So duply/duplicity will always need root privileges anyway, and sourcing the config file does not escalate privilege.
However, because the duply and duplicity configuration files are sourced directly, bash will treat any line in those config files as a command and try to execute. Because the duply/duplicity developers (and perhaps end users) are not considering any implications the config file will have on ReaR, we cannot have any assurance that the duply/duplicity config file will not cause a problem for ReaR.
It seems that it should be possible to run duply/duplicity without directly sourcing its config file. I hope to dive into the setup of duply/duplicity and figure out if ReaR needs to source the config file at all. (@jsmeix's cleanup of the code will make that analysis much easier!)
jsmeix commented at 2024-11-21 13:01:¶
Sourcing files in ReaR does not escalate privileges
because usr/sbin/rear runs already as 'root'.
But sourcing files in ReaR that are not trustworthy
to be executed by 'root' is the problem, see the section
"Protecting Against Code Injections" in
https://relax-and-recover.org/documentation/security-architecture
Best is to not source files in ReaR that do not need
to be executed (e.g. ReaR itself needs to source
its own scripts to get ReaR's own code executed).
Whether or not third-party (backup) software (config) files
need to be executed is an issue that is on our
todo list for the next ReaR major version 3.0, see
https://relax-and-recover.org/documentation/security-architecture
For the current next ReaR version 2.8
that we want to get released rather soon
(the last release 2.7. was more than 2 years ago)
we prefer to keep things working backward compatible
as far as possible with reasonable effort.
In this case here I do not know what the syntax of the
duply profile is - I am not a duplicity or duply user.
Is it full bash syntax (e.g. with "if then else fi")
or is is restricted to only simple (how simple?)
bash style varaible assingnments of the form
NAME=fixed_literal_value
and what quoting characters are allowed
or is indirection allowed like
THIS='string of words'
THAT="prefix $THIS suffix"
and whatnot?
So for now for the 2.8 release I cannot replace
the possibly insecure sourcing/executing of the
duply profile with e.g. only 'grep' the needed values
out of it.
Therefore I decided to enforce that the user must
explicitly specify his correct and fully trusted
DUPLY_PROFILE, see my explanation in default.conf in
https://github.com/rear/rear/pull/3345/files
This is a needed backward incompatible change
to get things reasonably secure right now
as far as possible with reasonable effort and
this backward incompatibility is sufficiently easy
to be solved by the user.
jsmeix commented at 2024-11-21 13:11:¶
@loyeyoung
regarding testing my current changes, see
https://github.com/rear/rear/pull/3345#issuecomment-2491107351
jsmeix commented at 2024-11-25 12:16:¶
With
https://github.com/rear/rear/pull/3345
merged
this issue should be sufficiently mitigated for now, cf.
https://github.com/rear/rear/issues/3293#issuecomment-2491085941
jsmeix commented at 2024-11-25 12:55:¶
@loyeyoung
with
https://github.com/rear/rear/pull/3345
merged
my DUPLICITY code changes are in GitHub master code
so for testing it you do no longer need to do a
git checkout jsmeix-source-DUPLY_PROFILE
cf.
https://github.com/rear/rear/pull/3345#issuecomment-2491107351
instead you can test with GitHub master code directly
as described in the section
"Testing current ReaR upstream GitHub master code" in
https://en.opensuse.org/SDB:Disaster_Recovery
[Export of Github issue for rear/rear.]