Skip Menu |
Report information
Id: 126922
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: bulk88 <bulk88 [at] hotmail.com>
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: (no value)
Type: (no value)
Perl Version: (no value)
Fixed In: (no value)



To: perl5-security-report [...] perl.org
From: bulk88 <bulk88 [...] hotmail.com>
Subject: crypt uses uninit/unalloced/freed memory as salt
Date: Tue, 15 Dec 2015 07:57:26 -0500
Download (untitled) / with headers
text/plain 9.8k
This is a bug report for perl from bulk88@hotmail.com, generated with the help of perlbug 1.40 running under perl 5.23.5. ----------------------------------------------------------------- [Please describe your issue here] There seems to be no bounds checking for minimum length of things in Perl_pp_crypt. Either win32_crypt/des_fcrypt are flawed (not correctly processing C empty string as arguments), or Perl_pp_crypt is flawed by not bounds checking for minimum string length before handing off the char *s to libc's crypt function, well in this case p5p's win32 equivalent of libc crypt. IDK what the minimum string length policy is on POSIX crypt. Submitted to security list since a hashing function that uses uninit memory as a seed can be used to do interesting things (collisions/perf degrade, or less permutations to brute force crack it). --------------------------------------------------------------------- C:\p523\src>drmemory -- perl -e"my$var=substr(chr 256,0,0);my$dummy=crypt 0,$var;" ~~Dr.M~~ Dr. Memory version 1.9.0 ~~Dr.M~~ Running "perl "-emy$var=substr(chr 256,0,0);my$dummy=crypt 0,$var;"" ~~Dr.M~~ ~~Dr.M~~ Error #1: UNINITIALIZED READ: reading register al ~~Dr.M~~ # 0 perl523.dll!des_fcrypt [c:\p523\src\win32\fcrypt.c:489] ~~Dr.M~~ # 1 perl523.dll!win32_crypt [c:\p523\src\win32\win32.c:2486] ~~Dr.M~~ # 2 perl523.dll!PerlProcCrypt [c:\p523\src\win32\perlhost.h:1530] ~~Dr.M~~ # 3 perl523.dll!Perl_pp_crypt [c:\p523\src\pp.c:3668] ~~Dr.M~~ # 4 perl523.dll!Perl_runops_standard [c:\p523\src\run.c:41] ~~Dr.M~~ # 5 perl523.dll!RunPerl [c:\p523\src\win32\perllib.c:258] ~~Dr.M~~ # 6 __tmainCRTStartup [f:\dd\vctools\crt\crtw32\dllstuff\crtexe.c:626] ~~Dr.M~~ # 7 KERNEL32.dll!BaseThreadInitThunk ~~Dr.M~~ Note: @0:00:02.355 in thread 5068 ~~Dr.M~~ Note: instruction: test %al %al ~~Dr.M~~ ~~Dr.M~~ ERRORS FOUND: ~~Dr.M~~ 0 unique, 0 total unaddressable access(es) ~~Dr.M~~ 1 unique, 1 total uninitialized access(es) ~~Dr.M~~ 0 unique, 0 total invalid heap argument(s) ~~Dr.M~~ 0 unique, 0 total GDI usage error(s) ~~Dr.M~~ 0 unique, 0 total handle leak(s) ~~Dr.M~~ 0 unique, 0 total warning(s) ~~Dr.M~~ 0 unique, 0 total, 0 byte(s) of leak(s) ~~Dr.M~~ 0 unique, 0 total, 0 byte(s) of possible leak(s) ~~Dr.M~~ ERRORS IGNORED: ~~Dr.M~~ 5 potential leak(s) (suspected false positives) ~~Dr.M~~ (details: C:\Users\Owner\AppData\Roaming\Dr. Memory\DrMemory-perl.exe.1720.000\potential_errors.txt) ~~Dr.M~~ 10 unique, 10 total, 300 byte(s) of still-reachable allocation(s) ~~Dr.M~~ (re-run with "-show_reachable" for details) ~~Dr.M~~ Details: C:\Users\Owner\AppData\Roaming\Dr. Memory\DrMemory-perl.exe.17 20.000\results.txt --------------------------------------------------------------------- char * des_fcrypt(const char *buf, const char *salt, char *buff) { unsigned int i,j,x,y; unsigned long Eswap0,Eswap1; unsigned long out[2],ll; des_cblock key; des_key_schedule ks; unsigned char bb[9]; unsigned char *b=bb; unsigned char c,u; /* eay 25/08/92 * If you call crypt("pwd","*") as often happens when you * have * as the pwd field in /etc/passwd, the function * returns *\0XXXXXXXXX * The \0 makes the string look like * so the pwd "*" would * crypt to "*". This was found when replacing the crypt in * our shared libraries. People found that the disbled * accounts effectivly had no passwd :-(. */ x=buff[0]=((salt[0] == '\0')?(char)'A':salt[0]); Eswap0=con_salt[x]; x=buff[1]=((salt[1] == '\0')?(char)'A':salt[1]);<<<<<<<<<<<<<<LINE 489 Eswap1=con_salt[x]<<4; for (i=0; i<8; i++) ------------------------------------------------------------------- PP(pp_crypt) { #ifdef HAS_CRYPT dSP; dTARGET; dPOPTOPssrl; STRLEN len; const char *tmps = SvPV_const(left, len); if (DO_UTF8(left)) { /* If Unicode, try to downgrade. * If not possible, croak. * Yes, we made this up. */ SV* const tsv = newSVpvn_flags(tmps, len, SVf_UTF8|SVs_TEMP); sv_utf8_downgrade(tsv, FALSE); tmps = SvPV_const(tsv, len); } # ifdef USE_ITHREADS # ifdef HAS_CRYPT_R if (!PL_reentrant_buffer->_crypt_struct_buffer) { /* This should be threadsafe because in ithreads there is only * one thread per interpreter. If this would not be true, * we would need a mutex to protect this malloc. */ PL_reentrant_buffer->_crypt_struct_buffer = (struct crypt_data *)safemalloc(sizeof(struct crypt_data)); #if defined(__GLIBC__) || defined(__EMX__) if (PL_reentrant_buffer->_crypt_struct_buffer) { PL_reentrant_buffer->_crypt_struct_buffer->initialized = 0; /* work around glibc-2.2.5 bug */ PL_reentrant_buffer->_crypt_struct_buffer->current_saltbits = 0; } #endif } # endif /* HAS_CRYPT_R */ # endif /* USE_ITHREADS */ # ifdef FCRYPT sv_setpv(TARG, fcrypt(tmps, SvPV_nolen_const(right))); # else sv_setpv(TARG, PerlProc_crypt(tmps, SvPV_nolen_const(right))); <<<<<<<<<<<<<<<<<<<<<<LINE 3668 # endif SvUTF8_off(TARG); SETTARG; RETURN; #else DIE(aTHX_ "The crypt() function is unimplemented due to excessive paranoia."); #endif } ------------------------------------------------------------------------- Callstack ------------------------------------------------------------------------- perl523.dll!des_fcrypt(const char * buf, const char * salt, char * buff) Line 489 C perl523.dll!win32_crypt(const char * txt, const char * salt) Line 2486 C perl523.dll!PerlProcCrypt(IPerlProc * piPerl, const char * clear, const char * salt) Line 1530 C++ perl523.dll!Perl_pp_crypt(interpreter * my_perl) Line 3668 C perl523.dll!Perl_runops_standard(interpreter * my_perl) Line 41 C perl523.dll!S_run_body(interpreter * my_perl, long oldscope) Line 2464 C perl523.dll!perl_run(interpreter * my_perl) Line 2387 C perl523.dll!RunPerl(int argc, char * * argv, char * * env) Line 258 C++ perl.exe!__tmainCRTStartup() Line 626 C kernel32.dll!@BaseThreadInitThunk@12() Unknown ntdll.dll!___RtlUserThreadStart@8() Unknown ntdll.dll!__RtlUserThreadStart@8() Unknown ------------------------------------------------------------------------- Dumps of SVs. SV * left from perl523.dll!Perl_pp_crypt(interpreter * my_perl) Line 3668 C SV = PVIV(0x2adde4) at 0x2af374 REFCNT = 1 FLAGS = 0x8005505 (IOK,POK,READONLY,pIOK,pPOK) IV = 0 PV = 0x2b5054 "0"\0 CUR = 1 LEN = 3 SV * right from perl523.dll!Perl_pp_crypt(interpreter * my_perl) Line 3668 C SV = PV(0x290884) at 0x2af314 REFCNT = 1 FLAGS = 0x20004403 (POK,pPOK,UTF8) PV = 0x2b37d4 ""\0 [UTF8 ""] CUR = 0 LEN = 2 [Please do not change anything below this line] ----------------------------------------------------------------- --- Flags: category=core severity=high --- Site configuration information for perl 5.23.5: Configured by Owner at Fri Dec 11 22:33:57 2015. Summary of my perl5 (revision 5 version 23 subversion 5) configuration: Local Commit: 8300cd2ba7e87391e21c6a94c00c193b077f3221 Ancestor: ab36d597128b4016dda4ce194191e3237cf3f6a1 Platform: osname=MSWin32, osvers=6.1, archname=MSWin32-x86-multi-thread uname='' config_args='undef' hint=recommended, useposix=true, d_sigaction=undef useithreads=define, usemultiplicity=define use64bitint=undef, use64bitall=undef, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='cl', ccflags ='-nologo -GF -W3 -O1 -MD -Zi -DNDEBUG -GL -DWIN32 -D_CONSOLE -DNO_STRICT -D_CRT_SECURE_NO_DEPRECATE -D_CRT_NONSTDC_NO_DEPRECATE -DPERL_TEXTMODE_SCRIPTS -DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -DWIN32_NO_REGISTRY', optimize='-O1 -MD -Zi -DNDEBUG -GL', cppflags='-DWIN32' ccversion='18.00.31101', gccversion='', gccosandvers='' intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234, doublekind=3 d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=8, longdblkind=0 ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='__int64', lseeksize=8 alignbytes=8, prototype=define Linker and Libraries: ld='link', ldflags ='-nologo -nodefaultlib -debug -opt:ref,icf -ltcg -libpath:"c:\perl\lib\CORE" -machine:x86 -subsystem:console,"5.01"' libpth=\lib libs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib comdlg32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib version.lib odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib perllibs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib comdlg32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib version.lib odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib libc=msvcrt.lib, so=dll, useshrplib=true, libperl=perl523.lib gnulibc_version='' Dynamic Linking: dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' ' cccdlflags=' ', lddlflags='-dll -nologo -nodefaultlib -debug -opt:ref,icf -ltcg -libpath:"c:\perl\lib\CORE" -machine:x86 -subsystem:console,"5.01"' Locally applied patches: e871160f42ac868768134e1f539f4c2406c18308 8300cd2ba7e87391e21c6a94c00c193b077f3221 --- @INC for perl 5.23.5: C:/p523/src/lib . --- Environment for perl 5.23.5: HOME (unset) LANG (unset) LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=C:\Program Files\Dr. Memory\bin;C:\sp520\c\bin;C:\Windows\system32;C:\Program Files\Microsoft Visual Studio 12.0\VC\BIN;C:\Program Files\Windows Kits\8.1\bin\x86;C:\Windows;C:\Program Files\ActiveState Komodo Edit 9;C:\Program Files\Git\bin; PERL_BADLANG (unset) SHELL (unset)
From: Dave Mitchell <davem [...] iabyn.com>
Date: Tue, 15 Dec 2015 18:10:29 +0000
To: perl5-security-report [...] perl.org
Subject: Re: [perl #126922] crypt uses uninit/unalloced/freed memory as salt
Download (untitled) / with headers
text/plain 1.3k
On Tue, Dec 15, 2015 at 04:58:14AM -0800, bulk88 wrote: Show quoted text
> # New Ticket Created by bulk88 > # Please include the string: [perl #126922] > # in the subject line of all future correspondence about this issue. > # <URL: https://rt.perl.org/Ticket/Display.html?id=126922 > > > > This is a bug report for perl from bulk88@hotmail.com, > generated with the help of perlbug 1.40 running under perl 5.23.5. > > > ----------------------------------------------------------------- > [Please describe your issue here] > > There seems to be no bounds checking for minimum length of things in > Perl_pp_crypt. Either win32_crypt/des_fcrypt are flawed (not correctly > processing C empty string as arguments), or Perl_pp_crypt is flawed by > not bounds checking for minimum string length before handing off the > char *s to libc's crypt function, well in this case p5p's win32 > equivalent of libc crypt. IDK what the minimum string length policy is > on POSIX crypt.
I don't know what POSIX formally states, but the linux manpage for crypt is amiguous. Valgrind shows that a salt of length < 2 isn't causing any illegal accesses, and crpyt() is returning NULL for a length(salt) < 2. So I would lean towards win32_crypt/des_fcrypt being wrong for not detecting a short salt. -- But Pity stayed his hand. "It's a pity I've run out of bullets", he thought. -- "Bored of the Rings"
From: Zefram <zefram [...] fysh.org>
To: perl5-security-report [...] perl.org
Subject: Re: [perl #126922] crypt uses uninit/unalloced/freed memory as salt
Date: Tue, 15 Dec 2015 18:44:09 +0000
Download (untitled) / with headers
text/plain 426b
bulk88 wrote: Show quoted text
>There seems to be no bounds checking for minimum length of things in >Perl_pp_crypt.
There is no minimum length. crypt(3) takes two nul-terminated string arguments. An excessively short salt string can produce silly output, but not a boundary excursion. Show quoted text
> Either win32_crypt/des_fcrypt are flawed (not correctly >processing C empty string as arguments),
Yes, that's a clear flaw. -zefram
RT-Send-CC: rt-deliver-to-perl5-security-report [...] rt.perl.org
Download (untitled) / with headers
text/plain 983b
On Tue Dec 15 10:44:28 2015, zefram@fysh.org wrote: Show quoted text
> bulk88 wrote:
> >There seems to be no bounds checking for minimum length of things in > >Perl_pp_crypt.
> > There is no minimum length. crypt(3) takes two nul-terminated string > arguments. An excessively short salt string can produce silly output, > but not a boundary excursion.
POSIX requires that the salt be at least two bytes long. http://pubs.opengroup.org/onlinepubs/9699919799/functions/crypt.html Show quoted text
>
> > Either win32_crypt/des_fcrypt are flawed (not correctly > >processing C empty string as arguments),
> > Yes, that's a clear flaw.
The attached validates the salt and adds some tests. The error behaviour is modelled on the glibc crypt(): tony@mars:.../git/bse$ perl -le 'print crypt("abc", "!!") || $!' Invalid argument tony@mars:.../git/bse$ perl -le 'print crypt("abc", "") || $!' Invalid argument tony@mars:.../git/bse$ perl -le 'print crypt("abc", "//") || $!' //dOFYpvAsViA Tony
Subject: 0001-perl-126922-avoid-access-to-uninitialized-memory-in-.patch
From 1ff6c0fee48071c865f383e2d8168e99b7fb2a3a Mon Sep 17 00:00:00 2001 From: Tony Cook <tony@develop-help.com> Date: Thu, 17 Dec 2015 11:15:31 +1100 Subject: [perl #126922] avoid access to uninitialized memory in win32 crypt() Previously the Win32 crypt implementation() would access the first and second characters of the salt, even if the salt was zero length. Add validation that will detect both a short salt and invalid characters in the salt. --- MANIFEST | 1 + t/win32/crypt.t | 41 +++++++++++++++++++++++++++++++++++++++++ win32/fcrypt.c | 14 ++++++++++++++ 3 files changed, 56 insertions(+) create mode 100644 t/win32/crypt.t diff --git a/MANIFEST b/MANIFEST index 2adf881..a0fc308 100644 --- a/MANIFEST +++ b/MANIFEST @@ -5576,6 +5576,7 @@ t/uni/universal.t See if Unicode in calls to UNIVERSAL works t/uni/upper.t See if Unicode casing works t/uni/variables.t See that the rules for variable names work t/uni/write.t See if Unicode formats work +t/win32/crypt.t Test Win32 crypt for compatibility t/win32/fs.t Test Win32 link for compatibility t/win32/popen.t Test for stdout races in backticks, etc t/win32/runenv.t Test if Win* perl honors its env variables diff --git a/t/win32/crypt.t b/t/win32/crypt.t new file mode 100644 index 0000000..f0e89ab --- /dev/null +++ b/t/win32/crypt.t @@ -0,0 +1,41 @@ +#!./perl + +BEGIN { + chdir 't' if -d 't'; + @INC = '../lib'; + require "./test.pl"; + eval 'use Errno'; + die $@ if $@ and !is_miniperl(); +} + +my @bad_salts = + ( + [ '', 'zero-length' ], + [ 'a', 'length 1' ], + [ '!a', 'bad first character' ], + [ 'a!', 'bad second character' ], + [ '@a', 'fencepost before A' ], + [ '[a', 'fencepost after Z' ], + [ '`a', 'fencepost before a' ], + [ '{a', 'fencepost after z' ], + [ '-a', 'fencepost before .' ], + [ ':a', 'fencepost after 9' ], + ); + +my @good_salts = qw(aa zz AA ZZ .. 99); + +plan tests => 2 * @bad_salts + 1 + @good_salts; + +for my $bad_salt (@bad_salts) { + my ($salt, $what) = @$bad_salt; + $! = 0; + is(crypt("abc", $salt), undef, "bad salt ($what)"); + is(0+$!, &Errno::EINVAL, "check errno ($what)"); +} + +is(crypt("abcdef", "ab"), "abDMWw5NL.afs", "sanity check result"); + +# just to check we're not rejecting any good salts +for my $good_salt (@good_salts) { + isnt(crypt("abcdef", $good_salt), undef, "good salt $good_salt"); +} diff --git a/win32/fcrypt.c b/win32/fcrypt.c index fd42d75..4433e68 100644 --- a/win32/fcrypt.c +++ b/win32/fcrypt.c @@ -1,6 +1,7 @@ /* fcrypt.c */ /* Copyright (C) 1993 Eric Young - see README for more details */ #include <stdio.h> +#include <errno.h> /* Eric Young. * This version of crypt has been developed from my MIT compatable @@ -464,6 +465,14 @@ unsigned const char cov_2char[64]={ 0x73,0x74,0x75,0x76,0x77,0x78,0x79,0x7A }; +/* the salt for classic DES crypt (which is all we implement here) + permits [./0-9A-Za-z], since '.' and '/' immediately preceed + '0' we don't need individual checks for '.' and '/' +*/ +#define good_for_salt(c) \ + ((c) >= '.' && (c) <= '9' || (c) >= 'A' && (c) <= 'Z' || \ + (c) >= 'a' && (c) <= 'z') + char * des_fcrypt(const char *buf, const char *salt, char *buff) { @@ -476,6 +485,11 @@ des_fcrypt(const char *buf, const char *salt, char *buff) unsigned char *b=bb; unsigned char c,u; + if (!good_for_salt(salt[0]) || !good_for_salt(salt[1])) { + errno = EINVAL; + return NULL; + } + /* eay 25/08/92 * If you call crypt("pwd","*") as often happens when you * have * as the pwd field in /etc/passwd, the function -- 1.9.5.msysgit.0
To: "perl5-security-report [...] perl.org" <perl5-security-report [...] perl.org>
From: bulk 88 <bulk88 [...] hotmail.com>
Date: Sat, 19 Dec 2015 11:42:33 -0500
Subject: RE: [perl #126922] crypt uses uninit/unalloced/freed memory as salt
Download (untitled) / with headers
text/plain 7.1k


> Subject: [perl #126922] crypt uses uninit/unalloced/freed memory as salt
> From: perl5-security-report@perl.org
> To: bulk88@hotmail.com
> Date: Wed, 16 Dec 2015 16:21:44 -0800
>
> On Tue Dec 15 10:44:28 2015, zefram@fysh.org wrote:
> > bulk88 wrote:
> > >There seems to be no bounds checking for minimum length of things in
> > >Perl_pp_crypt.
> >
> > There is no minimum length. crypt(3) takes two nul-terminated string
> > arguments. An excessively short salt string can produce silly output,
> > but not a boundary excursion.
>
> POSIX requires that the salt be at least two bytes long.
>
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/crypt.html
>
> >
> > > Either win32_crypt/des_fcrypt are flawed (not correctly
> > >processing C empty string as arguments),
> >
> > Yes, that's a clear flaw.
>
> The attached validates the salt and adds some tests.
>
> The error behaviour is modelled on the glibc crypt():
>
> tony@mars:.../git/bse$ perl -le 'print crypt("abc", "!!") || $!'
> Invalid argument
> tony@mars:.../git/bse$ perl -le 'print crypt("abc", "") || $!'
> Invalid argument
> tony@mars:.../git/bse$ perl -le 'print crypt("abc", "//") || $!'
> //dOFYpvAsViA
>
> Tony

Couldn't that win32/crypt.t test be for all OSes, not just Win32?

Another question generic question unrelated to the uninit memory read, if the input character set for the salt is only 60 chars out of 256, or %23, isn't that a very bad entropy reduction? If I read the POSIX document you linked, it says only the first 2 bytes of the salt string are used, is that correct? Or POSIX crypt stopped being crypto-secure a long time and is in the family as perl rand()?

I'll note glibc filters charset https://sourceware.org/git/?p=glibc.git;a=blob;f=crypt/crypt_util.c;h=05f6a56257b44a5923af7552c499eee20f37cba9;hb=HEAD#l581

glibc seems to only use the first 2 bytes https://sourceware.org/git/?p=glibc.git;a=blob;f=crypt/crypt_util.c;h=05f6a56257b44a5923af7552c499eee20f37cba9;hb=HEAD#l604

Your patch fixed the uninit memory read report from Dr Mem with the one liner q|perl -e"my$var=substr(chr256,0,0);my$dummy=crypt 0,$var;"| . Out of curiosity, i ran win32/crypt.t on the unpatched perl. Result below

C:\p523\src\t>drmemory -- perl -I..\lib win32/crypt.t
~~Dr.M~~ Dr. Memory version 1.9.0
~~Dr.M~~ Running "perl -I..\lib win32/crypt.t"
1..27
~~Dr.M~~
~~Dr.M~~ Error #1: UNINITIALIZED READ: reading register al
~~Dr.M~~ # 0 perl523.dll!des_fcrypt             [c:\p523\src\win32\fcrypt.c:489]

~~Dr.M~~ # 1 perl523.dll!win32_crypt            [c:\p523\src\win32\win32.c:2486]

~~Dr.M~~ # 2 perl523.dll!PerlProcCrypt          [c:\p523\src\win32\perlhost.h:15
30]
~~Dr.M~~ # 3 perl523.dll!Perl_pp_crypt          [c:\p523\src\pp.c:3668]
~~Dr.M~~ # 4 perl523.dll!Perl_runops_standard   [c:\p523\src\run.c:41]
~~Dr.M~~ # 5 perl523.dll!RunPerl                [c:\p523\src\win32\perllib.c:258
]
~~Dr.M~~ # 6 __tmainCRTStartup                  [f:\dd\vctools\crt\crtw32\dllstu
ff\crtexe.c:626]
~~Dr.M~~ # 7 KERNEL32.dll!BaseThreadInitThunk
~~Dr.M~~ Note: @0:00:10.702 in thread 1192
~~Dr.M~~ Note: instruction: test   %al %al
not ok 1 - bad salt (zero-length)
# Failed test 1 - bad salt (zero-length) at win32/crypt.t line 32
#      got "Ao.bTQLzE83bs"
# expected undef
not ok 2 - check errno (zero-length)
# Failed test 2 - check errno (zero-length) at win32/crypt.t line 33
#      got "0"
# expected "22"
not ok 3 - bad salt (length 1)
# Failed test 3 - bad salt (length 1) at win32/crypt.t line 32
#      got "aAI3uM763cGMY"
# expected undef
not ok 4 - check errno (length 1)
# Failed test 4 - check errno (length 1) at win32/crypt.t line 33
#      got "0"
# expected "22"
not ok 5 - bad salt (bad first character)
# Failed test 5 - bad salt (bad first character) at win32/crypt.t line 32
#      got "!aywdxyygOYLw"
# expected undef
not ok 6 - check errno (bad first character)
# Failed test 6 - check errno (bad first character) at win32/crypt.t line 33
#      got "0"
# expected "22"
not ok 7 - bad salt (bad second character)
# Failed test 7 - bad salt (bad second character) at win32/crypt.t line 32
#      got "a!uoC5noeqDXE"
# expected undef
not ok 8 - check errno (bad second character)
# Failed test 8 - check errno (bad second character) at win32/crypt.t line 33
#      got "0"
# expected "22"
not ok 9 - bad salt (fencepost before A)
# Failed test 9 - bad salt (fencepost before A) at win32/crypt.t line 32
#      got "@ajAyahtsZls6"
# expected undef
not ok 10 - check errno (fencepost before A)
# Failed test 10 - check errno (fencepost before A) at win32/crypt.t line 33
#      got "0"
# expected "22"
not ok 11 - bad salt (fencepost after Z)
# Failed test 11 - bad salt (fencepost after Z) at win32/crypt.t line 32
#      got "[aqSURRK/uV3A"
# expected undef
not ok 12 - check errno (fencepost after Z)
# Failed test 12 - check errno (fencepost after Z) at win32/crypt.t line 33
#      got "0"
# expected "22"
not ok 13 - bad salt (fencepost before a)
# Failed test 13 - bad salt (fencepost before a) at win32/crypt.t line 32
#      got "`aTaMzmfjIpJ6"
# expected undef
not ok 14 - check errno (fencepost before a)
# Failed test 14 - check errno (fencepost before a) at win32/crypt.t line 33
#      got "0"
# expected "22"
not ok 15 - bad salt (fencepost after z)
# Failed test 15 - bad salt (fencepost after z) at win32/crypt.t line 32
#      got "{aywdxyygOYLw"
# expected undef
not ok 16 - check errno (fencepost after z)
# Failed test 16 - check errno (fencepost after z) at win32/crypt.t line 33
#      got "0"
# expected "22"
not ok 17 - bad salt (fencepost before .)
# Failed test 17 - bad salt (fencepost before .) at win32/crypt.t line 32
#      got "-aywdxyygOYLw"
# expected undef
not ok 18 - check errno (fencepost before .)
# Failed test 18 - check errno (fencepost before .) at win32/crypt.t line 33
#      got "0"
# expected "22"
not ok 19 - bad salt (fencepost after 9)
# Failed test 19 - bad salt (fencepost after 9) at win32/crypt.t line 32
#      got ":a0QeMflQunsk"
# expected undef
not ok 20 - check errno (fencepost after 9)
# Failed test 20 - check errno (fencepost after 9) at win32/crypt.t line 33
#      got "0"
# expected "22"
ok 21 - sanity check result
ok 22 - good salt aa
ok 23 - good salt zz
ok 24 - good salt AA
ok 25 - good salt ZZ
ok 26 - good salt ..
ok 27 - good salt 99
~~Dr.M~~
~~Dr.M~~ ERRORS FOUND:
~~Dr.M~~       0 unique,     0 total unaddressable access(es)
~~Dr.M~~       1 unique,     1 total uninitialized access(es)
~~Dr.M~~       0 unique,     0 total invalid heap argument(s)
~~Dr.M~~       0 unique,     0 total GDI usage error(s)
~~Dr.M~~       0 unique,     0 total handle leak(s)
~~Dr.M~~       0 unique,     0 total warning(s)
~~Dr.M~~       0 unique,     0 total,      0 byte(s) of leak(s)
~~Dr.M~~       0 unique,     0 total,      0 byte(s) of possible leak(s)
~~Dr.M~~ ERRORS IGNORED:
~~Dr.M~~      16 potential leak(s) (suspected false positives)
~~Dr.M~~          (details: C:\Users\Owner\AppData\Roaming\Dr. Memory\DrMemory-p
erl.exe.3012.000\potential_errors.txt)
~~Dr.M~~      10 unique,    10 total,    300 byte(s) of still-reachable allocati
on(s)
~~Dr.M~~          (re-run with "-show_reachable" for details)
~~Dr.M~~ Details: C:\Users\Owner\AppData\Roaming\Dr. Memory\DrMemory-perl.exe.30
12.000\results.txt

C:\p523\src\t>
RT-Send-CC: rt-deliver-to-perl5-security-report [...] rt.perl.org
On Sat Dec 19 11:53:43 2015, bulk88 wrote: Show quoted text
> Couldn't that win32/crypt.t test be for all OSes, not just Win32?
Not all other OS implementations support classic DES crypt(), for example see https://rt.perl.org/Ticket/Display.html?id=121591. The tests in t/op/crypt.t attempt to test perl's interface to the underlying crypt implementation, the new tests attempt to test the win32 specific implementation. Show quoted text
> Another question generic question unrelated to the uninit memory read, > if the input character set for the salt is only 60 chars out of 256, > or %23, isn't that a very bad entropy reduction? If I read the POSIX > document you linked, it says only the first 2 bytes of the salt string > are used, is that correct? Or POSIX crypt stopped being crypto-secure > a long time and is in the family as perl rand()?
The classic DES crypt() hasn't been considered secure for many years, which is why many implementations support extensions that use other algorithms. Show quoted text
It's doing pretty much what I did - rejecting invalid characters. Show quoted text
As does the win32 implementation. Show quoted text
> > Your patch fixed the uninit memory read report from Dr Mem with the > one liner q|perl -e"my$var=substr(chr256,0,0);my$dummy=crypt 0,$var;"| > . Out of curiosity, i ran win32/crypt.t on the unpatched perl. Result > below > > C:\p523\src\t>drmemory -- perl -I..\lib win32/crypt.t > ~~Dr.M~~ Dr. Memory version 1.9.0 > ~~Dr.M~~ Running "perl -I..\lib win32/crypt.t" > 1..27 > ~~Dr.M~~ > ~~Dr.M~~ Error #1: UNINITIALIZED READ: reading register al > ~~Dr.M~~ # 0 perl523.dll!des_fcrypt > [c:\p523\src\win32\fcrypt.c:489]
(mostly failing tests ommitted) Cool. We have another Win32 specific security issue outstanding. Does this issue require a CVE? Tony
To: perl5-security-report [...] perl.org
From: bulk88 <bulk88 [...] hotmail.com>
Subject: Re: [perl #126922] crypt uses uninit/unalloced/freed memory as salt
Date: Fri, 25 Dec 2015 22:09:53 -0500
Download (untitled) / with headers
text/plain 2.2k
Tony Cook via RT wrote: Show quoted text
> > Cool. > > We have another Win32 specific security issue outstanding. > > Does this issue require a CVE? > > Tony
I'd like for this ticket to be made public sooner than later. This memory corruption/segv bug ticket is the cause of the SEGVs in https://github.com/perl11/cperl/issues/95 and CI smoke https://ci.appveyor.com/project/rurban/cperl/build/5.22.2.728/job/6d2y5prny5ukq6or#L6653 . The patch in this ticket seems to have fixed the SEGV for me when I apply it to cperl. I dont have much comment on a CVE, its not my thing to decide. The only thing I can say is, I think (didnt read the failure mode too deeply), if there is a coding error and an app was passing empty string as the salt to crypt, the salt used would be predictable if you can predict what the bytes will be in the tail of the empty string PV buffer from Win32 malloc. One byte would be null (empty string term), and the 2nd byte either uninit 0s (fresh VM block from OS), or contents of previous memory allocation at that addr or semi-predictable malloc bookkeeping. The other argument is, des implemented crypt() ((26+26+12)*(26+26+12)=4096 possible salts, a 12 bit encryption key) is not secure in any way, and it is simply obfuscation, not encryption, obfuscation can always be reversed, hence this isn't a security bug. That is my thinking, but I am not the person to ask on security questions. Another question, is the result of crypt() on POSIX required to be stable between reboots? Can it be serialized to disk and guaranteed to be the same after a reboot on the same OS version? If the app that was passing by mistake empty string as salt, it stores serialized the hash number (that is what crypt returns kind of) to disk using uninit memory as the salt, reboots and ASLR/memory randomization changes things, then recomputes a different input with empty string as hash, a collision can occur since the salt of "empty string" in perl lang has different values between reboots and caused 2 different inputs that shouldn't have had the same hash number, have the same hash number because of different salts in C but they were the same salt in perl lang (""). I am not sure if anyone but TonyC will respond to this ticket before it is made public due to a lack of eyes on the security list.
Date: Sat, 26 Dec 2015 11:02:53 +0000
From: Dave Mitchell <davem [...] iabyn.com>
Subject: Re: [perl #126922] crypt uses uninit/unalloced/freed memory as salt
CC: perl5-security-report [...] perl.org
To: bulk88 <bulk88 [...] hotmail.com>
Download (untitled) / with headers
text/plain 1.5k
On Fri, Dec 25, 2015 at 10:09:53PM -0500, bulk88 wrote: Show quoted text
> Another question, is the result of crypt() on POSIX required to be stable > between reboots? Can it be serialized to disk and guaranteed to be the same > after a reboot on the same OS version?
The main purpose of having crypt() built-in to perl was to make it easy to write admin scripts that could create new password hashes, or test existing password hashes, for the user account "database" in /etc/password on traditional UNIX systems. The hashed password and the two salt characters for a newly-created user or password would be stored as 2+11 characters in a field in /etc/password. To create a new password, a script would create 2 random salt chars ($salt say), then store crypt($pass,$salt) and $salt in /etc/password. To check a password, ($hash,$salt) would be read from /etc/password, then (crypt($pass,$salt) eq $hash) would be tested for. I suspect that any code which failed to pass a 2-char salt to crypt() would have been spotted almost immediately, as it would either fail to generate new accounts that can logged in with, or would fail to authenticate users. Also, I suspect the use of crypt() on win32 systems is vanishingly small, and the use of crypt() on *any* system is rare now, since the hashing algorithm for user passwords on UNIX systems changed to more secure functions years ago. So I don't think a CVE is required. -- Counsellor Troi states something other than the blindingly obvious. -- Things That Never Happen in "Star Trek" #16
To: Dave Mitchell <davem [...] iabyn.com>
From: Ricardo Signes <perl.security [...] rjbs.manxome.org>
CC: bulk88 <bulk88 [...] hotmail.com>, perl5-security-report [...] perl.org
Date: Thu, 31 Dec 2015 21:49:35 -0500
Subject: Re: [perl #126922] crypt uses uninit/unalloced/freed memory as salt
Download (untitled) / with headers
text/plain 426b
* Dave Mitchell <davem@iabyn.com> [2015-12-26T06:02:53] Show quoted text
> So I don't think a CVE is required.
Sorry for my delayed reply. The holidays have kept me busy. I tentatively agree. As the one who is likely to deal with a CVE, I know that I have a bias against having a CVE. ;-) Accordingly, I ask for feedback. If we have any objections, please let them be stated by the 4th. Failing that, let us make this public. -- rjbs
Download signature.asc
application/pgp-signature 473b

Message body not shown because it is not plain text.

RT-Send-CC: perl5-porters [...] perl.org
I have moved this ticket from perl5-security to perl5. -- rjbs
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 502b
On Wed Dec 16 16:21:43 2015, tonyc wrote: Show quoted text
> The attached validates the salt and adds some tests. > > The error behaviour is modelled on the glibc crypt(): > > tony@mars:.../git/bse$ perl -le 'print crypt("abc", "!!") || $!' > Invalid argument > tony@mars:.../git/bse$ perl -le 'print crypt("abc", "") || $!' > Invalid argument > tony@mars:.../git/bse$ perl -le 'print crypt("abc", "//") || $!' > //dOFYpvAsViA
Applied as d691474c4cf3d3119367a72ebb28a990d039baf3 and added to maint-votes. Tony


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org