search

Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.9k stars 540 forks source link

Perl5 Windows Command Injection Vulnerability #14434

Open p5pRT opened 9 years ago

p5pRT commented 9 years ago

Migrated from rt.perl.org#123638 (status was 'open')

Searchable as RT123638$

p5pRT commented 9 years ago

From sshbhat@gmail.com

Hello Team\,

Please find attached report for vulnerability (I Think) I've identified in Perl5 for Windows.

Hope it helps.

Regards\, Shrivathsa Bhat.

p5pRT commented 9 years ago

From sshbhat@gmail.com

Perl5_Windows_Command_Injection_Vulnerability.docx

p5pRT commented 9 years ago

From @tonycoz

On Tue Jan 20 10​:53​:18 2015\, sshbhat@​gmail.com wrote​:

Hello Team\,

Please find attached report for vulnerability (I Think) I've identified in Perl5 for Windows.

PDF version for anyone who doesn't want to deal with Word.

Tony

p5pRT commented 9 years ago

From @tonycoz

Perl5_Windows_Command_Injection_Vulnerability.pdf

p5pRT commented 9 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 9 years ago

From @iabyn

On Tue\, Jan 20\, 2015 at 02​:13​:07PM -0800\, Tony Cook via RT wrote​:

On Tue Jan 20 10​:53​:18 2015\, sshbhat@​gmail.com wrote​:

Hello Team\,

Please find attached report for vulnerability (I Think) I've identified in Perl5 for Windows.

PDF version for anyone who doesn't want to deal with Word.

and the TL;DR​: version​:

  #! usr/bin/perl   system('ECHO'\,'Hiii &""calc'); #PoC to ECHO the value   system('SET'\,'TEST'\,'Hiii &""calc'); #To set value to CMD environment variables

These both launch 'calc' apparently\, in apparent violation of the 'multiple args bypasses the shell' rule.

However\, in perlport\, it says​:

  C\ without the use of indirect object syntax (C\<system   PROGRAM LIST>) may fall back to trying the shell if the first spawn()   fails. (Win32)

Not being a win32 person\, I don't know whether that is the case here.

-- The Enterprise successfully ferries an alien VIP from one place to another without serious incident.   -- Things That Never Happen in "Star Trek" #7

p5pRT commented 9 years ago

From @tonycoz

On Tue\, Jan 20\, 2015 at 10​:52​:12PM +0000\, Dave Mitchell wrote​:

On Tue\, Jan 20\, 2015 at 02​:13​:07PM -0800\, Tony Cook via RT wrote​:

On Tue Jan 20 10​:53​:18 2015\, sshbhat@​gmail.com wrote​:

Hello Team\,

Please find attached report for vulnerability (I Think) I've identified in Perl5 for Windows.

PDF version for anyone who doesn't want to deal with Word.

and the TL;DR​: version​:

\#\! usr/bin/perl
system\('ECHO'\,'Hiii &""calc'\); \#PoC to ECHO the value
system\('SET'\,'TEST'\,'Hiii &""calc'\); \#To set value to CMD environment variables

These both launch 'calc' apparently\, in apparent violation of the 'multiple args bypasses the shell' rule.

This is a problem for pipe open too.

However\, in perlport\, it says​:

C\<system LIST> without the use of indirect object syntax \(C\<system
PROGRAM LIST>\) may fall back to trying the shell if the first spawn\(\)
fails\.  \(Win32\)

Not being a win32 person\, I don't know whether that is the case here.

Yes\, that falls back to the shell\, both echo and set are cmd.exe built-ins\, but we might be able to improve on how we deal with the shell.

win32.c already has code that converts the array into a single string (create_command_line())\, which attempts to deal with cmd.exe's idiosyncrasies.

I'm working on an improvement that escapes specials like & and | with cmd.exe's escape character "^".

I'll post a patch once it's ready.

Tony

p5pRT commented 9 years ago

From sshbhat@gmail.com

Thank you\,

I thought same\, When system() is giving syntax like system($command\, @​arguments); Then\, it would be better if not execute command otherthan $command.

Anyway I'll go through perlport again to get better way to use shell commands and meanwhile I'll keep my eyes open for this patch.

Regards\, Shrivathsa Bhat. On Jan 21\, 2015 5​:59 AM\, "Tony Cook via RT" \perl5\-security\-report@&#8203;perl\.org wrote​:

On Tue\, Jan 20\, 2015 at 10​:52​:12PM +0000\, Dave Mitchell wrote​:

On Tue\, Jan 20\, 2015 at 02​:13​:07PM -0800\, Tony Cook via RT wrote​:

On Tue Jan 20 10​:53​:18 2015\, sshbhat@​gmail.com wrote​:

Hello Team\,

Please find attached report for vulnerability (I Think) I've identified in Perl5 for Windows.

PDF version for anyone who doesn't want to deal with Word.

and the TL;DR​: version​:

\#\! usr/bin/perl
system\('ECHO'\,'Hiii &""calc'\); \#PoC to ECHO the value
system\('SET'\,'TEST'\,'Hiii &""calc'\); \#To set value to CMD

environment variables

These both launch 'calc' apparently\, in apparent violation of the 'multiple args bypasses the shell' rule.

This is a problem for pipe open too.

However\, in perlport\, it says​:

C\<system LIST> without the use of indirect object syntax \(C\<system
PROGRAM LIST>\) may fall back to trying the shell if the first spawn\(\)
fails\.  \(Win32\)

Not being a win32 person\, I don't know whether that is the case here.

Yes\, that falls back to the shell\, both echo and set are cmd.exe built-ins\, but we might be able to improve on how we deal with the shell.

win32.c already has code that converts the array into a single string (create_command_line())\, which attempts to deal with cmd.exe's idiosyncrasies.

I'm working on an improvement that escapes specials like & and | with cmd.exe's escape character "^".

I'll post a patch once it's ready.

Tony

p5pRT commented 9 years ago

From @tonycoz

On Thu Jan 22 01​:53​:05 2015\, sshbhat@​gmail.com wrote​:

Thank you\,

I thought same\, When system() is giving syntax like system($command\, @​arguments); Then\, it would be better if not execute command otherthan $command.

Anyway I'll go through perlport again to get better way to use shell commands and meanwhile I'll keep my eyes open for this patch.

I've spent some time on this\, at this point I don't think it's possiblle to escape shell-builtins without breaking other parts of the perl eco-system.

The only real fix would be to disable the fallback to the shell\, which would make the behaviour closer to POSIX systems.

It might be worth removing that behaviour from the multi-arg pipe open I added before 5.22 is released.

Tony

p5pRT commented 9 years ago

From @rjbs

* Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org [2015-02-03T22​:17​:57]

It might be worth removing that behaviour from the multi-arg pipe open I added before 5.22 is released.

That's probably worth doing.

I think any further changes should probably wait until 5.23\, but I'm not sure we can safely make any changes to the default behavior. Possibly with a pragma. Blah. :(

-- rjbs

p5pRT commented 9 years ago

From @Leont

On Wed\, Feb 4\, 2015 at 4​:17 AM\, Tony Cook via RT \< perl5-security-report@​perl.org> wrote​:

I've spent some time on this\, at this point I don't think it's possiblle to escape shell-builtins without breaking other parts of the perl eco-system.

I suspect the same. I've long considered system() on Win32 to be broken (in ways that I don't even want to grok)\, but Win32​::ShellQuote is my friend that manages to fix things up. I'd prefer not needing it though.

The only real fix would be to disable the fallback to the shell\, which would make the behaviour closer to POSIX systems.

That sounds rather sensible.

It might be worth removing that behaviour from the multi-arg pipe open I added before 5.22 is released.

Yes\, that would be nice.

Leon

p5pRT commented 9 years ago

From @tonycoz

On Wed Feb 04 07​:57​:55 2015\, LeonT wrote​:

On Wed\, Feb 4\, 2015 at 4​:17 AM\, Tony Cook via RT \< perl5-security-report@​perl.org> wrote​:

I've spent some time on this\, at this point I don't think it's possiblle to escape shell-builtins without breaking other parts of the perl eco-system.

I suspect the same. I've long considered system() on Win32 to be broken (in ways that I don't even want to grok)\, but Win32​::ShellQuote is my friend that manages to fix things up. I'd prefer not needing it though.

The only real fix would be to disable the fallback to the shell\, which would make the behaviour closer to POSIX systems.

That sounds rather sensible.

It might be worth removing that behaviour from the multi-arg pipe open I added before 5.22 is released.

Yes\, that would be nice.

Patch attached. I'll apply it next week unless someone objects.

Tony

p5pRT commented 9 years ago

From @tonycoz

0001-remove-the-shell-fallback-from-list-pipe-open-on-Win.patch ```diff From fea6a1922b8e30d706dc79a92d010133d68e4ebc Mon Sep 17 00:00:00 2001 From: Tony Cook Date: Thu, 5 Feb 2015 16:36:55 +1100 Subject: [PATCH] remove the shell fallback from list pipe open on Win32 Without the shell, CreateProcess() won't pick up .cmd or .bat files, which several perl utilities are packaged as on Win32, so use qualify_path() to produce a full executable name. --- pod/perlport.pod | 3 --- t/win32/system_tests | 4 ++-- win32/win32.c | 44 +++++++++++++++++++++++++++----------------- 3 files changed, 29 insertions(+), 22 deletions(-) diff --git a/pod/perlport.pod b/pod/perlport.pod index 3bb10e3..62443d6 100644 --- a/pod/perlport.pod +++ b/pod/perlport.pod @@ -1874,9 +1874,6 @@ Not implemented. (Android, Win32, VMS, S, S, VOS) open to C<|-> and C<-|> are unsupported. (Win32, S) -List-form pipe opens may fall back to the shell if the first spawn() -fails. (Win32) - Opening a process does not automatically flush output handles on some platforms. (SunOS, Solaris, HP-UX) diff --git a/t/win32/system_tests b/t/win32/system_tests index 8307222..91113db 100644 --- a/t/win32/system_tests +++ b/t/win32/system_tests @@ -121,12 +121,12 @@ for my $cmds (@commands) { $^D = 0; note "# pipe [".join(";", @cmds, @args). "]"; - if (open my $io, "|-", @cmds, @args) { + if (open my $io, "-|", @cmds, @args) { print <$io>; close $io; } else { - print "Failed pipe open: $!\n"; + print STDERR "Failed pipe open [",join(";", @cmds, @args),"]: $!\n"; } } } diff --git a/win32/win32.c b/win32/win32.c index ebb2b19..a78c598 100644 --- a/win32/win32.c +++ b/win32/win32.c @@ -149,7 +149,7 @@ static long filetime_to_clock(PFILETIME ft); static BOOL filetime_from_time(PFILETIME ft, time_t t); static char* create_command_line(char *cname, STRLEN clen, const char * const *args); -static char* qualified_path(const char *cmd); +static char* qualified_path(const char *cmd, bool other_exts); static void ansify_path(void); static LRESULT win32_process_message(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam); @@ -3009,23 +3009,20 @@ do_popen(const char *mode, const char *command, IV narg, SV **args) { } else { int i; + const char *exe_name; Newx(args_pvs, narg + 1 + w32_perlshell_items, const char *); SAVEFREEPV(args_pvs); for (i = 0; i < narg; ++i) args_pvs[i] = SvPV_nolen(args[i]); args_pvs[i] = NULL; + exe_name = qualified_path(args_pvs[0], TRUE); + if (!exe_name) + /* let CreateProcess() try to find it instead */ + exe_name = args_pvs[0]; - if ((childpid = do_spawnvp_handles(P_NOWAIT, args_pvs[0], args_pvs, handles)) == -1) { - if (errno == ENOEXEC || errno == ENOENT) { - /* possible shell-builtin, invoke with shell */ - Move(args_pvs, args_pvs+w32_perlshell_items, narg+1, const char *); - Copy(w32_perlshell_vec, args_pvs, w32_perlshell_items, const char *); - if ((childpid = do_spawnvp_handles(P_NOWAIT, args_pvs[0], args_pvs, handles)) == -1) - goto cleanup; - } - else - goto cleanup; + if ((childpid = do_spawnvp_handles(P_NOWAIT, exe_name, args_pvs, handles)) == -1) { + goto cleanup; } } @@ -3550,8 +3547,15 @@ create_command_line(char *cname, STRLEN clen, const char * const *args) return cmd; } +static const char *exe_extensions[] = + { + ".exe", /* this must be first */ + ".cmd", + ".bat" + }; + static char * -qualified_path(const char *cmd) +qualified_path(const char *cmd, bool other_exts) { char *pathstr; char *fullcmd, *curfullcmd; @@ -3590,10 +3594,16 @@ qualified_path(const char *cmd) if (cmd[cmdlen-1] != '.' && (cmdlen < 4 || cmd[cmdlen-4] != '.')) { - strcpy(curfullcmd, ".exe"); - res = GetFileAttributes(fullcmd); - if (res != 0xFFFFFFFF && !(res & FILE_ATTRIBUTE_DIRECTORY)) - return fullcmd; + int i; + /* first extension is .exe */ + int ext_limit = other_exts ? C_ARRAY_LENGTH(exe_extensions) : 1; + for (i = 0; i < ext_limit; ++i) { + strcpy(curfullcmd, exe_extensions[i]); + res = GetFileAttributes(fullcmd); + if (res != 0xFFFFFFFF && !(res & FILE_ATTRIBUTE_DIRECTORY)) + return fullcmd; + } + *curfullcmd = '\0'; } @@ -3830,7 +3840,7 @@ RETRY: * jump through our own hoops by picking out the path * we really want it to use. */ if (!fullcmd) { - fullcmd = qualified_path(cname); + fullcmd = qualified_path(cname, FALSE); if (fullcmd) { if (cname != cmdname) Safefree(cname); -- 1.9.5.msysgit.0 ```
p5pRT commented 9 years ago

From @tonycoz

On Wed Feb 04 21​:39​:04 2015\, tonyc wrote​:

Patch attached. I'll apply it next week unless someone objects.

Applied as f5fe1b19fc61e51b533b25b128ec7a6f764372da with an updated comment.

This does *not* fix the original issue.

I think we'd need to go through a deprecation cycle to fix that by removing the fallback.

Tony

p5pRT commented 7 years ago

From @tonycoz

On Tue Feb 24 01​:09​:44 2015\, tonyc wrote​:

On Wed Feb 04 21​:39​:04 2015\, tonyc wrote​:

Patch attached. I'll apply it next week unless someone objects.

Applied as f5fe1b19fc61e51b533b25b128ec7a6f764372da with an updated comment.

This does *not* fix the original issue.

I think we'd need to go through a deprecation cycle to fix that by removing the fallback.

Here's a deprecation patch.

Tony

p5pRT commented 7 years ago

From @tonycoz

0001-perl-123638-deprecate-fallback-to-shell-on-Win32.patch ```diff From 6e789b3df95e0c5c29a9b84cef8d28c28f349001 Mon Sep 17 00:00:00 2001 From: Tony Cook Date: Wed, 2 Nov 2016 09:51:20 +1100 Subject: [PATCH] (perl #123638) deprecate fallback to shell on Win32 --- MANIFEST | 1 + pod/perldiag.pod | 12 ++++++++++++ t/lib/warnings/win32 | 7 +++++++ win32/win32.c | 4 ++++ 4 files changed, 24 insertions(+) create mode 100644 t/lib/warnings/win32 diff --git a/MANIFEST b/MANIFEST index d2dfa4c..514fa3d 100644 --- a/MANIFEST +++ b/MANIFEST @@ -5333,6 +5333,7 @@ t/lib/warnings/toke Tests for toke.c for warnings.t t/lib/warnings/universal Tests for universal.c for warnings.t t/lib/warnings/utf8 Tests for utf8.c for warnings.t t/lib/warnings/util Tests for util.c for warnings.t +t/lib/warnings/win32 Tests for win32/win32.c for warnings.t t/loc_tools.pl Common functions for finding platform's locales t/mro/basic.t mro tests t/mro/basic_01_c3.t mro tests diff --git a/pod/perldiag.pod b/pod/perldiag.pod index 2e3496f..463b179 100644 --- a/pod/perldiag.pod +++ b/pod/perldiag.pod @@ -5883,6 +5883,18 @@ notifies you that it is giving up trying. machine. In some machines the functionality can exist but be unconfigured. Consult your system support. +=item system(LIST) fallback to shell deprecated + +(W deprecated) C on Win32 currently falls back to the +shell if the supplied program name cannot be found. + +The Win32 API to create a new process only accepts a single string for +the process arguments, so to emulate the behaviour on POSIX systems, +perl quotes the arguments in the same way as most software on Win32. + +Unfortunately the Win32 shell accepts arguments escaped in a different +way to most other Win32 software, which can be used by an attacker. + =item syswrite() on closed filehandle %s (W closed) The filehandle you're writing to got itself closed sometime diff --git a/t/lib/warnings/win32 b/t/lib/warnings/win32 new file mode 100644 index 0000000..608fd99 --- /dev/null +++ b/t/lib/warnings/win32 @@ -0,0 +1,7 @@ +__END__ +# win32.c +# NAME system(LIST) fallback +# SKIP ?$^O ne "MSWin32" +system("echo", "foo"); +EXPECT +foo diff --git a/win32/win32.c b/win32/win32.c index 6ac73e2..6412f00 100644 --- a/win32/win32.c +++ b/win32/win32.c @@ -657,6 +657,10 @@ Perl_do_aspawn(pTHX_ SV *really, SV **mark, SV **sp) status = win32_spawnvp(flag, (const char*)(really ? SvPV_nolen(really) : argv[0]), (const char* const*)argv); + if (status >= 0 && ckWARN(WARN_DEPRECATED)) { + Perl_warner(aTHX_ packWARN(WARN_DEPRECATED), + "system(LIST) fallback to shell deprecated"); + } } if (flag == P_NOWAIT) { -- 2.7.0.windows.1 ```
p5pRT commented 7 years ago

From @iabyn

On Tue\, Nov 01\, 2016 at 03​:55​:45PM -0700\, Tony Cook via RT wrote​:

On Tue Feb 24 01​:09​:44 2015\, tonyc wrote​:

On Wed Feb 04 21​:39​:04 2015\, tonyc wrote​:

Patch attached. I'll apply it next week unless someone objects.

Applied as f5fe1b19fc61e51b533b25b128ec7a6f764372da with an updated comment.

This does *not* fix the original issue.

I think we'd need to go through a deprecation cycle to fix that by removing the fallback.

Here's a deprecation patch.

Tony

From 6e789b3df95e0c5c29a9b84cef8d28c28f349001 Mon Sep 17 00​:00​:00 2001 From​: Tony Cook \tony@&#8203;develop\-help\.com Date​: Wed\, 2 Nov 2016 09​:51​:20 +1100 Subject​: [PATCH] (perl #123638) deprecate fallback to shell on Win32 [snip] +=item system(LIST) fallback to shell deprecated

What's the current status of this patch and this ticket?

-- You never really learn to swear until you learn to drive.

p5pRT commented 7 years ago

From @tonycoz

On Thu\, 02 Feb 2017 05​:55​:52 -0800\, davem wrote​:

On Tue\, Nov 01\, 2016 at 03​:55​:45PM -0700\, Tony Cook via RT wrote​:

On Tue Feb 24 01​:09​:44 2015\, tonyc wrote​:

On Wed Feb 04 21​:39​:04 2015\, tonyc wrote​:

Patch attached. I'll apply it next week unless someone objects.

Applied as f5fe1b19fc61e51b533b25b128ec7a6f764372da with an updated comment.

This does *not* fix the original issue.

I think we'd need to go through a deprecation cycle to fix that by removing the fallback.

Here's a deprecation patch.

Tony

From 6e789b3df95e0c5c29a9b84cef8d28c28f349001 Mon Sep 17 00​:00​:00 2001 From​: Tony Cook \tony@&#8203;develop\-help\.com Date​: Wed\, 2 Nov 2016 09​:51​:20 +1100 Subject​: [PATCH] (perl #123638) deprecate fallback to shell on Win32 [snip] +=item system(LIST) fallback to shell deprecated

What's the current status of this patch and this ticket?

Do we deprecate system(LIST) falling back to the shell or not?

Tony

p5pRT commented 7 years ago

From @xsawyerx

On Mon\, Feb 20\, 2017 at 12​:54 AM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

On Thu\, 02 Feb 2017 05​:55​:52 -0800\, davem wrote​:

On Tue\, Nov 01\, 2016 at 03​:55​:45PM -0700\, Tony Cook via RT wrote​:

On Tue Feb 24 01​:09​:44 2015\, tonyc wrote​:

On Wed Feb 04 21​:39​:04 2015\, tonyc wrote​:

Patch attached. I'll apply it next week unless someone objects.

Applied as f5fe1b19fc61e51b533b25b128ec7a6f764372da with an updated comment.

This does *not* fix the original issue.

I think we'd need to go through a deprecation cycle to fix that by removing the fallback.

Here's a deprecation patch.

Tony

From 6e789b3df95e0c5c29a9b84cef8d28c28f349001 Mon Sep 17 00​:00​:00 2001 From​: Tony Cook \tony@&#8203;develop\-help\.com Date​: Wed\, 2 Nov 2016 09​:51​:20 +1100 Subject​: [PATCH] (perl #123638) deprecate fallback to shell on Win32 [snip] +=item system(LIST) fallback to shell deprecated

What's the current status of this patch and this ticket?

Do we deprecate system(LIST) falling back to the shell or not?

I thought this is what we decided. No?

p5pRT commented 7 years ago

From @iabyn

On Mon\, Feb 20\, 2017 at 02​:56​:46PM +0200\, Sawyer X wrote​:

On Mon\, Feb 20\, 2017 at 12​:54 AM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

On Thu\, 02 Feb 2017 05​:55​:52 -0800\, davem wrote​:

On Tue\, Nov 01\, 2016 at 03​:55​:45PM -0700\, Tony Cook via RT wrote​:

On Tue Feb 24 01​:09​:44 2015\, tonyc wrote​:

On Wed Feb 04 21​:39​:04 2015\, tonyc wrote​:

Patch attached. I'll apply it next week unless someone objects.

Applied as f5fe1b19fc61e51b533b25b128ec7a6f764372da with an updated comment.

This does *not* fix the original issue.

I think we'd need to go through a deprecation cycle to fix that by removing the fallback.

Here's a deprecation patch.

Tony

From 6e789b3df95e0c5c29a9b84cef8d28c28f349001 Mon Sep 17 00​:00​:00 2001 From​: Tony Cook \tony@&#8203;develop\-help\.com Date​: Wed\, 2 Nov 2016 09​:51​:20 +1100 Subject​: [PATCH] (perl #123638) deprecate fallback to shell on Win32 [snip] +=item system(LIST) fallback to shell deprecated

What's the current status of this patch and this ticket?

Do we deprecate system(LIST) falling back to the shell or not?

I thought this is what we decided. No?

I don't know. There hasn't been much discussion\, pro or con\, in this ticket. Given that once the deprecation is applied to blead the issue effectively becomes public without being immediately fixed\, I suggest we might as well move this ticket to the public queue and open up the deprecation warning proposal to wider discussion.

-- Never work with children\, animals\, or actors.

p5pRT commented 7 years ago

From @xsawyerx

On Mon\, Mar 13\, 2017 at 2​:27 PM\, Dave Mitchell \davem@&#8203;iabyn\.com wrote​:

On Mon\, Feb 20\, 2017 at 02​:56​:46PM +0200\, Sawyer X wrote​:

On Mon\, Feb 20\, 2017 at 12​:54 AM\, Tony Cook via RT \perl5\-security\-report@&#8203;perl\.org wrote​:

On Thu\, 02 Feb 2017 05​:55​:52 -0800\, davem wrote​:

On Tue\, Nov 01\, 2016 at 03​:55​:45PM -0700\, Tony Cook via RT wrote​:

On Tue Feb 24 01​:09​:44 2015\, tonyc wrote​:

On Wed Feb 04 21​:39​:04 2015\, tonyc wrote​:

Patch attached. I'll apply it next week unless someone objects.

Applied as f5fe1b19fc61e51b533b25b128ec7a6f764372da with an updated comment.

This does *not* fix the original issue.

I think we'd need to go through a deprecation cycle to fix that by removing the fallback.

Here's a deprecation patch.

Tony

From 6e789b3df95e0c5c29a9b84cef8d28c28f349001 Mon Sep 17 00​:00​:00 2001 From​: Tony Cook \tony@&#8203;develop\-help\.com Date​: Wed\, 2 Nov 2016 09​:51​:20 +1100 Subject​: [PATCH] (perl #123638) deprecate fallback to shell on Win32 [snip] +=item system(LIST) fallback to shell deprecated

What's the current status of this patch and this ticket?

Do we deprecate system(LIST) falling back to the shell or not?

I thought this is what we decided. No?

I don't know. There hasn't been much discussion\, pro or con\, in this ticket. Given that once the deprecation is applied to blead the issue effectively becomes public without being immediately fixed\, I suggest we might as well move this ticket to the public queue and open up the deprecation warning proposal to wider discussion.

Let's do that.

p5pRT commented 7 years ago

From @iabyn

