Hello,
Attached is a patch to version 3.0.2 to correct what I believe to be
a bug and serious problem in rsync. This patch causes rsync to honor the
absence of the "--inplace" option for permission, owner and group
changes. This patch is a greatly simplified version of Matt McCutchen's
excellent "tweak-opts" patch found in his git repository
(http://mattmccutchen.net/rsync/rsync.git/?a=shortlog;h=refs/heads/tweak-opts).
However this patch does not add any new options. Without this patch
rsync will always modify files in place to reflect changes in
permissions, ownership or group without regard to the "--inplace"
option. The "--inplace" option is supposed to regulate whether files to
be backed up that are already found in the destination directory are
modified in place or simply used as the basis for new files. Not being
able to completely eliminate modifying files in place greatly and
negatively affects a very common use of rsync: backup systems that
create incremental backup snapshots using hard links. In such systems
the current behavior of rsync makes it a trivial exercise for a
misconfigured or compromised / malicious client to seriously damage or
even destroy all current and prior backup snapshots in the set. The
"--link-dest" option attempts to solve the problem but adds serious
problems of its own. Some of the problems I see are:
1. *The hard links and permission / owner / group change problem:*
This is a problem that has been well known for many years. When
using hard links to implement snapshots (as most incremental
snapshot backup systems that utilize rsync do) when files are
modified in place (as rsync always does for permission, owner and
group changes without any regard for the "--inplace" option) those
changes affect not just those files in the current backup but also
in all previous backups (since the last time the contents of the
file were changed). So if for example a client machine is
compromised and the attacker executes the command *"chmod -R 666
/**"* the next time the rsync backup is performed *all* of the
backup snapshots become unreliable. This can also happen if the
filesystem is accidentally backed up and restored locally using a
medium that does not support permissions.
2. *Problems with the "--link-dest" option:* The "--link-dest" option
was apparently added to rsync to address the above problem but it
does so in a way that introduces vulnerabilities of its own.
1. A big problem with "--link-dest" is that it requires that
the client machine be correctly configured to work. There is
no way to enforce a correct "--link-dest" option on the
server. Not only does the client need to specify the option
but also its use of the option needs to exactly match what
the rotation / renaming scripts on the server expect or
not-so-wonderful things will happen (you can use your
imagination). Configuration mistakes or malicious users can
wreak all sorts of havoc by omitting or playing with this
option. *Any backup solution that requires the server to
trust that the client is properly configured in order for
the server to work correctly is not reliable.
*
2. Another big problem with "--link-dest" is that when used as
documented and intended there is only one copy of the
previous backup and *it cannot be protected from the forked
daemon by chroot!* The daemon must have direct access to
the one and only good copy of your data so a bug, exploit or
malicious client can easily clobber it. *Any backup solution
that requires the server to trust that the client is
properly configured in order to protect the integrity of
previous backups is not reliable.* On the other hand if the
linked copy of the backup is created *after* the rsync
daemon has finished (i.e., not using "--link-dest") your
previous backups can be protected from the daemon by chroot
with only a copy of the data acted upon by the daemon.*
3. Lots of other problems and confusion related to
"--link-dest" . (See bugs
<https://bugzilla.samba.org/buglist.cgi?query_format=specific&order=relevance+desc&bug_status=__all__&product=rsync&content=--link-dest>).
I hope the above adequately explains why this is an important problem
and why the "--link-dest" option should be avoided by anyone who cares
about the integrity of their backups. It's my opinion that this simple
patch makes any rsync-based backup system that uses hard links (as so
many do) more reliable and robust and significantly helps maintain the
integrity of the backups. It does not require any changes to the
configuration or use of the clients, server or backup system to achieve
this benefit.
*I do not mean to imply by this that *chroot()*, particularly the way
it's used by rsync, is more than trivial protection but it can help make
things a little safer.
Thank you,
Carl E. Thompson
Who am I? I have been a professional software developer and engineer for
about 20 years and have specialized in Unix and Linux networking and
security for the last 14 years.
diff -urN rsync-3.0.2-orig/generator.c rsync-3.0.2/generator.c
--- rsync-3.0.2-orig/generator.c 2008-03-28 10:30:11.000000000 -0700
+++ rsync-3.0.2/generator.c 2008-05-07 15:35:08.317364774 -0700
@@ -1508,6 +1508,7 @@
if (preserve_links && S_ISLNK(file->mode)) {
#ifdef SUPPORT_LINKS
+ int iflags = 0;
const char *sl = F_SYMLINK(file);
if (safe_symlinks && unsafe_symlink(sl, fname)) {
if (verbose) {
@@ -1528,7 +1529,15 @@
else if ((len = readlink(fname, lnk, MAXPATHLEN-1)) > 0
&& strncmp(lnk, sl, len) == 0 && sl[len] == '\0')
{
/* The link is pointing to the right place. */
- set_file_attrs(fname, file, &sx, NULL,
maybe_ATTRS_REPORT);
+ if (inplace) {
+ if (verbose > 2)
+ rprintf(FINFO, "possibly
tweaking attributes of %s\n", fname);
+ set_file_attrs(fname, file, &sx, NULL,
maybe_ATTRS_REPORT);
+ } else if (!unchanged_attrs(fname, file, &sx)) {
+ if (verbose > 2)
+ rprintf(FINFO, "recreating %s
due to changed attributes\n", fname);
+ goto recreate_symlink;
+ }
if (itemizing)
itemize(fname, file, ndx, 0, &sx, 0, 0,
NULL);
#if defined SUPPORT_HARD_LINKS && defined CAN_HARDLINK_SYMLINK
@@ -1538,7 +1547,9 @@
if (remove_source_files == 1)
goto return_with_success;
goto cleanup;
- }
+ } else
+ iflags = ITEM_REPORT_CHANGE;
+ recreate_symlink:
/* Not the right symlink (or not a symlink), so
* delete it. */
if (delete_item(fname, sx.st.st_mode, del_opts |
DEL_FOR_SYMLINK) != 0)
@@ -1572,18 +1583,20 @@
set_file_attrs(fname, file, NULL, NULL, 0);
if (itemizing) {
itemize(fname, file, ndx, statret, &sx,
- ITEM_LOCAL_CHANGE|ITEM_REPORT_CHANGE,
0, NULL);
+ ITEM_LOCAL_CHANGE|iflags, 0, NULL);
}
- if (code != FNONE && verbose)
+ if ((iflags & ITEM_REPORT_CHANGE) && code != FNONE &&
verbose)
rprintf(code, "%s -> %s\n", fname, sl);
#ifdef SUPPORT_HARD_LINKS
if (preserve_hard_links && F_IS_HLINKED(file))
finish_hard_link(file, fname, ndx, NULL,
itemizing, code, -1);
#endif
- /* This does not check remove_source_files == 1
- * because this is one of the items that the old
- * --remove-sent-files option would remove. */
- if (remove_source_files)
+ /* When the symlink value changed, we do not check
+ * remove_source_files == 1 because this is one of the
+ * items that the old --remove-sent-files option would
+ * remove. */
+ if ((iflags & ITEM_REPORT_CHANGE) ? remove_source_files
+ : remove_source_files == 1)
goto return_with_success;
}
#endif
@@ -1592,6 +1605,7 @@
if ((am_root && preserve_devices && IS_DEVICE(file->mode))
|| (preserve_specials && IS_SPECIAL(file->mode))) {
+ int iflags = 0;
uint32 *devp = F_RDEV_P(file);
dev_t rdev = MAKEDEV(DEV_MAJOR(devp), DEV_MINOR(devp));
if (statret == 0) {
@@ -1609,7 +1623,15 @@
&& BITS_EQUAL(sx.st.st_mode, file->mode, _S_IFMT)
&& sx.st.st_rdev == rdev) {
/* The device or special file is identical. */
- set_file_attrs(fname, file, &sx, NULL,
maybe_ATTRS_REPORT);
+ if (inplace) {
+ if (verbose > 2)
+ rprintf(FINFO, "possibly
tweaking attributes of %s\n", fname);
+ set_file_attrs(fname, file, &sx, NULL,
maybe_ATTRS_REPORT);
+ } else if (!unchanged_attrs(fname, file, &sx)) {
+ if (verbose > 2)
+ rprintf(FINFO, "recreating %s
due to changed attributes\n", fname);
+ goto recreate_D;
+ }
if (itemizing)
itemize(fname, file, ndx, 0, &sx, 0, 0,
NULL);
#ifdef SUPPORT_HARD_LINKS
@@ -1619,7 +1641,9 @@
if (remove_source_files == 1)
goto return_with_success;
goto cleanup;
- }
+ } else
+ iflags = ITEM_REPORT_CHANGE;
+ recreate_D:
if (delete_item(fname, sx.st.st_mode, del_opts |
del_for_flag) != 0)
goto cleanup;
} else if (basis_dir[0] != NULL) {
@@ -1656,9 +1680,9 @@
set_file_attrs(fname, file, NULL, NULL, 0);
if (itemizing) {
itemize(fname, file, ndx, statret, &sx,
- ITEM_LOCAL_CHANGE|ITEM_REPORT_CHANGE,
0, NULL);
+ ITEM_LOCAL_CHANGE|iflags, 0, NULL);
}
- if (code != FNONE && verbose)
+ if ((iflags & ITEM_REPORT_CHANGE) && code != FNONE &&
verbose)
rprintf(code, "%s\n", fname);
#ifdef SUPPORT_HARD_LINKS
if (preserve_hard_links && F_IS_HLINKED(file))
@@ -1776,13 +1800,31 @@
else if (fnamecmp_type == FNAMECMP_FUZZY)
;
else if (unchanged_file(fnamecmp, file, &sx.st)) {
+ /* fnamecmp == fname, fnamecmp_type == FNAMECMP_FNAME */
+ int iflags = 0;
+
if (partialptr) {
do_unlink(partialptr);
handle_partial_dir(partialptr, PDIR_DELETE);
}
- set_file_attrs(fname, file, &sx, NULL, maybe_ATTRS_REPORT);
+ if (inplace) {
+ /* Currently, we call set_file_attrs on all tweakable
+ * files, though ideally it would have no effect when
+ * unchanged_attrs returns true. */
+ if (verbose > 2)
+ rprintf(FINFO, "possibly tweaking attributes of
%s\n", fname);
+ set_file_attrs(fname, file, &sx, NULL,
maybe_ATTRS_REPORT);
+ } else if (!unchanged_attrs(fname, file, &sx)) {
+ /* Need to recreate the file.
+ * copy_altdest_file sets its attributes, etc. */
+ if (verbose > 2)
+ rprintf(FINFO, "recreating %s due to changed
attributes\n", fname);
+ if (!dry_run && copy_altdest_file(fnamecmp, fname,
file))
+ goto cleanup;
+ iflags |= ITEM_LOCAL_CHANGE;
+ }
if (itemizing)
- itemize(fnamecmp, file, ndx, statret, &sx, 0, 0, NULL);
+ itemize(fnamecmp, file, ndx, statret, &sx, iflags, 0,
NULL);
#ifdef SUPPORT_HARD_LINKS
if (preserve_hard_links && F_IS_HLINKED(file))
finish_hard_link(file, fname, ndx, &sx.st, itemizing,
code, -1);
--
Please use reply-all for most replies to avoid omitting the mailing list.
To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync
Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html
