crypt uses uninit/unalloced/freed memory as salt #15091
Comments
From @bulk88Created by @bulk88There seems to be no bounds checking for minimum length of things in Submitted to security list since a hashing function that uses uninit --------------------------------------------------------------------- --------------------------------------------------------------------- /* eay 25/08/92 for (i=0; i<8; i++) ------------------------------------------------------------------- if (DO_UTF8(left)) { sv_utf8_downgrade(tsv, FALSE); Callstack ------------------------------------------------------------------------- Dumps of SVs. SV * left from perl523.dll!Perl_pp_crypt(interpreter * my_perl) Line SV = PVIV(0x2adde4) at 0x2af374 SV * right from perl523.dll!Perl_pp_crypt(interpreter * my_perl) Line SV = PV(0x290884) at 0x2af314 Perl Info |
From @iabynOn Tue, Dec 15, 2015 at 04:58:14AM -0800, bulk88 wrote:
I don't know what POSIX formally states, but the linux manpage for crypt Valgrind shows that a salt of length < 2 isn't causing any illegal accesses, So I would lean towards win32_crypt/des_fcrypt being wrong for not -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From zefram@fysh.orgbulk88 wrote:
There is no minimum length. crypt(3) takes two nul-terminated string
Yes, that's a clear flaw. -zefram |
From @tonycozOn Tue Dec 15 10:44:28 2015, zefram@fysh.org wrote:
POSIX requires that the salt be at least two bytes long. http://pubs.opengroup.org/onlinepubs/9699919799/functions/crypt.html
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", "!!") || $!' Tony |
From @tonycoz0001-perl-126922-avoid-access-to-uninitialized-memory-in-.patchFrom 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
|
From @bulk88
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
C:\p523\src\t> |
From @tonycozOn Sat Dec 19 11:53:43 2015, bulk88 wrote:
Not all other OS implementations support classic DES crypt(), for example see https://rt-archive.perl.org/perl5/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.
The classic DES crypt() hasn't been considered secure for many years, which is why many implementations support extensions that use other algorithms.
It's doing pretty much what I did - rejecting invalid characters.
As does the win32 implementation.
Cool. We have another Win32 specific security issue outstanding. Does this issue require a CVE? Tony |
From @bulk88Tony Cook via RT wrote:
I dont have much comment on a CVE, its not my thing to decide. The only Another question, is the result of crypt() on POSIX required to be I am not sure if anyone but TonyC will respond to this ticket before it |
From @iabynOn Fri, Dec 25, 2015 at 10:09:53PM -0500, bulk88 wrote:
The main purpose of having crypt() built-in to perl was to make it easy to The hashed password and the two salt characters for a newly-created user To create a new password, a script would create 2 random salt chars ($salt To check a password, ($hash,$salt) would be read from /etc/password, then I suspect that any code which failed to pass a 2-char salt to crypt() Also, I suspect the use of crypt() on win32 systems is vanishingly small, So I don't think a CVE is required. -- |
From @rjbs* Dave Mitchell <davem@iabyn.com> [2015-12-26T06:02:53]
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 If we have any objections, please let them be stated by the 4th. Failing that, -- |
From @rjbsI have moved this ticket from perl5-security to perl5. -- |
From [Unknown Contact. See original ticket]I have moved this ticket from perl5-security to perl5. -- |
From @tonycozOn Wed Dec 16 16:21:43 2015, tonyc wrote:
Applied as d691474 and added to maint-votes. Tony |
|
@tonycoz - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#126922 (status was 'resolved')
Searchable as RT126922$
The text was updated successfully, but these errors were encountered: