[pacman-dev] [PATCH 1/2] makepkg: don't let the strip program mess up file attributes

newer
[pacman-dev] [PATCH] doxygen: fix...

older
[pacman-dev] [PATCH] libmakepkg:...

Eli Schwartz

8 Feb 2021 8 Feb '21

12:55 a.m.

It updates the stripped file by creating a temp file, chown/chmodding it, and replacing the original file. But upstream binutils has CVE-worthy issues with this if running strip as root, and some recent versions of strip don't play nicely with fakeroot. Also, this has always destroyed xattrs. :/ Sidestep the issue by telling strip to write to a temporary file, and manually dump the contents of that back into the original binary. Since the original binary is intact, albeit with different contents, it retains its correct attributes in fakeroot. Signed-off-by: Eli Schwartz --- scripts/libmakepkg/tidy/strip.sh.in | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/libmakepkg/tidy/strip.sh.in b/scripts/libmakepkg/tidy/strip.sh.in index 4d50f4475..f7238f813 100644 --- a/scripts/libmakepkg/tidy/strip.sh.in +++ b/scripts/libmakepkg/tidy/strip.sh.in @@ -93,7 +93,10 @@ strip_file() { fi fi - strip $@ "$binary" + if strip "$@" "$binary" -o "$binary.stripped"; then + cat "$binary.stripped" > "$binary" + fi + rm -f "$binary.stripped" } -- 2.30.0

Show replies by date

Eli Schwartz

8 Feb 8 Feb

12:55 a.m.

New subject: [pacman-dev] [PATCH 2/2] support xattr when extracting packages

This permits storing the result of setcap during package() and applying the resulting capabilities to the installed program. Formerly, it was necessary to edit the binary after the fact (and thus dirty the file according to -Qkk) by using an install scriptlet. One problem that needs to be solved before this is useful, is preventing the strip routine from destroying xattrs. This is taken care of in the previous patch. Signed-off-by: Eli Schwartz --- lib/libalpm/add.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index db0dba68d..53368b471 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -119,6 +119,7 @@ static int perform_extraction(alpm_handle_t *handle, struct archive *archive, ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_TIME | ARCHIVE_EXTRACT_UNLINK | + ARCHIVE_EXTRACT_XATTR | ARCHIVE_EXTRACT_SECURE_SYMLINKS; archive_entry_set_pathname(entry, filename); -- 2.30.0

Eli Schwartz

1:20 a.m.

On 2/7/21 7:55 PM, Eli Schwartz wrote:

...

It updates the stripped file by creating a temp file, chown/chmodding it, and replacing the original file. But upstream binutils has CVE-worthy issues with this if running strip as root, and some recent versions of strip don't play nicely with fakeroot.

Also, this has always destroyed xattrs. :/

Sidestep the issue by telling strip to write to a temporary file, and manually dump the contents of that back into the original binary. Since the original binary is intact, albeit with different contents, it retains its correct attributes in fakeroot.

Note: this is an alternative to Allan's patch "maintain file ownership while stripping". It does not rely on reintroducing @STATCMD@ and running chown, because that does not solve the xattr problem -- which is a problem that bothered me for a long time, but the binutils issue finally incentivized me sit down and implement this. Initially I wanted to use getfattr/setfattr, but this is not portable and does not solve the ownership issues either, at which point I realized retaining the original file is the simplest solution for both problems!

...

Signed-off-by: Eli Schwartz --- scripts/libmakepkg/tidy/strip.sh.in | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/libmakepkg/tidy/strip.sh.in b/scripts/libmakepkg/tidy/strip.sh.in index 4d50f4475..f7238f813 100644 --- a/scripts/libmakepkg/tidy/strip.sh.in +++ b/scripts/libmakepkg/tidy/strip.sh.in @@ -93,7 +93,10 @@ strip_file() { fi fi

- strip $@ "$binary" + if strip "$@" "$binary" -o "$binary.stripped"; then + cat "$binary.stripped" > "$binary" + fi + rm -f "$binary.stripped" }

-- Eli Schwartz Bug Wrangler and Trusted User

Allan McRae

3:44 a.m.

On 8/2/21 10:55 am, Eli Schwartz wrote:

...

It updates the stripped file by creating a temp file, chown/chmodding it, and replacing the original file. But upstream binutils has CVE-worthy issues with this if running strip as root, and some recent versions of strip don't play nicely with fakeroot.

Also, this has always destroyed xattrs. :/

Sidestep the issue by telling strip to write to a temporary file, and manually dump the contents of that back into the original binary. Since the original binary is intact, albeit with different contents, it retains its correct attributes in fakeroot.

This patch works when just stripping files, but fails to keep ownership correct when creating debug packages. We could apply it on top of my patch, so that we still maintain xattrs. Allan

...

Signed-off-by: Eli Schwartz --- scripts/libmakepkg/tidy/strip.sh.in | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/libmakepkg/tidy/strip.sh.in b/scripts/libmakepkg/tidy/strip.sh.in index 4d50f4475..f7238f813 100644 --- a/scripts/libmakepkg/tidy/strip.sh.in +++ b/scripts/libmakepkg/tidy/strip.sh.in @@ -93,7 +93,10 @@ strip_file() { fi fi

- strip $@ "$binary" + if strip "$@" "$binary" -o "$binary.stripped"; then + cat "$binary.stripped" > "$binary" + fi + rm -f "$binary.stripped" }

Eli Schwartz

3:54 a.m.

New subject: [pacman-dev] [PATCH v2 1/2] makepkg: don't let the strip routine mess up file attributes

It updates the stripped/objcopied file by creating a temp file, chown/chmodding it, and replacing the original file. But upstream binutils has CVE-worthy issues with this if running strip as root, and some recent versions of strip don't play nicely with fakeroot. Also, this has always destroyed xattrs. :/ Sidestep the issue by telling strip/objcopy to write to a temporary file, and manually dump the contents of that back into the original binary. Since the original binary is intact, albeit with different contents, it retains its correct attributes in fakeroot. Signed-off-by: Eli Schwartz --- v2: also use the `cat foo > bar` workaround for the second call site destroying file permissions. Yes, we need to do it this way and not with stat/chown, or objcopy will destroy both ownership and xattrs and we'll only restore the former. scripts/libmakepkg/tidy/strip.sh.in | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/scripts/libmakepkg/tidy/strip.sh.in b/scripts/libmakepkg/tidy/strip.sh.in index 868b96f3b..2212b8ec4 100644 --- a/scripts/libmakepkg/tidy/strip.sh.in +++ b/scripts/libmakepkg/tidy/strip.sh.in @@ -69,7 +69,9 @@ strip_file() { # copy debug symbols to debug directory mkdir -p "$dbgdir/${binary%/*}" objcopy --only-keep-debug "$binary" "$dbgdir/$binary.debug" - objcopy --add-gnu-debuglink="$dbgdir/${binary#/}.debug" "$binary" + objcopy --add-gnu-debuglink="$dbgdir/${binary#/}.debug" "$binary" "$binary.temp" + cat "$binary.temp" > "$binary" + rm -f "$binary.temp" # create any needed hardlinks while IFS= read -rd '' file ; do @@ -93,7 +95,10 @@ strip_file() { fi fi - strip $@ "$binary" + if strip "$@" "$binary" -o "$binary.stripped"; then + cat "$binary.stripped" > "$binary" + fi + rm -f "$binary.stripped" } -- 2.30.0

Eli Schwartz

4:09 a.m.

New subject: [pacman-dev] [PATCH v3 1/2] makepkg: don't let the strip routine mess up file attributes

It updates the stripped/objcopied file by creating a temp file, chown/chmodding it, and replacing the original file. But upstream binutils has CVE-worthy issues with this if running strip as root, and some recent versions of strip don't play nicely with fakeroot. Also, this has always destroyed xattrs. :/ Sidestep the issue by telling strip/objcopy to write to a temporary file, and manually dump the contents of that back into the original binary. Since the original binary is intact, albeit with different contents, it retains its correct attributes in fakeroot. Signed-off-by: Eli Schwartz --- v3: use mktemp to prevent clobbering mysterious packaged *.temp files scripts/libmakepkg/tidy/strip.sh.in | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/scripts/libmakepkg/tidy/strip.sh.in b/scripts/libmakepkg/tidy/strip.sh.in index 868b96f3b..9cb0fd8d0 100644 --- a/scripts/libmakepkg/tidy/strip.sh.in +++ b/scripts/libmakepkg/tidy/strip.sh.in @@ -69,7 +69,10 @@ strip_file() { # copy debug symbols to debug directory mkdir -p "$dbgdir/${binary%/*}" objcopy --only-keep-debug "$binary" "$dbgdir/$binary.debug" - objcopy --add-gnu-debuglink="$dbgdir/${binary#/}.debug" "$binary" + local tempfile=$(mktemp "$binary.XXXXXX") + objcopy --add-gnu-debuglink="$dbgdir/${binary#/}.debug" "$binary" "$tempfile" + cat "$tempfile" > "$binary" + rm "$tempfile" # create any needed hardlinks while IFS= read -rd '' file ; do @@ -93,7 +96,11 @@ strip_file() { fi fi - strip $@ "$binary" + local tempfile=$(mktemp "$binary.XXXXXX") + if strip "$@" "$binary" -o "$tempfile"; then + cat "$tempfile" > "$binary" + fi + rm -f "$tempfile" } -- 2.30.0

Allan McRae

4:24 a.m.

New subject: [pacman-dev] [PATCH v3 1/2] makepkg: don't let the strip routine mess up file attributes

On 8/2/21 2:09 pm, Eli Schwartz wrote:

...

It updates the stripped/objcopied file by creating a temp file, chown/chmodding it, and replacing the original file. But upstream binutils has CVE-worthy issues with this if running strip as root, and some recent versions of strip don't play nicely with fakeroot.

Also, this has always destroyed xattrs. :/

Sidestep the issue by telling strip/objcopy to write to a temporary file, and manually dump the contents of that back into the original binary. Since the original binary is intact, albeit with different contents, it retains its correct attributes in fakeroot.

Signed-off-by: Eli Schwartz ---

v3: use mktemp to prevent clobbering mysterious packaged *.temp files

Thanks - this version is good.

...

scripts/libmakepkg/tidy/strip.sh.in | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/scripts/libmakepkg/tidy/strip.sh.in b/scripts/libmakepkg/tidy/strip.sh.in index 868b96f3b..9cb0fd8d0 100644 --- a/scripts/libmakepkg/tidy/strip.sh.in +++ b/scripts/libmakepkg/tidy/strip.sh.in @@ -69,7 +69,10 @@ strip_file() { # copy debug symbols to debug directory mkdir -p "$dbgdir/${binary%/*}" objcopy --only-keep-debug "$binary" "$dbgdir/$binary.debug" - objcopy --add-gnu-debuglink="$dbgdir/${binary#/}.debug" "$binary" + local tempfile=$(mktemp "$binary.XXXXXX") + objcopy --add-gnu-debuglink="$dbgdir/${binary#/}.debug" "$binary" "$tempfile" + cat "$tempfile" > "$binary" + rm "$tempfile"

# create any needed hardlinks while IFS= read -rd '' file ; do @@ -93,7 +96,11 @@ strip_file() { fi fi

- strip $@ "$binary" + local tempfile=$(mktemp "$binary.XXXXXX") + if strip "$@" "$binary" -o "$tempfile"; then + cat "$tempfile" > "$binary" + fi + rm -f "$tempfile" }

-- 2.30.0 .

1183

Age (days ago)

1183

Last active (days ago)

List overview

Download

6 comments

2 participants

participants (2)

Allan McRae
Eli Schwartz