Perl 5.22 VDir::MapPathA/W Out-of-bounds Reads and Buffer Over-reads

How it was found

I have reported multiple vulnerabilities in Perl that have now been fixed.

https://rt.perl.org/Public/Bug/Display.html?id=126755

Perl 5.22 suffers from two out-of-bounds read and multiple small buffer over-read vulnerabilities in the VDir::MapPathA and VDir::MapPathW functions that could potentially be exploited to achieve arbitrary code execution. The out-of-bounds read issues exist because the functions in question do not validate that the chr argument passed to DriveIndex, which calculates an index:

inline int DriveIndex(char chr)
{
if (chr == '\\' || chr == '/')
    return ('Z'-'A')+1;
return (chr | 0x20)-'a';
};

In the VDir::MapPathA function, DriveIndex is called with a potentially untrusted value, pInName, and the return value is then passed to GetDirA:

char *VDir::MapPathA(const char pInName) { / * possiblities -- relative path or absolute path with or without drive letter * OR UNC name */ [...]

if (pInName[1] == ':') {
[...]
}
else {
    /* relative path with drive letter */
    strcpy(szBuffer, GetDirA(DriveIndex(*pInName)));
    strcat(szBuffer, &pInName[2]);
    if(strlen(szBuffer) > MAX_PATH)
	szBuffer[MAX_PATH] = '\0';

    DoGetFullPathNameA(szBuffer, sizeof(szLocalBufferA), szLocalBufferA);
}
}
else {
[...]
}

return szLocalBufferA;

}

GetDirA then uses the unbounded index argument to index into dirTableA, a fixed length char pointer array.

inline const char *GetDirA(int index)
{
char *ptr = dirTableA[index];
if (!ptr) {
    /* simulate the existence of this drive */
    ptr = szLocalBufferA;
    ptr[0] = 'A' + index;
    ptr[1] = ':';
    ptr[2] = '\\';
    ptr[3] = 0;
}
return ptr;
};

In cases where index is attacker controlled, this behavior can be used to read outside of the dirTableA array. This is especially problematic because the value returned is then copied to a fixed length buffer using strcpy:

strcpy(szBuffer, GetDirA(DriveIndex(*pInName)));

If an attacker can manipulate the layout of memory to trick GetDirA into returning a string larger than szBuffer, a buffer overflow will occur. The issue in VDir::MapPathW is nearly identical..

Further, multiple small and less critical buffer over-reads exist in both VDir::MapPathA and VDir::MapPathW:

char *VDir::MapPathA(const char pInName) { / * possiblities -- relative path or absolute path with or without drive letter * OR UNC name */ [...]

if (!length) <<< Check here confirms the buffer is at least of length 2 (including null) before continuing execution.
return (char*)pInName;

[...]

if (pInName[1] == ':') { <<< While technically no over-read can occur here, pInName is a single character, this checks the null terminator.
/* has drive letter */
if (IsPathSep(pInName[2])) { <<<< This could cause an over-read because the string could possibly be of length 2 (including null).
    /* absolute with drive letter */
    DoGetFullPathNameA((char*)pInName, sizeof(szLocalBufferA), szLocalBufferA);
}
else {
    /* relative path with drive letter */
    strcpy(szBuffer, GetDirA(DriveIndex(*pInName)));
    strcat(szBuffer, &pInName[2]); <<<< This could cause an over-read for the same reason.
    if(strlen(szBuffer) > MAX_PATH)
	szBuffer[MAX_PATH] = '\0';

    DoGetFullPathNameA(szBuffer, sizeof(szLocalBufferA), szLocalBufferA);
}
}
else {
[...]
}
}

return szLocalBufferA;

}

To observe the out-of-bounds read vulnerability in VDir::MapPathA, the following script can be executed while Perl is under a debugger:

print glob "]:";

Which will result in an exception similar to the following:

(f78.1dd8): Access violation - code c0000005 (first chance) First chance exceptions are reported before any exception handling. This exception may be expected and handled. eax=0081d62c ebx=0081dae2 ecx=765c7377 edx=7eff3920 esi=0081dae0 edi=0081d62c eip=747613a0 esp=0081d608 ebp=74744fac iopl=0 nv up ei pl nz na pe nc cs=0023 ss=002b ds=002b es=002b fs=0053 gs=002b efl=00010206 MSVCR110!strcat+0x71: 747613a0 8a11 mov dl,byte ptr [ecx] ds:002b:765c7377=?? 0:000> k ChildEBP RetAddr 0081d608 709059ea MSVCR110!strcat+0x71 0081d940 7090688e perl523!VDir::MapPathA+0xdd 0081d94c 7090e295 perl523!PerlDirMapPathA+0x1f 0081dab4 70906736 perl523!win32_stat+0x6e 0081dac0 72541f60 perl523!PerlLIOLstat+0xd 0081dee4 7254181b Glob!g_lstat+0x72 0081df6c 725415e7 Glob!glob2+0x7a 0081efb0 72541141 Glob!glob0+0x181 0081f7cc 725420e8 Glob!bsd_glob+0x11d 0081f814 72542929 Glob!doglob+0x3f 0081f850 72542391 Glob!csh_glob+0x4a2 0081f894 708a05f4 Glob!iterate+0x1e8 0081f8ac 708cd3d8 perl523!Perl_pp_glob+0x19a 0081f8b8 70871fd8 perl523!Perl_runops_standard+0xc 0081f8cc 70871ef8 perl523!S_run_body+0xdf 0081f938 70908290 perl523!perl_run+0x1e6 0081fb68 00fe1216 perl523!RunPerl+0xbc 0081fba8 76de3744 perl!__tmainCRTStartup+0xfd 0081fbbc 77b7a064 KERNEL32!BaseThreadInitThunk+0x24 0081fc04 77b7a02f ntdll!__RtlUserThreadStart+0x2f 0081fc14 00000000 ntdll!_RtlUserThreadStart+0x1b

To fix the issue, it is recommended that the VDir::MapPathA and VDir::MapPathW functions validate the drive letter to ensure no out-of-bounds reads occur, and also check the length of the pInName argument to ensure no buffer over-reads occur. A proposed patch is attached. However, the patch only addresses the issues in VDir::MapPathA because it was not immediately clear how to hit VDir::MapPathW for the purpose of testing.

Credit: John Leitch ([email protected]), Bryce Darling ([email protected])