#2405 PR `closed`: Glob values in COPY_AS_IS shouldn't be quoted.¶

Labels: fixed / solved / done, critical / security / legal

flyinggreenfrog opened issue at 2020-05-24 16:29:¶

Otherwise they are not included.

Pull Request Details:¶

Type: Bug Fix
Impact: Normal (for me it's High otherwise I can't restore to LUKS encrypted device)
Reference to related issue (URL): No new explicit issue, but reoccurrence of #679 (or #972)
How was this pull request tested? openSUSE Leap 15.1. Baremetal via USB rescue medium
Brief description of the changes in this pull request:

With a3ce9fe020d0f8b423e01fd0001776198a21ed8e needed cracklib files were added. In https://github.com/rear/rear/issues/679#issuecomment-192379400 it was stated that this commit was fixing that issue.

However, if I test from current master ad0f26351860f5ef50a58a3ed7844353c196ab26 I ran into the same problems as in https://github.com/rear/rear/issues/679#issue-114138180.

If I change /usr/share/cracklib/\* to /usr/share/cracklib/* (this PR) needed cracklib files are present in rescue/restore system and restore works like expected.

Besides that PR, I'm asking myself:

What is the correct way to specify things to COPY_AS_IS with glob patterns?
According to conf/default.conf (line 1358 and following):
```
# Usually globbing patterns in COPY_AS_IS are specified without quoting
# like COPY_AS_IS+=( /my/directory/* /path/to/my/files* )
# so that the bash pathname expansion works as usually intended
```
Why was in a3ce9fe020d0f8b423e01fd0001776198a21ed8e /usr/share/cracklib/\* added instead of /usr/share/cracklib/* and it was working, and now in current master it doesn't work again?
I don't know, if #2378 changed some behavior unintentionally?

flyinggreenfrog commented at 2020-05-24 17:05:¶

Just tested with commit aa82834d0ae5415c17d8300f3733d621b0e91b63 (Tag 2.5): there the cracklib files were added to the rescue system.

jsmeix commented at 2020-05-25 09:40:¶

@flyinggreenfrog
thank you for your issue report.

Something severe has changed since the ReaR 2.5
in general what build/GNU/Linux/100_copy_as_is.sh
copies.

A lot of other stuff is no longer copied into the recovery system
because of lots of tar arguments that are now quoted.

This affects all places where tar arguments are set as

COPY_AS_IS+=( '/path/to/directory/*' )

but also in the older form

COPY_AS_IS=( "${COPY_AS_IS[@]}" '/path/to/directory/*' )

It seems the root cause happens before the script
usr/share/rear/build/GNU/Linux/100_copy_as_is.sh
is run because it seems when it is run those tar arguments
appear already quoted so that then tar is run as

tar ... '/path/to/directory/*'

which lets tar report

tar: /path/to/directory/*: Cannot stat: No such file or directory

for example:

# tar -cvf /tmp/testy.tar /usr/share/cracklib/*
tar: Removing leading `/' from member names
/usr/share/cracklib/cracklib.magic
tar: Removing leading `/' from hard link targets
/usr/share/cracklib/pw_dict.hwm
/usr/share/cracklib/pw_dict.pwd
/usr/share/cracklib/pw_dict.pwi

# tar -cvf /tmp/testy.tar '/usr/share/cracklib/*'
tar: Removing leading `/' from member names
tar: /usr/share/cracklib/*: Cannot stat: No such file or directory
tar: Exiting with failure status due to previous errors

jsmeix commented at 2020-05-25 10:22:¶

Got it!

I can make things work again when I add

COPY_AS_IS=( ${COPY_AS_IS[@]} )

at the beginning of usr/share/rear/build/GNU/Linux/100_copy_as_is.sh

Ugh!
What a mess we have in our code!

In ReaR 2.5 we had for example in
rescue/GNU/Linux/550_copy_ldconfig.sh

COPY_AS_IS=( ${COPY_AS_IS[@]} /etc/ld.so.conf /etc/ld.so.conf.d/* )

which is the last script that had always re-written COPY_AS_IS
before build/GNU/Linux/100_copy_as_is.sh is run
but now that was changed to

COPY_AS_IS+=( /etc/ld.so.conf /etc/ld.so.conf.d/* )

which does no longer re-write COPY_AS_IS
but keep its already existing entries as is.

The falsely used unquoted ${COPY_AS_IS[@]}
had resulted that the COPY_AS_IS array members
got rewritten in a duplicated falsely false way
so that things had worked by accident for tar.

For example

# tarfiles=( '/usr/share/cracklib/*' )

# tarfiles=( "${tarfiles[@]}" '/etc/issue' )

# ( set -x ; echo "${tarfiles[@]}" )
+ echo '/usr/share/cracklib/*' /etc/issue
/usr/share/cracklib/* /etc/issue

# ( set -x ; tar -cvf /tmp/testy.tar "${tarfiles[@]}" )
+ tar -cvf /tmp/testy.tar '/usr/share/cracklib/*' /etc/issue
tar: Removing leading `/' from member names
tar: /usr/share/cracklib/*: Cannot stat: No such file or directory
/etc/issue
tar: Removing leading `/' from hard link targets
tar: Exiting with failure status due to previous errors

or the same behaviour with array+=( additional elements )

# tarfiles=( '/usr/share/cracklib/*' )

# tarfiles+=( '/etc/issue' )

# ( set -x ; echo "${tarfiles[@]}" )
+ echo '/usr/share/cracklib/*' /etc/issue
/usr/share/cracklib/* /etc/issue

# ( set -x ; tar -cvf /tmp/testy.tar "${tarfiles[@]}" )
+ tar -cvf /tmp/testy.tar '/usr/share/cracklib/*' /etc/issue
tar: Removing leading `/' from member names
tar: /usr/share/cracklib/*: Cannot stat: No such file or directory
/etc/issue
tar: Removing leading `/' from hard link targets
tar: Exiting with failure status due to previous errors

versus duplicated falsely false things that work by accident:

# tarfiles=( '/usr/share/cracklib/*' )

# tarfiles=( ${tarfiles[@]} '/etc/issue' )

# ( set -x ; echo "${tarfiles[@]}" )
+ echo /usr/share/cracklib/cracklib.magic /usr/share/cracklib/pw_dict.hwm /usr/share/cracklib/pw_dict.pwd /usr/share/cracklib/pw_dict.pwi /etc/issue
/usr/share/cracklib/cracklib.magic /usr/share/cracklib/pw_dict.hwm /usr/share/cracklib/pw_dict.pwd /usr/share/cracklib/pw_dict.pwi /etc/issue

# ( set -x ; tar -cvf /tmp/testy.tar "${tarfiles[@]}" )
+ tar -cvf /tmp/testy.tar /usr/share/cracklib/cracklib.magic /usr/share/cracklib/pw_dict.hwm /usr/share/cracklib/pw_dict.pwd /usr/share/cracklib/pw_dict.pwi /etc/issue
tar: Removing leading `/' from member names
/usr/share/cracklib/cracklib.magic
tar: Removing leading `/' from hard link targets
/usr/share/cracklib/pw_dict.hwm
/usr/share/cracklib/pw_dict.pwd
/usr/share/cracklib/pw_dict.pwi
/etc/issue

I will clean that up...

jsmeix commented at 2020-05-25 10:53:¶

Currently I think a minimal change to make things work again
and to not cause unexpected regressions for users that use
in their etc/rear/local.conf any kind of the methods

COPY_AS_IS=( "${COPY_AS_IS[@]}" '/path/to/directory/*' )
COPY_AS_IS=( ${COPY_AS_IS[@]} /path/to/directory/* )
COPY_AS_IS+=( '/path/to/directory/*' )
COPY_AS_IS+=( /path/to/directory/* )

is the following diff in build/GNU/Linux/100_copy_as_is.sh

--- a/usr/share/rear/build/GNU/Linux/100_copy_as_is.sh
+++ b/usr/share/rear/build/GNU/Linux/100_copy_as_is.sh
@@ -84,7 +84,7 @@ done >$copy_as_is_exclude_file
 #  -rw-r--r-- root/root         4 2017-10-12 11:31 foo
 #  -rw-r--r-- root/root         4 2017-10-12 11:31 baz
 # Because pipefail is not set it is the second 'tar' in the pipe that determines whether or not the whole operation was successful:
-if ! tar -v -X $copy_as_is_exclude_file -P -C / -c "${COPY_AS_IS[@]}" 2>$copy_as_is_filelist_file | tar $v -C $ROOTFS_DIR/ -x 1>/dev/null ; then
+if ! tar -v -X $copy_as_is_exclude_file -P -C / -c ${COPY_AS_IS[@]} 2>$copy_as_is_filelist_file | tar $v -C $ROOTFS_DIR/ -x 1>/dev/null ; then
     Error "Failed to copy files and directories in COPY_AS_IS minus COPY_AS_IS_EXCLUDE"
 fi
 Log "Finished copying files and directories in COPY_AS_IS minus COPY_AS_IS_EXCLUDE"

i.e. use unquoted ${COPY_AS_IS[@]} in the tar command call
or use ${COPY_AS_IS[*]} to make it more obvious what is meant.

jsmeix commented at 2020-05-25 11:30:¶

With
https://github.com/rear/rear/commit/014867e90ad2e564d8dd8db6ec15ec1dc0b0a7ba
I use now plain ${COPY_AS_IS[*]} for tar
to copy things into the recovery system, cf.
https://github.com/rear/rear/pull/2405#issuecomment-633512932

jsmeix commented at 2020-05-25 11:39:¶

@flyinggreenfrog
could you verify if things work again for you
with current GitHub master code i.e. with an unchanged
layout/save/GNU/Linux/260_crypt_layout.sh but with
https://github.com/rear/rear/commit/014867e90ad2e564d8dd8db6ec15ec1dc0b0a7ba

flyinggreenfrog commented at 2020-05-25 11:44:¶

Just for the sake of completeness, I added 6c52b449017260268c16a40192eacb17e53e2d00 to revert my previous commit for fixing this particular item, to prefer the global solution 014867e90ad2e564d8dd8db6ec15ec1dc0b0a7ba from @jsmeix.

Will test current master with 014867e90ad2e564d8dd8db6ec15ec1dc0b0a7ba.

flyinggreenfrog commented at 2020-05-25 12:30:¶

@flyinggreenfrog
could you verify if things work again for you
with current GitHub master code i.e. with an unchanged
layout/save/GNU/Linux/260_crypt_layout.sh but with
014867e

At current master with your commit it's working for me again.

Just tested backup/restore for me.

@jsmeix Thanks for your fast solution of this issue.

I assume I just delete this branch, since finally no new commit was added? Or will it still be merged with commit and reverting commit to have this issue shown in the history?

jsmeix commented at 2020-05-25 12:50:¶

The underlying issue is fixed via
https://github.com/rear/rear/commit/014867e90ad2e564d8dd8db6ec15ec1dc0b0a7ba
so I think I can close this pull request.

jsmeix commented at 2020-05-25 13:28:¶

A side note:

Currently we have several places where we specify

COPY_AS_IS+=( /path/to/directory/* )

instead of just the wanted directory

COPY_AS_IS+=( /path/to/directory )

I found out that the /path/to/directory/* form is another fragile way
how ReaR code works in certain cases because of sublte side-effects.

I tried if in usr/share/rear/conf/GNU/Linux.conf
the following diff also works

-COPY_AS_IS+=( '/etc/ssl/certs/*' '/etc/pki/*' '/usr/lib/ssl/*' '/usr/share/ca-certificates/*' '/etc/ca-certificates/*' )
+COPY_AS_IS+=( /etc/ssl/certs/ /etc/pki/ /usr/lib/ssl/ /usr/share/ca-certificates/ /etc/ca-certificates/ )

That failed because I got only the plain /etc/ssl/certs/ directory
without any files therein copied into the recovery system.
But the other directories were copied with all their contents.

The reason is that (at least on my openSUSE leap 15.1 system)
/etc/ssl/certs/ is a symlink with target /var/lib/ca-certificates/pem
and the tar in build/GNU/Linux/100_copy_as_is.sh must intentionally
not follow symlinks (see the comment in 100_copy_as_is.sh).

So when /path/to/directory is a symlink it helps to specify
COPY_AS_IS+=( /path/to/directory/* ) to get all directory contents
copied into the recovery system (of course except files that start with .)

In particular regarding the /etc/ssl/certs/* usage see
https://github.com/rear/rear/pull/1971

pcahyna commented at 2020-05-25 17:40:¶

@jsmeix in order to perform filename expansion (glob expansion) but not word splitting (to support filenames with spaces properly), one can apparently set IFS to an empty string.

pcahyna commented at 2020-05-25 17:50:¶

... or maybe not, not sure myself whether it behaves properly.

jsmeix commented at 2020-05-26 06:25:¶

@pcahyna
I would very much appreciate it if you could help to find a good solution
for our current fragile way how things cet copied into the recovery system.

For now - i.e. for the ReaR 2.6 release - I will better explain in default.conf
how COPY_AS_IS must be used, in particular that symlinks cannot be followed.

After the ReaR 2.6 release we can think about how to improve things
in a more relaxed way.

The current tar ... ${COPY_AS_IS[*]} works same as things had worked
all the time before via accidental COPY_AS_IS=( ${COPY_AS_IS[@]} ... )
so there are no regressions but now it is described at the right place
(i.e. in build/GNU/Linux/100_copy_as_is.sh) how things work.

pcahyna commented at 2020-05-26 09:19:¶

@jsmeix I understand and very much appreciate the point about no regressions. I would like to improve this area, but unfortunately the amount of time I can spend on ReaR nowadays is limited, so I can't promise anything.

jsmeix commented at 2020-05-26 09:45:¶

@pcahyna
no worries - take your time - cf. my
https://github.com/rear/rear/issues/799#issuecomment-531247109

jsmeix commented at 2020-05-26 09:46:¶

Via
https://github.com/rear/rear/commit/4cbd4249d5aece70448b995e5779706954e98863
it is now better explained in default.conf how COPY_AS_IS works,
in particular that symlinks cannot be followed and
that files or directories that contain blanks or
other $IFS characters cannot be specified.

[Export of Github issue for rear/rear.]

#2405 PR closed: Glob values in COPY_AS_IS shouldn't be quoted.¶