I've just moved this ticket to the public queue. The remaining issue to be discussed is for win32 system()​: it will sometimes fallback to using the shell\, even with individual arguments. It's been proposed that this behaviour should be deprecated​:

+=item system(LIST) fallback to shell deprecated + +(W deprecated) C\<system(LIST)> on Win32 currently falls back to the +shell if the supplied program name cannot be found. + +The Win32 API to create a new process only accepts a single string for +the process arguments\, so to emulate the behaviour on POSIX systems\, +perl quotes the arguments in the same way as most software on Win32. + +Unfortunately the Win32 shell accepts arguments escaped in a different +way to most other Win32 software\, which can be used by an attacker.

Can anyone see a reason not to do this? [NB​: I am just the messenger; I have no opinion on this]

-- Dave's first rule of Opera​: If something needs saying\, say it​: don't warble it.

p5pRT commented 7 years ago

From @bulk88

On Mon\, 20 Mar 2017 02​:37​:31 -0700\, davem wrote​:

I've just moved this ticket to the public queue. The remaining issue to be discussed is for win32 system()​: it will sometimes fallback to using the shell\, even with individual arguments. It's been proposed that this behaviour should be deprecated​:

+=item system(LIST) fallback to shell deprecated + +(W deprecated) C\<system(LIST)> on Win32 currently falls back to the +shell if the supplied program name cannot be found. + +The Win32 API to create a new process only accepts a single string for +the process arguments\, so to emulate the behaviour on POSIX systems\, +perl quotes the arguments in the same way as most software on Win32. + +Unfortunately the Win32 shell accepts arguments escaped in a different +way to most other Win32 software\, which can be used by an attacker.

Can anyone see a reason not to do this? [NB​: I am just the messenger; I have no opinion on this]

This code in miniperl uses shell execute to run a builtin on Win32.

https://perl5.git.perl.org/perl.git/blob/28118845adbde9f823d609bb19abbbf8d1ffee47:/dist/PathTools/Cwd.pm#l625

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 7 years ago

From @tonycoz

On Thu\, Mar 30\, 2017 at 04​:29​:58PM -0700\, bulk88 via RT wrote​:

On Mon\, 20 Mar 2017 02​:37​:31 -0700\, davem wrote​:

I've just moved this ticket to the public queue. The remaining issue to be discussed is for win32 system()​: it will sometimes fallback to using the shell\, even with individual arguments. It's been proposed that this behaviour should be deprecated​:

+=item system(LIST) fallback to shell deprecated + +(W deprecated) C\<system(LIST)> on Win32 currently falls back to the +shell if the supplied program name cannot be found. + +The Win32 API to create a new process only accepts a single string for +the process arguments\, so to emulate the behaviour on POSIX systems\, +perl quotes the arguments in the same way as most software on Win32. + +Unfortunately the Win32 shell accepts arguments escaped in a different +way to most other Win32 software\, which can be used by an attacker.

Can anyone see a reason not to do this? [NB​: I am just the messenger; I have no opinion on this]

This code in miniperl uses shell execute to run a builtin on Win32.

https://perl5.git.perl.org/perl.git/blob/28118845adbde9f823d609bb19abbbf8d1ffee47:/dist/PathTools/Cwd.pm#l625

That code doesn't use the list form of system() (or even system() at all).

Tony

lightsey commented 4 years ago

Shortly after this issue was made public in RT, a related issue was submitted to the Perl security team by Noah Misch. This new issue was given the RT ticket number 131065 and became Perl/perl5-security#83 after the github migration.

Noah's report covered several different problems, but most of the discussion focused on a variation of this bug that stems from the way that Perl translates array argv parameters for subprocesses into the single CommandLine argument that Windows accepts for the CreateProcess() function and the default logic Windows uses to parse the stringified CommandLine back into an argv array.

This #14434 issue shows that Perl is quoting arguments incorrectly for cmd.exe, and the Perl/perl5-security#83 issue showed that a very similar problem applies when cmd.exe is not involved in spawning the subprocess.

This example code demonstrates the faulty behavior when cmd.exe is not involved:

arg-injection.pl ```perl #! /usr/bin/perl use strict; use warnings; use Config; # envision a real program where this argument comes from an untrusted source my $arg; $ENV{PATH} = '/usr/bin' if ${^TAINT}; my $perl = $Config{perlpath}; $perl .= $Config{_exe} unless $perl =~ /$Config{_exe}$/i; # CreateProcess gets: C:\STRAWB~1\perl\bin\perl.exe -V:"u32size" "-Mnosuch_example" # => Can't locate nosuch_example.pm in @INC (you may need to install the nosuch_example module) (@INC contains: C:/Strawberry/perl/site/lib C:/Strawberry/perl/vendor/lib C:/Strawberry/perl/lib .). $arg = '"u32size" "-Mnosuch_example"'; system {$perl} $perl, "-V:$arg"; ```

Like the cmd.exe argument quoting problems, this additional variation is impossible to fix without breaking existing Perl code that uses Win32::ShellQuote to work around the current behavior.

@tonycoz proposed the following patch that adds a toggle to enables correct argument passing in both scenarios.

