Thanks for the report, so fixed in backuppc-4.1.3-3.mga7

CC: (none) => geiger.david68210
Resolution: (none) => FIXED
Status: NEW => RESOLVED

Thanks for the rapid action. I was just drafting a message to the dev (or discuss) ML suggesting and arguing that it would be a good thing to have a more recent version of BackupPC in Mageia-7 - dropped this mail for the moment in favour of this bugzilla comment.

BackupPC-4.1.3 is one of the very early releases in the backuppc-4 series. The latest version (BackupPC-4.3.0) contains a lot of bug-fixes and improvements which reflect the maturation of version 4. I dont think that upgrading the Mageia package with these modifications would entail a big packaging effort. What do you think?

Then reopened and assigning to the registered maintainer!

Status: RESOLVED => REOPENED
Resolution: FIXED => (none)
Assignee: bugsquad => shlomif

I just discovered a rather server new bug: BackupPC 4 creates all backup files (cpool directory) with world rwx permission. That is bad security - and annoying since msec becomes clogged with corresponding warnings - an additional security issue since that makes msec output more or less unreadable.

I will signal this to the BackupPc mailing list (maybe I can concoct a temporary fix for rapid remedy).

Given the security aspect, do you want me to file a specify bug on this topic?

Things have become somewhat clearer:
  - the huge number of msec messages due to this security problem made me over-react: world-writable properties are only set on directories;
  - the file nodes are protected conservatively;
  - nevertheless, this issue is serious - anybody with access to the server can easily corrupt backup storage (add, delete, replace backed-up files).

There is also a positive aspect: a discussion on the backuppc mailing list does only make sense if it refers to the latest release. I therefore did a local build of a 4.3.0 backuppc package - just needed to add in the correct sources and bump release specifications - no need stick to 4.1.3 (but the security issue is also present on 4.3.0).

Digging into the code, I have so far not found a quick and easy way to temporarily fix the problem (backuppc is full with calls to mkpath with 0777 properties).

I spent some time trying to clarify - a short note on what I have seen

- the problem appears to be limited to installations that use rsync(d), i.e. a problem of perl-BackupPC-XS

- a local build of BackupPC-XS terminates with "success", but at a closer look, there are severe warnings which indicate that buffers are too small, data getting truncated or causing clobbering (to get more details, do a local build and have a look at the error messages)

- initially I suspected the upstream code, but it now looks that chances are big that these overflow problems are Mageia-7 or Mageia-packaging specific - a "make" of the upstream tarball (but on a 6.1 system) does not flag a single error.

I suggest to start by evident things like adapting the array dimensions that overflow (e.g. modify LogData.mesgSize) and purging these build warnings.

If there is need, I should be able to give a hand with that, but will be away from my server for three weeks, and have limited time and skills (and working with C-code wrapped up in perl modules and packages is a real pain in the neck).

The downgrade to 4.3 is a good move - provides continuity and safety for users of backuppc. But on the long run, 4.3 will die.

(In reply to Juergen Harms from comment #6)
> I spent some time trying to clarify - a short note on what I have seen
> 
> - the problem appears to be limited to installations that use rsync(d), i.e.
> a problem of perl-BackupPC-XS
> 
> - a local build of BackupPC-XS terminates with "success", but at a closer
> look, there are severe warnings which indicate that buffers are too small,
> data getting truncated or causing clobbering (to get more details, do a
> local build and have a look at the error messages)
> 
> - initially I suspected the upstream code, but it now looks that chances are
> big that these overflow problems are Mageia-7 or Mageia-packaging specific -
> a "make" of the upstream tarball (but on a 6.1 system) does not flag a
> single error.
> 

The mageia build process adds some warning flags to gcc/etc. which are not normally there in manual use.

> I suggest to start by evident things like adapting the array dimensions that
> overflow (e.g. modify LogData.mesgSize) and purging these build warnings.
> 
> If there is need, I should be able to give a hand with that, but will be
> away from my server for three weeks, and have limited time and skills (and
> working with C-code wrapped up in perl modules and packages is a real pain
> in the neck).
> 
> The downgrade to 4.3 is a good move - provides continuity and safety for
> users of backuppc. But on the long run, 4.3 will die.

I now tried making the tarball on Mageia-7: as opposed to Mageia-6, a lot of similar warnings do now appear.

Googling for this kind of warning seems to indicate that gcc-8 has new checks against format truncation in the source code
https://stackoverflow.com/questions/51534284/how-to-circumvent-format-truncation-warning-in-gcc

I have finally got around to understand what is happening, and to suggest a patch that fixes the problem. This fix solves to problem, but it is a workaround: a solid solution would better come from upstream.

The problem why directories with backup data are created with world-write permission lies in the procedure "bpc_path_create" (module rsync-bpc-3.0.9.8/backuppc/bpc_dirOPS.c) of the "rsync-bpc-3.0.9.8" package: it provides the "mode" argument of the call to mkdir by the constant ACCESSPERMS; the value of that constant is - inadequately - 0777. I suggest a patch that simply replaces the constant ACCESSPERMS by directly specifying the value of 0770. I will append a corresponding patch file.

I have made a local build from the corresponding SRPMs from the Mageia repository (in the case of rsync-bpc including the suggested patch). Using the thus created backuppc works without problems, the directories with backup data are now protected against world-access - have the same permission mode as data created by backuppc version-3. "bpc_path_create" is called from some 3 or 4 places, but in the environment of backuppc there cannot be any adverse side-effects related to the protection against world-access, and I have not observed any.

One suggestion:

The transition from version 3 to version 4 of backuppc is not transparent to the user: the format of the backup data is modified, and the user needs to manually modify the config.pl file. Some users might temporarily wish to maintain using backuppc 3. How about not only offering the three new version 4 backuppc packages, but also keeping a backuppc version-3 package in the repositories (for instance, renaming it to something like backuppc3-3.3.2.1) ?

Created attachment 10721 [details]
file access_perms.patch (patch file for rsync-bpc-3.0.9.8)

Here are some additional observations that might help with future work on the backuppc packages.

Issue 1: Determining permissions for the directories created by backuppc
------------------------------------------------------------------------
Currently, the packages implement 2 seriously conflicting approaches for determining permissions of directories with backup data:

a) using a hardwired value for the permission mode; this is presently implemented (in the faulty rsync-bpc the constant ACCESSPERMS, for the patched rsync-bpc the value 0770);

b) use the dynamic value, defined by the "UmaskMode" configuration parameter in config.pl; this value is newly read each time backuppc is (re-)started, and also when the modificiation time of a host configuration is modified; this is presently documented (comments in config.pl) but not implemented and does not correspond to reality.


Issue 2: redundancy of procedures in perl-BackupPC-XS and rsync-bpc:
--------------------------------------------------------------------
Most bpc_... procedures (e.g. the bpc_dirOps.c subject of the suggested patch) are redundantly defined both in the perl-BackupPC-XS and in the rsync-bpc package, without any apparent differences; those in perl-BackupPC_XS appear to be "dead" - not used: applying the patch to rsync-bpc was sufficient; the reason for the duplification of these procedures is not evident - looks like a packaging problem.

This redundancy is not only confusing, but may be the source of errors and loss of time with future work on the packages


Issue 3: Warnings appearing during package build
------------------------------------------------
When any of the 3 packages is built under cauldron, build issues an important amount of warnings - mainly concerning the size of buffers for string data. This was not the case for compilations under Mageia-6, and is certainly due to stricter checking by gcc-8. Most warnings are trivial, but I have seen 1 or 2 cases where the size of a buffer should really be increased to prevent clobbering - sorry, I have lost the notes I made on that. But anyhow, that is something upstream will probably pick up.

Re-visiting, I think the 0770 permission I naively implemented in the attached patch should be modified to 0750, blocking also group-write access. That also has the advantage to conform to what the UmaskMode config variable promises to do, but does not do.

I did some additional testing, appending version-4 backupps to existing version-3 ones, the ordinary situation after an upgrade (so far I had only tested on virgin version-4 backup storage):
- no problem
- no problem even if the user forgets to modify the config.pl file when he updates from version 3 to version 4 without being aware of that upgrade (the web interface will simply refuse to launch starting to dump backup data).

Nevertheless, it would be good to add a remark on the backuppc 3 -> 4 transition into the release notes. Once the patched backuppc appears in the cauldron repository, I will make a corresponding comment in the "discuss" ml.

Still valid.

Workaround available

Just tried Beta-2 as production support - my backuppc storage got again polluted by tons of world-writable nodes: 

In reaction to this bug report, backuppc got reverted to backuppc version 3; but unpatched BackupPC version 4 has made into Beta-2, which causes havock. Is this design - based on what consideration? -, or a handling error at iso building that should be signalled ?

(In reply to Juergen Harms from comment #14)
> Just tried Beta-2 as production support - my backuppc storage got again
> polluted by tons of world-writable nodes: 
> 
> In reaction to this bug report, backuppc got reverted to backuppc version 3;
> but unpatched BackupPC version 4 has made into Beta-2, which causes havock.
> Is this design - based on what consideration? -, or a handling error at iso
> building that should be signalled ?

I don't think it got reverted, on the mirrors we have backuppc-4.3.0-1.mga7 since December 17

CC: (none) => marja11

Thanks for picking this up - and sorry for the mess in the comments. A concise summary:

1. Bug initially opened on a packaging problem of backuppc 4.1.3: missing require for perl-BackupPC-XS-0

2. Require fixed, backuppc 4.1.3 upgraded to 4.3.0 -> mirrors, bug closed.

3. Bug re-opened because backuppc-4.3.0 (as did 4.1.3) creates directories for backed-up data with world-write permission (directories under /var/lib/backuppc/pc/...)

In consequence,  cauldron mirrors were reverted to backuppc version 3 (I think 3.3.2; I have no precise date, I am sure the switch to backuppc 3 had already happened on Dec. 18) and remained so until a short time before the release of Beta-2.

4. Simple patch provided as a workaround for this problem (attachment 10721 [details], comments 9 and 12). Locally builds and works fine and does not mess up the backup storage any more.

5. Present situation:

Unpatched backuppc-4.3.0
 - works smoothly, but creates backup data in /var/lib/backuppc/pc/... in directories that have world write permission;
 - this is not dramatic as long as /var/lib/backuppc/ does not have world-execute permission, or as backup-up data are not exported; but backed-up data are kept over years, and accumulating directories with non-perfect protection is bad security

Patched backuppc-4.3.0 stops the world-write problem (it is a very local patch that cannot have side-effects).

The same bug of upstream backuppc also has the consequence that the configuration variable $Conf{UmaskMode} does not determine the permissions of backed-up data as it should (nor does the patch correct this problem) - but this is a very minor issue.

My suggestion is to add the proposed patch to backuppc-4.3.0 before the next isos are created.

A new bug report , bug 24441, was created for the problem the world writeable directories. CLosing this bug as fixed, because the original issue was fixed.

(In reply to Marja Van Waes from comment #17)
> A new bug report , bug 24441, was created for the problem the world
> writeable directories. CLosing this bug as fixed, because the original issue
> was fixed.

Status: REOPENED => RESOLVED
Resolution: (none) => FIXED