Padding removal went wrong

[ofer]

Hello again
I wrote an encryption program, based on Crypt::Blowfish.
This is a block cipher, which means that I have to encrypt 8 bytes each
time.
The problem is that I want to encrypt strings which are not exactly 8
bytes, or a whole multiplication of 8.
What I did was to pad the last, incomplete buffer.
If I have this string:
XXXX
I just pad the buffer like this:
XXXX4444
some more examples:
XX -> XX666666
XXX -> XXX55555
X -> X7777777
XXXXXXX -> XXXXXXX1

I hope you get it.

Okay, so that went great.
Now I want to decrypt it. So first I decrypt, and then I remove the
padding.

----------------------------------------------------------------------------------------
while ( length( $block ) < $block_size ) {
my $did_read = read( IN, $block, $block_size - length( $block ),
length( $block ) );
defined( $did_read ) || die( "Reading from $fn failed ($!)" );
$did_read == 0 && do {
$last_block = 1;
};
}

# Decrypt block
$block_out = $cipher->decrypt( $block );
# Remove padding if last block
$last_block && do {
my $pad_len = substr( $block_out, -1 );
substr( $block_out, -$pad_len ) = "";
}
----------------------------------------------------------------------------------------

I have pasted only the relevant parts of course.
The first part is here in order to show you what is $last_block and how
I read from the file, and the second part is the decryption and padding
removal.

I can't explain what is the problem exactly, but I know what the
padding is not removed as it finishes.
Can you please help me?
Thank you very much!
And sorry that I don't give you a full functional program, I just
thought that it won't be neccesary because it is a logical problem
which can be solved by only looking at the code. If you need anything
else please let me know

 

[Anno Siegel]

Hello again
I wrote an encryption program, based on Crypt::Blowfish.

[non-runnable code snipped]

And sorry that I don't give you a full functional program, I just
thought that it won't be neccesary because it is a logical problem
which can be solved by only looking at the code.

That won't do. Even if the solution is found by looking at the code,
(most are, quite generally), we'd want to run it for verification and
debugging. So we'd have to make a runnable version anyway, one for each
replier. I'm not going there.

Anno

 

[Aaron Baugher]

The problem is that I want to encrypt strings which are not exactly 8
bytes, or a whole multiplication of 8.
What I did was to pad the last, incomplete buffer.
If I have this string:
XXXX
I just pad the buffer like this:
XXXX4444
some more examples:
XX -> XX666666
XXX -> XXX55555
X -> X7777777
XXXXXXX -> XXXXXXX1

I hope you get it.

Okay, so that went great.
Now I want to decrypt it. So first I decrypt, and then I remove the
padding.

Your code that removes the padding works fine:

my $block_out = 'XXX55555'; # my line
my $pad_len = substr( $block_out, -1 );
substr( $block_out, -$pad_len ) = "";
print "\$pad_len = $pad_len, \$block_out = $block_out\n"; # my line

results in:

$pad_len = 5, $block_out = XXX

So either $block_out doesn't contain what you think it does, or
$last_block isn't getting set and your code isn't getting executed.
Stick a print line in there, like 'print "Removing padding from
$block_out\n"', and see if/what it prints. I'm guessing your problem
is that $last_block isn't being set in this earlier code when you
think it is:

my $did_read = read( IN, $block, $block_size - length( $block ),
length( $block ) );
defined( $did_read ) || die( "Reading from $fn failed ($!)" );
$did_read == 0 && do {
$last_block = 1;
};

because your file contains an even multiple of 8 bytes, so end-of-file
will be reached *after* you read that last block of 8.

By the way, that's some pretty painful code. It'd be much more
perlish to replace the last three lines with:

$last_block = 1 unless $did_read;

However, it'd still be broken. In fact, that whole part is a mess;
I'd scrap it and start over.

 

[ofer]

Hello again
I wrote an encryption program, based on Crypt::Blowfish.

[non-runnable code snipped]

And sorry that I don't give you a full functional program, I just
thought that it won't be neccesary because it is a logical problem
which can be solved by only looking at the code.

That won't do. Even if the solution is found by looking at the code,
(most are, quite generally), we'd want to run it for verification and
debugging. So we'd have to make a runnable version anyway, one for each
replier. I'm not going there.

Okay, sorry. In the end of this post you can find runnable version

Anno
--
If you want to post a followup via groups.google.com, don't use
the broken "Reply" link at the bottom of the article. Click on
"show options" at the top of the article, then click on the
"Reply" at the bottom of the article headers.

got it.

====================================================
Runnable version notes:
1. I know I should use a module for the command line options. I just
wanted this fast hack for the moment. I will change it soon. For now,
this is not the problem anyway.
2. the usage() subroutine is not implemented, because it is very long
and irrelevant.
3. use this like this: ./script.pl [-e|-d]
[password|filename-for-output] [filename]
I will give 3 examples:
A. ./script.pl -e 12345678 somefile
B. ./script.pl -d 12345678 somefile
C. ./script.pl -e 12345678

Version C will read from STDIN. That's because I want to pipe it.. like
this:
cat file | myscript.pl -e 12345678 | tar or whatever