0001-perl-131064-work-in-progress.patch ```diff From 6660e463b4212fc5db6a0145c122e9566c501b21 Mon Sep 17 00:00:00 2001 From: Tony Cook Date: Wed, 16 Aug 2017 15:58:33 +1000 Subject: (perl #131064) work in progress needs more tests, more eyes, perhaps a better mechanism to switch to new escaping --- embed.fnc | 3 + embed.h | 3 + ext/XS-APItest/APItest.pm | 2 +- ext/XS-APItest/APItest.xs | 21 +++++ ext/XS-APItest/t/win32.t | 73 ++++++++++++++++- makedef.pl | 1 + proto.h | 7 ++ win32/win32.c | 199 +++++++++++++++++++++++++++++++++++++++++++++- 8 files changed, 305 insertions(+), 4 deletions(-) diff --git a/embed.fnc b/embed.fnc index 2dd73bf..7d20694 100644 --- a/embed.fnc +++ b/embed.fnc @@ -433,6 +433,9 @@ Ap |int |do_aspawn |NULLOK SV* really|NN SV** mark|NN SV** sp Ap |int |do_spawn |NN char* cmd Ap |int |do_spawn_nowait|NN char* cmd #endif +#if defined(WIN32) && (defined(PERL_CORE) || defined(PERL_EXT)) +pMnER |char * |quote_command_line|NULLOK const char *cname|NN const char * const *args +#endif #if !defined(WIN32) p |bool|do_exec3 |NN const char *incmd|int fd|int do_report #endif diff --git a/embed.h b/embed.h index a41020d..f0799b8 100644 --- a/embed.h +++ b/embed.h @@ -1145,6 +1145,9 @@ # if defined(PERL_IN_UTF8_C) || defined(PERL_IN_REGCOMP_C) || defined(PERL_IN_REGEXEC_C) #define _to_fold_latin1(a,b,c,d) Perl__to_fold_latin1(aTHX_ a,b,c,d) # endif +# if defined(WIN32) && (defined(PERL_CORE) || defined(PERL_EXT)) +#define quote_command_line Perl_quote_command_line +# endif #endif #ifdef PERL_CORE #define Slab_Alloc(a) Perl_Slab_Alloc(aTHX_ a) diff --git a/ext/XS-APItest/APItest.pm b/ext/XS-APItest/APItest.pm index 7de08ad..796605f 100644 --- a/ext/XS-APItest/APItest.pm +++ b/ext/XS-APItest/APItest.pm @@ -5,7 +5,7 @@ use strict; use warnings; use Carp; -our $VERSION = '0.90'; +our $VERSION = '0.91'; require XSLoader; diff --git a/ext/XS-APItest/APItest.xs b/ext/XS-APItest/APItest.xs index 23e698c..b1c5abf 100644 --- a/ext/XS-APItest/APItest.xs +++ b/ext/XS-APItest/APItest.xs @@ -6579,6 +6579,27 @@ Comctl32Version() mPUSHi(dwValueLS2); } +SV * +quote_command_line(SV *cname_sv, ...) + PREINIT: + const char *cname; + char ** args; + int i; + char *result; + CODE: + SvGETMAGIC(cname_sv); + cname = SvOK(cname_sv) ? SvPV_nolen(cname_sv) : NULL; + /* items less one for cname_sv, plus one for final NULL */ + Newx(args, items, char *); + for (i = 0; i < items-1; ++i) + args[i] = SvPV_nolen(ST(i+1)); + args[i] = NULL; + result = quote_command_line(cname, (const char * const *)args); + RETVAL = result ? newSVpv(result, 0) : &PL_sv_undef; + Safefree(result); + OUTPUT: + RETVAL + #endif diff --git a/ext/XS-APItest/t/win32.t b/ext/XS-APItest/t/win32.t index fdd794a..83b9ff0 100644 --- a/ext/XS-APItest/t/win32.t +++ b/ext/XS-APItest/t/win32.t @@ -9,7 +9,7 @@ plan skip_all => "Tests only apply on MSWin32" SKIP: { - # [perl #126755] previous the bad drive tests would crash + # [perl #126755] previously the bad drive tests would crash $Config{ccflags} =~ /(?:\A|\s)-DPERL_IMPLICIT_SYS\b/ or skip "need implicit_sys for this test", 1; eval "use Encode; 1" @@ -45,4 +45,75 @@ SKIP: } } +{ + my %cmap = ( n => "\n", t => "\t", v => "\x0b", r => "\r" ); + # test that we escape command-line arguments correctly + while () { + next if /^#/ || !/\S/; + chomp; + # ~n etc to \n etc + s/~([nrvt])/$cmap{$1}/g; + my ($cname, $args, $expect, $out, $name) = split /;/; + $cname eq '$^X' and $cname = $^X; + my @args = split /,/, $args; + for my $arg (@args) { + $arg eq '$^X' and $arg = $^X; + } + $cname eq '-' and undef $cname; + $expect eq '-' and undef $expect; + my $result = quote_command_line($cname, @args); + $expect =~ s/\$\^X/$^X/g; + is($result, $expect, $name); + unless ($out eq '-') { + # run it + my $got; + local ${^WIN32_SAFER_ARGV_ESCAPE} = 1; + if (open my $fh, "-|", @args) { + $got = do { local $/; <$fh> }; + close $fh; + } + else { + $got = "cannot open: $!"; + } + is($got, $out, "$name - output"); + } + } +} + done_testing(); + +__DATA__ +# fields are, separated by ; +# cname - command name +# args - command args, separated by commas +# expected - expected escaped string +# output - expected output of running commant +# name - test name +# cname and expected of "-" is treated as undef. +# ~n, ~t, ~v, ~r are converted to \n, \t, \v, \r respectively +# a cname, arg, or string in expected of '$^X' is replaced with $^X +# +# simple cmd.exe escaping +cmd.exe;cmd.exe,/c,echo,>foo;cmd.exe /c echo ^"^>foo^";-;simple cmd.exe +-;cmd.exe,/c,echo,': + case '&': + case '|': + if (is_cmd) { + /* count ^ quote it for cmd */ + ++len; + } + break; + } + if (c == '\\') + ++bs_count; + else + bs_count = 0; + } + len += bs_count; + + return len; + } + else { + return strlen(arg); + } +} + +static STRLEN +quote_cmd_entry(char *out, const char *arg, bool is_cmd) { + if (*arg == 0 + || strpbrk(arg, is_cmd ? cmd_escapables : command_escapables)) { + const char *argp = arg; + char *outp = out; + U8 c; + int bs_count = 0; + + if (is_cmd) + *outp++ = '^'; + *outp++ = '"'; + while (c = (U8)*argp++) { + switch (c) { + case '"': + for (; bs_count > 0; --bs_count) { + *outp++ = '\\'; + } + *outp++ = '\\'; + if (is_cmd) + *outp++ = '^'; + break; + + case '\n': + case '\r': + if (is_cmd) { + /* we only get here if the caller or + * calc_cmd_entry_space() is broken + */ + assert(0); + /* these can't be quoted for cmd */ + errno = EINVAL; + return 0; + } + break; + case '(': + case ')': + case '%': + case '!': + case '^': + case '<': + case '>': + case '&': + case '|': + if (is_cmd) + *outp++ = '^'; + break; + } + if (c == '\\') + ++bs_count; + else + bs_count = 0; + *outp++ = c; + } + for (; bs_count > 0; --bs_count) + *outp++ = '\\'; + if (is_cmd) + *outp++ = '^'; + *outp++ = '"'; + *outp = 0; + + /*fprintf(stderr, "\n\narg %s\nout %s\nlen %d\ngotlen %d\n\n", arg, out, + (int)calc_cmd_entry_space(arg, is_cmd), (int)(outp-out)); */ + assert((outp - out) == calc_cmd_entry_space(arg, is_cmd)); + return outp - out; + } + else { + strcpy(out, arg); + return strlen(arg); + } +} + +char * +Perl_quote_command_line(const char *cname, const char * const *args) { + dTHX; + bool is_cmd = 0; + int backslash_count = 0; + STRLEN total_size = 0; + const char * const * argp; + const char *bs, *fs, *exe; + char *out, *outp; + + PERL_ARGS_ASSERT_QUOTE_COMMAND_LINE; + + if (!cname) + cname = args[0]; + + if (!cname) { + errno = EINVAL; + return NULL; + } + + bs = strrchr(cname, '\\'); + fs = strrchr(cname, '//'); + if (bs) { + exe = fs ? ( bs > fs ? bs : fs ) : bs; + } + else if (fs) { + exe = fs; + } + else + exe = cname; + is_cmd = stricmp(exe, "cmd.exe") == 0 || stricmp(exe, "cmd") == 0; + + total_size = calc_cmd_entry_space(cname, FALSE); + + for (argp = args + 1; *argp; ++argp) { + STRLEN entry_len = calc_cmd_entry_space(*argp, is_cmd); + if (entry_len == 0) + return NULL; + total_size += 1 + entry_len; + } + + Newx(out, total_size + 1, char); + outp = out; + outp += quote_cmd_entry(outp, args[0], FALSE); + for (argp = args + 1; *argp; ++argp) { + *outp++ = ' '; + outp += quote_cmd_entry(outp, *argp, is_cmd); + assert(!*outp); + } + + return out; +} static char * create_command_line(char *cname, STRLEN clen, const char * const *args) @@ -3754,7 +3946,7 @@ win32_spawnvp(int mode, const char *cmdname, const char *const *argv) static int do_spawnvp_handles(int mode, const char *cmdname, const char *const *argv, const int *handles) { - dTHXa(NULL); + dTHX; int ret; void* env; char* dir; @@ -3785,7 +3977,10 @@ do_spawnvp_handles(int mode, const char *cmdname, const char *const *argv, } } - cmd = create_command_line(cname, clen, argv); + if (SvTRUE(get_sv("\x17IN32_SAFER_ARGV_ESCAPE", GV_ADD))) + cmd = quote_command_line(cname, argv); + else + cmd = create_command_line(cname, clen, argv); aTHXa(PERL_GET_THX); env = PerlEnv_get_childenv(); -- 2.7.0.windows.1 ```
xenu commented 4 years ago

This issue is a duplicate of #8961 and #13190