Bug #118956 for Crypt-Sodium: Crash when trying to decrypt an empty string

Fri Nov 25 06:07:44 2016 NEWELLC [...] cpan.org - Ticket created

Subject:

Crash when trying to decrypt an empty string

If I try to round trip '' through crypto_secretbox/crypto_secretbox_open I get a segfault - Signal SEGV. Looking at the code it appears that Perl_newSVpv assumes that if you pass it 0 for a length, it assumes you didn't know the length and it tries to use strlen to calculate it, leading to strlen segfaulting as you aren't providing a null terminated string. I have attached a quick patch to fix that and a test to check the issue is fixed.

Subject:

empty-string-segfault.patch

diff -Naur Crypt-Sodium-0.08/Sodium.xs /home/colin/.cpanm/work/1480069977.15466/Crypt-Sodium-0.08/Sodium.xs --- Crypt-Sodium-0.08/Sodium.xs 2015-10-18 06:34:28.000000000 +0100 +++ /home/colin/.cpanm/work/1480069977.15466/Crypt-Sodium-0.08/Sodium.xs 2016-11-25 10:55:24.232418482 +0000 @@ -308,7 +308,13 @@ (unsigned long long) clen, (const unsigned char*)n, (const unsigned char*)sk); if (status == 0) { - RETVAL = newSVpv( m, clen - crypto_secretbox_MACBYTES ); + // newSVpv assumes 0 in length mean 'don't know, figure it out by + // calling strlen, so we need to fudge it. + if (clen - crypto_secretbox_MACBYTES == 0) { + RETVAL = newSVpv( "", 0 ); + } else { + RETVAL = newSVpv( m, clen - crypto_secretbox_MACBYTES ); + } } else { RETVAL = &PL_sv_undef; } diff -Naur Crypt-Sodium-0.08/t/Crypt-Sodium.t /home/colin/.cpanm/work/1480069977.15466/Crypt-Sodium-0.08/t/Crypt-Sodium.t --- Crypt-Sodium-0.08/t/Crypt-Sodium.t 2015-10-18 06:24:22.000000000 +0100 +++ /home/colin/.cpanm/work/1480069977.15466/Crypt-Sodium-0.08/t/Crypt-Sodium.t 2016-11-25 10:57:44.596842752 +0000 @@ -28,6 +28,9 @@ $enciphered = crypto_secretbox($message, $n, $k); is(crypto_secretbox_open($enciphered, $n, $k), $message, "Testing roundtrip of crypto_secretbox"); +my $empty = crypto_secretbox('', $n, $k); +is(crypto_secretbox_open($empty, $n, $k), '', "Testing we can encrypt/decrypt empty string"); + # public key crypto my ($pk1, $sk1) = box_keypair(); my ($pk2, $sk2) = box_keypair();

Fri Nov 25 06:15:49 2016 NEWELLC [...] cpan.org - Correspondence added

Having read the documentation a little more it appears that newSVpvn is the function this module really needs. I've created a better patch.

Subject:

empty-string-segfault.patch

diff -Naur Crypt-Sodium-0.08/Sodium.xs /home/colin/.cpanm/work/1480069977.15466/Crypt-Sodium-0.08/Sodium.xs --- Crypt-Sodium-0.08/Sodium.xs 2015-10-18 06:34:28.000000000 +0100 +++ /home/colin/.cpanm/work/1480069977.15466/Crypt-Sodium-0.08/Sodium.xs 2016-11-25 11:11:52.659734477 +0000 @@ -308,7 +308,7 @@ (unsigned long long) clen, (const unsigned char*)n, (const unsigned char*)sk); if (status == 0) { - RETVAL = newSVpv( m, clen - crypto_secretbox_MACBYTES ); + RETVAL = newSVpvn( m, clen - crypto_secretbox_MACBYTES ); } else { RETVAL = &PL_sv_undef; } diff -Naur Crypt-Sodium-0.08/t/Crypt-Sodium.t /home/colin/.cpanm/work/1480069977.15466/Crypt-Sodium-0.08/t/Crypt-Sodium.t --- Crypt-Sodium-0.08/t/Crypt-Sodium.t 2015-10-18 06:24:22.000000000 +0100 +++ /home/colin/.cpanm/work/1480069977.15466/Crypt-Sodium-0.08/t/Crypt-Sodium.t 2016-11-25 11:12:57.551967477 +0000 @@ -28,6 +28,9 @@ $enciphered = crypto_secretbox($message, $n, $k); is(crypto_secretbox_open($enciphered, $n, $k), $message, "Testing roundtrip of crypto_secretbox"); +my $empty = crypto_secretbox('', $n, $k); +is(crypto_secretbox_open($empty, $n, $k), '', "Testing we can encrypt/decrypt empty string"); + # public key crypto my ($pk1, $sk1) = box_keypair(); my ($pk2, $sk2) = box_keypair();

