module version 3.01 and up breaks Twofish encryption somehow

[TLINDEN]

Hi,

I am the author of Crypt::PWSafe3, a module to read and write passwordsafe3 files. Until 2.33 of Crypt::CBC everything worked as expected, but starting with 3.01 (and up) it doesn't anymore. There's an issue in my module (TLINDEN/Crypt--PWSafe3#12) and another on RT for yours (https://rt.cpan.org/Public/Bug/Display.html?id=134355).

The problem is, if I remove all references to -blocksize, then it should work, since Crypt::Twofish propagates a blocksize of 16, which I had in my module as well. So, removing this call should fix it. But it doesn't. pwsafe3 files created with Crypt::CBC >= 3.01 are not decryptable with Crypt::CBC >= 3.01 anymore. However, files created with Crypt::CBC < 3.01 are still readable and stay intact as long one doesn't edit them (which re-creates the file, now written with 3.01, which can't be decrypted).

I don't know if anything else has changed. But I looked into the git history of the module and discovered that you made a refactoring of the code. Maybe somehing went wrong during the process.

If you want to reproduce the problem, checkout https://github.com/TLINDEN/Crypt--PWSafe3, comment out all 3 references to -blocksize in lib/Crypt/PWSafe3.pm and run perl Makefile.PL && make test. You'll realize that test 2 succeeds, it opens the supplied 9 year old pwsafe3 file successfully. Everything breaks apart from test 3 and following, they all create new pwsafe3 files.

[pghmcfc]

This seems to be related to the behaviour of Crypt::CBC when encrypting an empty block.
If I make this change to Crypt::CBC:

diff -up ./lib/Crypt/CBC.pm.pws3 ./lib/Crypt/CBC.pm
--- ./lib/Crypt/CBC.pm.pws3	2022-02-16 14:47:01.022412974 +0000
+++ ./lib/Crypt/CBC.pm	2022-02-16 14:52:27.617799760 +0000
@@ -252,7 +252,7 @@ sub finish (\$) {
 	$result = $self->{padding}->($result,$bs,'d')        if $self->_needs_padding;
     } else {
 	$block = $self->{padding}->($block,$bs,'e')          if $self->_needs_padding;
-	$self->$code($self->{crypt},\$iv,\$result,[$block])  unless length $block==0 && !$self->_needs_padding
+	$self->$code($self->{crypt},\$iv,\$result,[$block])  unless length $block==0;
     }
         
     delete $self->{'civ'};

Then the Crypt-PWSafe3 test runs OK (assuming the -blocksize options are removed as discussed earlier). However, Crypt-CBC's own test suite then fails:

$ make test
PERL_DL_NONLAZY=1 "/usr/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'blib/lib', 'blib/arch')" t/*.t
t/AES.t .............. ok
t/Blowfish.t ......... ok
t/Blowfish_PP.t ...... ok
t/CAST5.t ............ skipped: Crypt::CAST5 not installed
t/DES.t .............. ok
input must be 8 bytes long at /usr/lib64/perl5/vendor_perl/Crypt/Blowfish.pm line 58.
t/func.t ............. 
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 823/1537 subtests 
t/IDEA.t ............. ok
using Crypt::Cipher::AES for testing
t/nopadding.t ........ ok
t/null_data.t ........ ok
t/OFB.t .............. ok
using Crypt::Cipher::AES for testing
t/onezeropadding.t ... ok
t/parameters.t ....... ok
t/pbkdf.t ............ ok
t/PCBC.t ............. ok
Using Crypt::Cipher::AES for test
t/preexisting.t ...... ok
t/Rijndael.t ......... ok
t/Rijndael_compat.t .. ok

Test Summary Report
-------------------
t/func.t           (Wstat: 65280 Tests: 716 Failed: 2)
  Failed tests:  205, 461
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 1537 tests but ran 716.
Files=17, Tests=4238, 15 wallclock secs ( 0.10 usr  0.01 sys + 14.66 cusr  0.22 csys = 14.99 CPU)
Result: FAIL
Failed 1/17 test programs. 2/4238 subtests failed.

At this point I'm not sure whether the problem is in Crypt-CBC or Crypt-PWSafe3. It seems reasonable to me that a padded empty block (which in the case of Crypt-PWSafe3 will just be an empty block anyway since a null-padded empty block is an empty block) should be encrypted, which is what Crypt-CBC currently does.

[TLINDEN]

K, I changed the padding method to rijndael_compat (TLINDEN/Crypt--PWSafe3@e33d702)

now all my unit tests run successful:

% make test
Skip blib/lib/Crypt/PWSafe3/Databaseformat.pm (unchanged)
Skip blib/lib/Crypt/PWSafe3/Record.pm (unchanged)
cp lib/Crypt/PWSafe3.pm blib/lib/Crypt/PWSafe3.pm
Skip blib/lib/Crypt/PWSafe3/SHA256.pm (unchanged)
Skip blib/lib/Crypt/PWSafe3/Field.pm (unchanged)
Skip blib/lib/Crypt/PWSafe3/PasswordPolicy.pm (unchanged)
Skip blib/lib/Crypt/PWSafe3/HeaderField.pm (unchanged)
PERL_DL_NONLAZY=1 "/home/*****/perl5/perlbrew/perls/perl-5.34.0/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; 
test_harness(0, 'blib/lib', 'blib/arch')" t/*.t
t/run.t .. 1/13 # 3 done
t/run.t .. ok     
All tests successful.
Files=1, Tests=13,  1 wallclock secs ( 0.03 usr  0.01 sys +  0.40 cusr  0.08 csys =  0.52 CPU)
Result: PASS

I didn't test this code against the older Crypt::CBC but suspect it'd work with it too. I'll need a couple more tests, esp using the so created pwsafe3 files with the android app, just to make sure it's still interoperable.

[lstein]

Hi, There was a bug in the Crypt-CBC padding methods that was causing issues with OpenSSL. My fix probably is responsible for breaking PWSafe3. I will go back into the changelogs to refresh my memory about what exactly I changed, but I'm pretty sure the implementation is now correct. Lincoln

…

On Sun, Feb 27, 2022 at 7:57 AM T.v.Dein ***@***.***> wrote: K, I changed the padding method to rijndael_compat ( ***@***.*** <TLINDEN/Crypt--PWSafe3@e33d702> ) now all my unit tests run successful: % make test Skip blib/lib/Crypt/PWSafe3/Databaseformat.pm (unchanged) Skip blib/lib/Crypt/PWSafe3/Record.pm (unchanged) cp lib/Crypt/PWSafe3.pm blib/lib/Crypt/PWSafe3.pm Skip blib/lib/Crypt/PWSafe3/SHA256.pm (unchanged) Skip blib/lib/Crypt/PWSafe3/Field.pm (unchanged) Skip blib/lib/Crypt/PWSafe3/PasswordPolicy.pm (unchanged) Skip blib/lib/Crypt/PWSafe3/HeaderField.pm (unchanged) PERL_DL_NONLAZY=1 "/home/*****/perl5/perlbrew/perls/perl-5.34.0/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'blib/lib', 'blib/arch')" t/*.t t/run.t .. 1/13 # 3 done t/run.t .. ok All tests successful. Files=1, Tests=13, 1 wallclock secs ( 0.03 usr 0.01 sys + 0.40 cusr 0.08 csys = 0.52 CPU) Result: PASS I didn't test this code against the older Crypt::CBC but suspect it'd work with it too. I'll need a couple more tests, esp using the so created pwsafe3 files with the android app, just to make sure it's still interoperable. — Reply to this email directly, view it on GitHub <#5 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA3EVPPF7LFE23DFF7TLYLU5INTJANCNFSM5OLIYK2A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-- *Lincoln Stein* Head, Adaptive Oncology, OICR Senior Principal Investigator, OICR Professor, Department of Molecular Genetics, University of Toronto Tel: 416-673-8514 Cell: 416-817-8240 ***@***.*** *E**xecutive Assistant* Michelle Xin Tel: 647-260-7927 ***@***.*** ***@***.***>* *Ontario Institute for Cancer Research* MaRS Centre, 661 University Avenue, Suite 510, Toronto, Ontario, Canada M5G 0A3 @OICR_news <https://can01.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftwitter.com%2Foicr_news&data=04%7C01%7CMichelle.Xin%40oicr.on.ca%7C9fa8636ff38b4a60ff5a08d926dd2113%7C9df949f8a6eb419d9caa1f8c83db674f%7C0%7C0%7C637583553462287559%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=PS9KzggzFoecbbt%2BZQyhkWkQo9D0hHiiujsbP7Idv4s%3D&reserved=0> | www.oicr.on.ca *Collaborate. Translate. Change lives.* This message and any attachments may contain confidential and/or privileged information for the sole use of the intended recipient. Any review or distribution by anyone other than the person for whom it was originally intended is strictly prohibited. If you have received this message in error, please contact the sender and delete all copies. Opinions, conclusions or other information contained in this message may not be that of the organization.

[TLINDEN]

Hi Lincoln,

I don't know if this helps, but I did a git bisect session with your repo and my unit tests. According to bisect, df47ba1 is the commit that introduced the change which makes Crypt::PWSafe3 fail:

[08.Mar 19:05:19] --- [~/dev/Lib-Crypt-CBC] ---
scip@tripod: % git bisect bad
df47ba1877968762889165fe0c8899422e5ccb2f is the first bad commit
commit df47ba1877968762889165fe0c8899422e5ccb2f
Author: Lincoln Stein <lincoln.stein@gmail.com>
Date:   Fri Feb 5 09:02:17 2021 -0500

    all regression tests pass; additional testing needed

 MANIFEST                        |   3 +-
 lib/Crypt/CBC.pm                | 258 ++++++++++++++++++++--------------------
 lib/Crypt/CBC/PBKDF/none.pm     |   4 +-
 lib/Crypt/CBC/PBKDF/nosalt.pm   |  34 ------
 lib/Crypt/CBC/PBKDF/randomiv.pm |  36 ++++++
 lib/Crypt/CBC/PBKDF/sslv1.pm    |  33 -----
 t/parameters.t                  | 222 ++++++++++++++++++++++------------
 7 files changed, 312 insertions(+), 278 deletions(-)
 delete mode 100644 lib/Crypt/CBC/PBKDF/nosalt.pm
 create mode 100644 lib/Crypt/CBC/PBKDF/randomiv.pm
 delete mode 100644 lib/Crypt/CBC/PBKDF/sslv1.pm

However, what do you think about the change I made (-padding => 'rijndael_compat'), could I leave it? Because in this case I could just release another version and fix my own bug which in turn would then enable the redhat guys to close theirs. Or should I wait?

Thanks and best regards,
Tom

best regards,
Tom

[TLINDEN]

Anything new on this?