Perl crash due to wrong delimiter in PATH env

[p5pRT]

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

Searchable as RT94846$

[p5pRT]

From karthik.rajagopalan@schrodinger.com

Hi,

The following script access the dirTableA beyond the array limit and
might result in crash. We observed the issue at customer site where
PATH env was set with wrong delimiter in Windows and the script in
perl which they use to launch our jobs resulted in crash. Debugging
the perl code, showed the issue in vdir.h where dirTableA array was
being accessed beyond the limit. It is good to do bound check and make
perl not to crash. The attached patch fix the issue.

@​path = (

"C​:/Windows/system32",
".​:c​:/abc​:C​:/msys/1.0/scr/rajagopa/build05/Windows​:C​:/msys/1.0/scr/rajagopa/build05/Windows/utilities",

);

foreach my $dir(@​path)
{
  print $dir, "\n";
  $path = "$dir\\mount.exe";
  return $path if -x $path;
}

For second component of @​path, the index in vdir.h is -51 and
dirTableA array was being accessed with this index.

This is seen in all platforms of Windows and we observed the crash
with perl-5.10.1 of Active State. I checked the latest version and the
crash is observed even in that case.

-Karthik

[p5pRT]

From karthik.rajagopalan@schrodinger.com

0001-Restrict-the-dirTableA-access-between-0-and-drivecou.patch

From 56777d9085e995e6cc41020db088d02b444914ac Mon Sep 17 00:00:00 2001
From: Karthik Rajagopalan <rajagopa@pauline.schrodinger.com>
Date: Thu, 14 Jul 2011 13:29:26 -0400
Subject: [PATCH 1/2] Restrict the dirTableA access between 0 and
 drivecount-1.

In situation where PATH env is set with wrong delimiter say -

c:\\component1;c:\\component2;.:c:/component1:c:/component2

the code in perl which check if an executable exist in each component
of PATH env would crash. Though the delimiter need to be fixed, it is better
to avoid perl crash due to invalid access for wrong input.
---
 win32/vdir.h |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/win32/vdir.h b/win32/vdir.h
index db7ec84..2b36635 100644
--- a/win32/vdir.h
+++ b/win32/vdir.h
@@ -83,7 +83,15 @@ protected:
     };
     inline const char *GetDirA(int index)
     {
-	char *ptr = dirTableA[index];
+        char *ptr = NULL;
+
+        // Limit the access to dirTableA between
+        // 0 and driveCount-1. This can result
+        // in crash if PATH env is set with wrong
+        // delimiter otherwise - Karthik Rajagopalan
+	if(index >=0 && index < driveCount)
+	    ptr = dirTableA[index];
+
 	if (!ptr) {
 	    /* simulate the existance of this drive */
 	    ptr = szLocalBufferA;
-- 
1.7.5

[p5pRT]

karthik.rajagopalan@schrodinger.com - Status changed from 'new' to 'open'

[p5pRT]

karthik.rajagopalan@schrodinger.com - Status changed from 'open' to 'new'

[p5pRT]

From @steve-m-hay

Thanks for the patch. I haven't had a chance to try it yet, but from
inspection I would think the same check should definitely be added to
GetDirW(), and possibly similar checks should be added to
SetDefaultDirA() and SetDefaultDirW() before they assign index to
nDefault so that the array access in GetDefaultDirA() and
GetDefaultDirW() are safe too.

[p5pRT]

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

[p5pRT]

From rajagopa@schrodinger.com

Hi Shay,

On Wed Aug 10 14​:56​:18 2011, shay wrote​:

Thanks for the patch. I haven't had a chance to try it yet, but from
inspection I would think the same check should definitely be added to
GetDirW(), and possibly similar checks should be added to
SetDefaultDirA() and SetDefaultDirW() before they assign index to
nDefault so that the array access in GetDefaultDirA() and
GetDefaultDirW() are safe too.

Yes, I will submit the revised patch for it today.

-Karthik

[p5pRT]

From @cpansprout

On Thu Aug 11 05​:11​:36 2011, kartlee wrote​:

Hi Shay,

On Wed Aug 10 14​:56​:18 2011, shay wrote​:

Thanks for the patch. I haven't had a chance to try it yet, but from
inspection I would think the same check should definitely be added to
GetDirW(), and possibly similar checks should be added to
SetDefaultDirA() and SetDefaultDirW() before they assign index to
nDefault so that the array access in GetDefaultDirA() and
GetDefaultDirW() are safe too.

Yes, I will submit the revised patch for it today.

Did you ever get a chance to revise the patch?

[p5pRT]

From rajagopa@schrodinger.com

Hi,

Did you ever get a chance to revise the patch?

This really slipped out of my list. I am really sorry for the delay. I
will make the modifications and send you the patch by early tomorrow.

-Karthik

[p5pRT]

From rajagopa@schrodinger.com

Hi,

I now attached the patch for this case. Please review and let me know
your comments.

-Karthik

[p5pRT]

From rajagopa@schrodinger.com

0001-Restrict-the-dirTableA-and-dirTableW-access-between-.patch

From 32bc7a6d53758eed943047a2ab3cfc7ef4337d02 Mon Sep 17 00:00:00 2001
From: Karthik Rajagopalan <rajagopa@francesca.schrodinger.com>
Date: Mon, 31 Oct 2011 17:24:39 -0400
Subject: [PATCH] Restrict the dirTableA and dirTableW access between 0 and
 driveCount -1.

In situation where PATH env is set with wrong delimiter say -
"c:\\component1;c:\\component2;.:c:/component1:c:/component2"

the code in perl which check if an executable exist in each component
of PATH env would crash. Though the delimiter need to be fixed, it is better
to avoid perl crash due to invalid access for wrong input.
---
 win32/vdir.h |   24 ++++++++++++++++++++----
 1 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/win32/vdir.h b/win32/vdir.h
index db7ec84..4e6b615 100644
--- a/win32/vdir.h
+++ b/win32/vdir.h
@@ -83,7 +83,15 @@ protected:
     };
     inline const char *GetDirA(int index)
     {
-	char *ptr = dirTableA[index];
+        char *ptr = NULL;
+
+        // Limit the access to dirTableA between
+        // 0 and driveCount-1. This can result
+        // in crash if PATH env is set with wrong
+        // delimiter otherwise - Karthik Rajagopalan
+        if(index >=0 && index < driveCount)
+            ptr = dirTableA[index];
+
 	if (!ptr) {
 	    /* simulate the existance of this drive */
 	    ptr = szLocalBufferA;
@@ -96,7 +104,15 @@ protected:
     };
     inline const WCHAR *GetDirW(int index)
     {
-	WCHAR *ptr = dirTableW[index];
+        WCHAR *ptr = NULL;
+        
+        // Limit the access to dirTableW between
+        // 0 and driveCount-1. This can result
+        // in crash if PATH env is set with wrong
+        // delimiter otherwise - Karthik Rajagopalan
+        if(index >=0 && index < driveCount)
+            ptr = dirTableW[index];
+
 	if (!ptr) {
 	    /* simulate the existance of this drive */
 	    ptr = szLocalBufferW;
@@ -192,7 +208,7 @@ int VDir::SetDirA(char const *pPath, int index)
     char chr, *ptr;
     int length = 0;
     WCHAR wBuffer[MAX_PATH+1];
-    if (index < driveCount && pPath != NULL) {
+    if (index >= 0 && index < driveCount && pPath != NULL) {
 	length = strlen(pPath);
 	pMem->Free(dirTableA[index]);
 	ptr = dirTableA[index] = (char*)pMem->Malloc(length+2);
@@ -262,7 +278,7 @@ int VDir::SetDirW(WCHAR const *pPath, int index)
 {
     WCHAR chr, *ptr;
     int length = 0;
-    if (index < driveCount && pPath != NULL) {
+    if (index >= 0 && index < driveCount && pPath != NULL) {
 	length = wcslen(pPath);
 	pMem->Free(dirTableW[index]);
 	ptr = dirTableW[index] = (WCHAR*)pMem->Malloc((length+2)*2);
-- 
1.7.7.1

[p5pRT]

From @cpansprout

On Mon Oct 31 14​:33​:16 2011, kartlee wrote​:

Hi,

I now attached the patch for this case. Please review and let me know
your comments.

-Karthik

Steve Hay​: Could you review the new patch in this ticket?

--

Father Chrysostomos

[p5pRT]

From [Unknown Contact. See original ticket]

On Mon Oct 31 14​:33​:16 2011, kartlee wrote​:

Hi,

I now attached the patch for this case. Please review and let me know
your comments.

-Karthik

Steve Hay​: Could you review the new patch in this ticket?

--

Father Chrysostomos

[p5pRT]

From @steve-m-hay

On Mon Oct 31 14​:47​:26 2011, sprout wrote​:

On Mon Oct 31 14​:33​:16 2011, kartlee wrote​:

Hi,

I now attached the patch for this case. Please review and let me know
your comments.

-Karthik

Steve Hay​: Could you review the new patch in this ticket?

Thanks for the new patch, but I had been thinking of adding extra checks
in SetDefaultDir[AW] as well so that we don't assign an out-of-bounds
value to nDefault, otherwise it would crash when using that index value
later in GetDefaultDir[AW].

However, on closer inspection, I'm not sure that the fix is ideal anyway.

I've run your example script with several different perl builds (without
your patch) and none of them crashed for me, although stepping through
it in the debugger I can see that an index of -51 is being accessed, so
it is just flukey that it didn't crash.

Stepping through it again in the debugger with your patch, it now
correctly avoids accessing index -51, but that causes it to go into this
piece of code​:

  if (!ptr) {
  /* simulate the existance of this drive */
  ptr = szLocalBufferA;
  ptr[0] = 'A' + index;
  ptr[1] = '​:';
  ptr[2] = '\\';
  ptr[3] = 0;
  }

which produces a garbage 'drive' in the ptr array (think​: which ASCII
character is 'A' + index when index is -51?!). The code scrapes through
without crashing, but it hardly seems much less flukey than it did before.

Perhaps GetDirA should just return NULL if index is out of bounds? But
then the strcpy() in MapPathA won't be happy, so that would need some
changes too... I wonder if DriveIndex also needs some work to stop it
returning an index of -51 in the first place when the path in question
begins with '.'?!

[p5pRT]

From @rjbs

Steve, do you have any news on this?