Runnable version:

#!/usr/bin/perl -s

use warnings;
use strict;
use Crypt::Blowfish;

# Get action from command line
our( $d, $e );
my $action = ( $d ? "decrypt" : $e ? "encrypt" : undef );
defined( $action ) || usage();

# Get password and optional input file name from command line
@ARGV == 1 || @ARGV == 2 || usage();
my ( $password, $fn ) = @ARGV;

# Open input and output files or dup stdin and stdout if no input file
name
my $fn_out;
if ( defined( $fn ) ) {
open( IN, $fn ) || die "Cannot open $fn for reading ($!)";
my $fn_out = "$fn.out";
open( OUT, ">$fn_out" ) || die "Cannot open $fn_out for writing
($!)";
} else {
( $fn, $fn_out ) = ( "stdin", "stdout" );
open( IN, "<&STDIN" ) || die "Cannot dup stdin ($!)";
open( OUT, ">&STDOUT" ) || die "Cannot dup stdout ($!)";
}

# Create cipher object
my $cipher = new Crypt::Blowfish ( $password ) or die "Cannot create
cipher object";

# Get cipher block size
my $block_size = $cipher->blocksize();

# Process input file
my $last_block;
while ( !$last_block ) {
# Read next block, or block fragment if EOF
my $block = "";
while ( length( $block ) < $block_size ) {
my $did_read = read( IN, $block, $block_size - length( $block
), length( $block ) );
defined( $did_read ) || die( "Reading from $fn failed ($!)" );
$last_block = 1 unless $did_read;
}

length( $block ) == 0 && last;

# print STDERR "read " . length( $block ) . " bytes\n";

my $block_out;
if ( $action eq "encrypt" ) {
# Pad block if last
$last_block && do {
my $pad_len = $block_size - length( $block );
$block .= ( $pad_len x $pad_len );
};

# Encrypt block
$block_out = $cipher->encrypt( $block );
} else {
# Decrypt block
$block_out = $cipher->decrypt( $block );

# Remove padding if last block
$last_block && do {
my $pad_len = substr( $block_out, -1 );
substr( $block_out, -$pad_len ) = "";
};
}
# Write to output file
print ( OUT $block_out ) || die "print() ($!)";
}
close( IN );
close( OUT );

 

[ofer]

Your code that removes the padding works fine:

my $block_out = 'XXX55555'; # my line
my $pad_len = substr( $block_out, -1 );
substr( $block_out, -$pad_len ) = "";
print "\$pad_len = $pad_len, \$block_out = $block_out\n"; # my line

results in:

$pad_len = 5, $block_out = XXX

So either $block_out doesn't contain what you think it does, or
$last_block isn't getting set and your code isn't getting executed.
Stick a print line in there, like 'print "Removing padding from
$block_out\n"', and see if/what it prints. I'm guessing your problem
is that $last_block isn't being set in this earlier code when you
think it is:

my $did_read = read( IN, $block, $block_size - length( $block ),
length( $block ) );
defined( $did_read ) || die( "Reading from $fn failed ($!)" );
$did_read == 0 && do {
$last_block = 1;
};

because your file contains an even multiple of 8 bytes, so end-of-file
will be reached *after* you read that last block of 8.

Yeah, okay. I get the problem.
So what are you suggesting?
I thought to keep the last 8 bytes in a variable, and after EOF, to act
on that variable.
What do you think?

Thank you!

 

[ofer]

[problem padding and unpadding plain text block]

Yeah, okay. I get the problem.
So what are you suggesting?
I thought to keep the last 8 bytes in a variable, and after EOF, to act
on that variable.
What do you think?

I think you should not re-invent existing code. Use Crypt::CBC (Cipher
Block Chaining) module:

#!/usr/local/bin/perl
use strict;
use warnings;
use Crypt::CBC;

I don't want to use CBC, because then I lose the main benefits of a
block cipher.
And I don't have a clue what are you talking about. I was not trying to
implement CBC, I was trying to use the block cipher as-is. CBC is not
what I wanted, and not what I've wrote.
You can read about CBC here:
http://en.wikipedia.org/wiki/Cipher-block_chaining
If one byte in the file was corrupted, the whole file is corrupted, and
I can not encrypt more than 1 block at time, because I need
information from the last block
I could just use stream cipher if I wanted that to happen... But I want
a recovery option and parallel encryption and decryption.

Note: the program you posted was too long and contained too much
irrelevant and archaic code, such as the -s flag to parse command-line
arguments, and was missing the usage() subroutine. I gave up trying to
get it to work. If you want more and better help in the future, please
comply with the guidelines for this newsgroup, which are posted here
every 3 days.

This program works just fine. I already explained why usage() is
missing, and I wrote the instructions with examples for people who
don't understand how to use the program.
If you "gave up trying" a working program, I guess you can't help me at
all, but thank you for trying anyway!
If you don't want to help - you don't have to, but don't complain that
my program ain't running, because it is. I don't think I can give
anything beyond PERFECTLY UNDERSTOOD instructions, and code that works
PERFECTLY FINE.

Thanks again and have a good day.