Fri Nov 25 06:27:55 2016 NEWELLC [...] cpan.org - Correspondence added

There are definitely 2 other places that suffer from the same basic problem. I've attached a patch with fixes and tests for them. Both crypto_box_open and crypto_stream_xor can crash with empty strings.

Subject:

patch2.txt

diff --git a/Sodium.xs b/Sodium.xs index 504c6d3..f66c01d 100644 --- a/Sodium.xs +++ b/Sodium.xs @@ -235,7 +235,7 @@ real_crypto_stream_xor(m, clen, n, k) CODE: unsigned char c[clen]; crypto_stream_xor(c, m, clen, n, k); - RETVAL = newSVpv((unsigned char *)c, clen); + RETVAL = newSVpvn((unsigned char *)c, clen); OUTPUT: RETVAL @@ -256,7 +256,7 @@ real_crypto_box_open(c, clen, n, pk, sk) (unsigned long long) clen, (const unsigned char*)n, (const unsigned char*)pk, (const unsigned char*)sk); if (status == 0) { - RETVAL = newSVpv( m, clen - crypto_box_MACBYTES ); + RETVAL = newSVpvn( m, clen - crypto_box_MACBYTES ); } else { RETVAL = &PL_sv_undef; } diff --git a/t/Crypt-Sodium.t b/t/Crypt-Sodium.t index a4beabe..307589f 100644 --- a/t/Crypt-Sodium.t +++ b/t/Crypt-Sodium.t @@ -21,6 +21,7 @@ my $k = crypto_stream_key(); my $n = crypto_stream_nonce(); my $enciphered = crypto_stream_xor($message, $n, $k); is(crypto_stream_xor($enciphered, $n, $k), $message, "Testing roundtrip of crypto_stream_xor"); +is(crypto_stream_xor(crypto_stream_xor('', $n, $k), $n, $k), '', "Testing roundtrip of ''"); # test crypto secret box stuff $k = crypto_stream_key(); @@ -38,6 +39,7 @@ my ($pk2, $sk2) = box_keypair(); $n = crypto_box_nonce(); $enciphered = crypto_box($message, $n, $pk2, $sk1); is(crypto_box_open($enciphered, $n, $pk1, $sk2), $message, "Testing roundtrip of crypto_box"); +is(crypto_box_open(crypto_box('', $n, $pk2, $sk1), $n, $pk1, $sk2), '', "Testing roundtrip of ''"); is(crypto_hash($message), crypto_hash($message), "Testing hash comparison"); is(length(randombytes_buf(24)), 24, "Testing random bytes output");

Mon Dec 12 13:21:02 2016 mike [...] mg2.org - Correspondence added 30 min

Fixed using NEWELLC's patch in dist 0.09 just uploaded to PAUSE.

Mon Dec 12 13:21:03 2016 The RT System itself - Status changed from 'new' to 'open'

Mon Dec 12 13:21:03 2016 mike [...] mg2.org - Status changed from 'open' to 'resolved'

Mon Dec 12 13:21:03 2016 mike [...] mg2.org - Taken