Re: [PATCH] Prevent Perl exec function from ever interpreting commands as shell
Hi!
On Wed, 2023-06-14 at 09:49:41 +0800, Paul Wise wrote:
> On Tue, 2023-06-13 at 11:56 +0200, Guillem Jover wrote:
> > This change would break the current semantics for this option, as it
> > is specified to take a "command-string". Perhaps the man page needs to
> > be improved to make all this more clear. Where did you find this to be
> > a problem? (If we'd really wanted to change the semantics that would
> > require an explicit deprecation cycle, first try to catch affected usage
> > and warn, then emit errors on those and use the new semantics, etc.,
> > but it's not clear to me this would be the better option.)
>
> I was writing this mail when I stumbled upon the behaviour:
>
> https://lists.debian.org/msgid-search/da17fe19bcc05ba1032e5d81112bb6715825a6a7.camel@debian.org
>
> Ever since writing an old blog post about shell metacharacter injection
> attacks, I am always looking for situations where shell metacharacter
> injection could lead to unwanted or unexpected behaviour and I vastly
> prefer consistent C-style exec behaviour where there is no chance of
> the shell being involved and messing around with the command. IMO the
> decision to use shell should always be on the user and require the user
> to explicitly call the shell and write a shell script (or use `sh -c`).
>
> https://bonedaddy.net/pabs3/log/2014/02/17/pid-preservation-society/
>
> So I think that at minimum, the current behaviour should be documented.
Attached is the man page update I've got queued locally, but I'm happy
to clarify further if you think that would be insufficient.
> Perhaps it should go through the deprecation cycle you mention too.
I checked on sources.d.n, and AFAIR only saw a couple of potentially
problematic instances. But this might be affecting local scripts and
tooling. I'll ponder about it, or perhaps ask on d-d or similar
whether this would be very disruptive.
> > While applying this would not harm, it should also not be needed as we
> > always pass at leasth three three elements in @cmds. Well it might on
> > Windows, but I don't think it is generally supported anyway.
>
> Yes I understood that, but I thought it would be best to change the
> style of exec usage to be consistent throughout dpkg. Dpkg::IPC uses
> this style, dpkg-architecture should and the whole codebase should too.
Ack, I considered whether switching that script to use Dpkg::IPC would
be better, but it does not depend on any other Dpkg module, and it is
intended to be run from the build tree, so your change as-is looks good,
and so I've queued that part too.
And I found another instance in the dpkg-shlibdeps code which I've
also fixed.
I'm attaching the three patches. Thanks!
Regards,
Guillem
From 68bd49c89adec124962a1fd654a488868ac08d00 Mon Sep 17 00:00:00 2001
From: Guillem Jover <guillem@debian.org>
Date: Thu, 15 Jun 2023 23:49:51 +0200
Subject: [PATCH 1/3] man: Clarify dpkg-architecture -c option
Prompted-by: Paul Wise <pabs@debian.org>
---
man/dpkg-architecture.pod | 3 +++
1 file changed, 3 insertions(+)
diff --git a/man/dpkg-architecture.pod b/man/dpkg-architecture.pod
index ea963db36..539f700d4 100644
--- a/man/dpkg-architecture.pod
+++ b/man/dpkg-architecture.pod
@@ -93,6 +93,9 @@ Print a similar command to B<--print-set> but to unset all variables.
Execute a I<command-string> in an environment which has all variables
set to the determined value.
+If the I<command-string> contains metacharacters, then it will be invoked
+through the system bourne shell.
+
=item B<-L>, B<--list-known>
Print a list of valid architecture names.
--
2.40.1
From be1723de4efcad748e0c7cb0d0cef7c4efd7bcb3 Mon Sep 17 00:00:00 2001
From: Paul Wise <pabs@debian.org>
Date: Tue, 4 Jul 2023 19:57:57 +0200
Subject: [PATCH 2/3] build: Avoid Perl's exec() falling back to system()
The system/exec functions sometimes execute the command as shell,
passing an indirect object as the first argument avoids that.
The shell usage happens always on Windows and on other platforms only
when there is one argument and it contains shell metacharacters.
Fixes: commit 07c81f94aa64e9b6f148c5b5cb24461708feb2b5
Ref: https://perldoc.perl.org/functions/exec.html
Signed-off-by: Guillem Jover <guillem@debian.org>
---
scripts/t/Dpkg_Shlibs/spacesyms-o-map.pl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/t/Dpkg_Shlibs/spacesyms-o-map.pl b/scripts/t/Dpkg_Shlibs/spacesyms-o-map.pl
index 89a1caf71..5081de48a 100755
--- a/scripts/t/Dpkg_Shlibs/spacesyms-o-map.pl
+++ b/scripts/t/Dpkg_Shlibs/spacesyms-o-map.pl
@@ -22,4 +22,4 @@ while (<$nm>) {
close $nm;
push @cmds, $input, $output;
-exec @cmds;
+exec { $cmds[0] } @cmds;
--
2.40.1
From 27781195debe2a10bb9b5396d70b89d5dcfcb3d6 Mon Sep 17 00:00:00 2001
From: Guillem Jover <guillem@debian.org>
Date: Tue, 4 Jul 2023 19:56:10 +0200
Subject: [PATCH 3/3] dpkg-shlibdeps: Switch from exec() to Dpkg::IPC::spawn()
Depending on the arguments the exec() builtin might fallback to invoke
the program through the system() builtin, which is not what we want.
Use the spawn() function which handles this safely, and is the common
pattern used in the entire codebase.
Prompted-by: Paul Wise <pabs@debian.org>
---
scripts/dpkg-shlibdeps.pl | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/scripts/dpkg-shlibdeps.pl b/scripts/dpkg-shlibdeps.pl
index ade77de0d..32f2dfc1d 100755
--- a/scripts/dpkg-shlibdeps.pl
+++ b/scripts/dpkg-shlibdeps.pl
@@ -32,6 +32,7 @@ use File::Basename qw(dirname);
use Dpkg ();
use Dpkg::Gettext;
use Dpkg::ErrorHandling;
+use Dpkg::IPC;
use Dpkg::Path qw(relative_to_pkg_root guess_pkg_root_dir
check_files_are_the_same get_control_path);
use Dpkg::Version;
@@ -909,17 +910,16 @@ sub find_packages {
}
return $pkgmatch unless scalar(@files);
- my $pid = open(my $dpkg_fh, '-|');
- syserr(g_('cannot fork for %s'), 'dpkg-query --search') unless defined $pid;
- if (!$pid) {
- # Child process running dpkg --search and discarding errors
- close STDERR;
- open STDERR, '>', '/dev/null'
- or syserr(g_('cannot open file %s'), '/dev/null');
- $ENV{LC_ALL} = 'C';
- exec 'dpkg-query', '--search', '--', @files
- or syserr(g_('unable to execute %s'), 'dpkg');
- }
+ # Child process running dpkg --search and discarding errors
+ my $dpkg_fh;
+ my $pid = spawn(
+ exec => [ 'dpkg-query', '--search', '--', @files ],
+ env => {
+ LC_ALL => 'C',
+ },
+ to_pipe => \$dpkg_fh,
+ error_to_file => '/dev/null',
+ );
while (<$dpkg_fh>) {
chomp;
if (m/^local diversion |^diversion by/) {
@@ -936,5 +936,7 @@ sub find_packages {
}
}
close($dpkg_fh);
+ wait_child($pid, nocheck => 1, cmdline => 'dpkg-query --search');
+
return $pkgmatch;
}
--
2.40.1
Reply to: