Debian Bug report logs - #694091
bcrypt: Tries to load whole file into memory regardless of the size

Toggle useless messages

Message #5 received at submit@bugs.debian.org (full text, mbox, reply):

From: Margarita Manterola <marga@debian.org>

To: Debian Bug Tracking System <submit@bugs.debian.org>

Subject: bcrypt: Tries to load whole file into memory regardless of the size

Date: Fri, 23 Nov 2012 20:41:24 +0000

Package: bcrypt
Version: 1.1-6
Severity: grave
Tags: lfs upstream

Hi,

The current version of bcrypt reads the whole file into memory by using
fread, regardless of the file size.  This means that for large files, a
machine can run out of memory by trying to just read the file.

The out of memory killer will kill other programs before killing an active
program, thus causing the death of unrelated process.

The solution to this problem would be to put a limit on how much can be
read and encrypt via various subsequent read/writes instead of just one big
read and one big write.

-- 
Regards,
Marga


-- System Information:
Debian Release: wheezy/sid
  APT prefers unstable
  APT policy: (500, 'unstable'), (1, 'experimental')
Architecture: i386 (i686)

Kernel: Linux 3.2.0-0.bpo.2-686-pae (SMP w/2 CPU cores)
Locale: LANG=es_AR.UTF-8, LC_CTYPE=es_AR.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash

Versions of packages bcrypt depends on:
ii  libc6                    2.13-37         Embedded GNU C Library: Shared lib
ii  zlib1g                   1:1.2.7.dfsg-13 compression library - runtime

bcrypt recommends no packages.

bcrypt suggests no packages.

-- no debconf information

Message #10 received at 694091@bugs.debian.org (full text, mbox, reply):

From: Michael Stapelberg <stapelberg@debian.org>

To: Margarita Manterola <marga@debian.org>

Cc: 694091@bugs.debian.org

Subject: Re: bcrypt: Tries to load whole file into memory regardless of the size

Date: Sat, 24 Nov 2012 17:47:17 +0100

Hi,

On Fri, 23 Nov 2012 20:41:24 +0000
Margarita Manterola <marga@debian.org> wrote:
> The solution to this problem would be to put a limit on how much can
> be read and encrypt via various subsequent read/writes instead of
> just one big read and one big write.
I am currently working on making bcrypt compress and encrypt as it goes
along. The encryption is already working, compression, decompression
and decryption are hopefully coming soon.

I’ll keep you updated.

-- 
Best regards,
Michael

Message #15 received at 694091@bugs.debian.org (full text, mbox, reply):

From: Michael Stapelberg <stapelberg@debian.org>

To: Margarita Manterola <marga@debian.org>

Cc: 694091@bugs.debian.org, 693460@bugs.debian.org, control@bugs.debian.org

Subject: Re: bcrypt: Tries to load whole file into memory regardless of the size

Date: Sun, 25 Nov 2012 12:53:25 +0100

[Message part 1 (text/plain, inline)]

tag 694091 + patch
tag 693460 + patch
thanks

Hi,

attached you can find a patch against bcrypt which makes it support
large files (> 2 GiB) and files which exceed the amount of available
RAM.

I tested this with a 128 MiB RAM VM:

root@squeezevm:/tmp# uname -a
Linux squeezevm 2.6.32-5-amd64 #1 SMP Sun Sep 23 10:07:46 UTC 2012 x86_64 GNU/Linux
root@squeezevm:/tmp# free -m
             total       used       free     shared    buffers     cached
Mem:           118         62         56          0         25         17
-/+ buffers/cache:         19         99
Swap:          382          0        382
root@squeezevm:/tmp# dd if=/dev/zero of=bigfile bs=10M count=20
20+0 records in
20+0 records out
209715200 bytes (210 MB) copied, 0.558276 s, 376 MB/s
root@squeezevm:/tmp# ./bcrypt -c bigfile
Encryption key:
Again:
root@squeezevm:/tmp# ls -hltr
total 201M
-rwxr-xr-x 1 root root  28K Nov 25 06:03 bcrypt
-rw-r--r-- 1 root root 201M Nov 25 06:04 bigfile.bfe
root@squeezevm:/tmp# free -m
             total       used       free     shared    buffers     cached
Mem:           118         75         43          0         21         34
-/+ buffers/cache:         20         98
Swap:          382          0        382

Also, here is a large file on an i686 (32-bit) machine:

root@stability /raid $ ls -l bigfile
-rw-r--r-- 1 root root 2621440000 2012-11-25 12:19 bigfile
root@stability /raid $ /home/michael/bcrypt-1.1/bcrypt -c bigfile
Encryption key:
Again:
/home/michael/bcrypt-1.1/bcrypt -c bigfile  188,96s user 1286,90s system 93% cpu 26:11,79 total
root@stability /raid $ ls -l bigfile.bfe  
-rw-r--r-- 1 root root 2621440066 2012-11-25 12:45 bigfile.bfe
root@stability /raid $ uname -a
Linux stability 2.6.30.1 #1 SMP Fri Jul 17 23:19:30 CEST 2009 i686
GNU/Linux

I have verified that the tool still works correctly by using a script.

The script runs 1000 iterations of each test, that
is, it tests that the new version can still read files created by the
old version, and it tests that the old version can still read files
created by the new version. You can find the script in the pull
request linked below.

I also tested that files created on big-endian machines can be
decrypted on little-endian machines and vice-versa.

I have also submitted this patch to the new upstream location at 
https://github.com/casta/bcrypt/pull/1

-- 
Best regards,
Michael

[bcrypt-big-files.patch (text/x-patch, attachment)]

Message #22 received at 694091@bugs.debian.org (full text, mbox, reply):

From: Margarita Manterola <marga@debian.org>

To: Michael Stapelberg <stapelberg@debian.org>

Cc: 694091@bugs.debian.org, 693460@bugs.debian.org

Subject: Re: bcrypt: Tries to load whole file into memory regardless of the size

Date: Sun, 25 Nov 2012 13:16:34 +0100

Hola Michael Stapelberg!

I have a couple of comments regarding your patch:

----------------------------
 /* from wrapbf.c */
-uLong BFEncrypt(char **input, char *key, uLong sz,
-       BCoptions *options);
-uLong BFDecrypt(char **input, char *key, char *key2,
-       uLong sz, BCoptions *options);
+off_t BFEncrypt(int infd, int outfd, char *key, BCoptions *options);
+off_t BFDecrypt(int infd, int outfd, char *key, char *key2, BCoptions
*options);

Please respect the original author indenting style.

+    if (outfd == -1) {
+      perror("open(infile)");
+      continue;
     } 

This should be perror("open(outfile)");

+    } else {
+      if ((sz = BFDecrypt(infd, outfd, key, key2, &options)) == 0) {
         fprintf(stderr, "Invalid encryption key for file: %s\n", infile);
         exit(1);
       }
+      ftruncate(outfd, sz);
+    }

What's the point of the truncate there?
----------------------------

For the complicated part, I would need more time to review it, but I think
it's a too disruptive change to make it into wheezy's release.  It's
basically a complete refactor of the whole encryption/decryption functions.

Also, I'm really not sure it makes much sense to have this tool.  It could
easily be replaced by a shell script that calls openssl and shred
appropriately.

-- 
Regards,
Marga

Message #27 received at 694091@bugs.debian.org (full text, mbox, reply):

From: Michael Stapelberg <stapelberg@debian.org>

To: Margarita Manterola <marga@debian.org>

Cc: 694091@bugs.debian.org, 693460@bugs.debian.org

Subject: Re: bcrypt: Tries to load whole file into memory regardless of the size

Date: Sun, 25 Nov 2012 13:26:45 +0100

Hi Margarita,

Margarita Manterola <marga@debian.org> writes:
> Please respect the original author indenting style.
There hardly is such a thing. The source code as-is uses different
indenting styles. But that is a minor nitpick not worthy of
discussion. Feel free to change my patch as you see fit.

> This should be perror("open(outfile)");
Correct.

> +    } else {
> +      if ((sz = BFDecrypt(infd, outfd, key, key2, &options)) == 0) {
>          fprintf(stderr, "Invalid encryption key for file: %s\n", infile);
>          exit(1);
>        }
> +      ftruncate(outfd, sz);
> +    }
>
> What's the point of the truncate there?
Since I don’t do all the processing in memory anymore, I cannot truncate
the file in memory (as the old code did). Therefore I just write the key
+ padding block to the output file and then truncate it. There is a
comment in the decryption function which explains how to improve this
situation, but since that is not really related to this issue, I didn’t
spend any time on that.

> For the complicated part, I would need more time to review it, but I think
> it's a too disruptive change to make it into wheezy's release.  It's
> basically a complete refactor of the whole encryption/decryption
> functions.
I agree that this is a large change. I tried to convince you that the
change is alright by providing a fuzzing-style automated testing script
which ran on my machine for thousands of iterations and did not find any
regressions.

I have to wonder: Since you seem opposed to fixing this for wheezy, why
did you not update your bugreport accordingly so that my — apparently
unnecessary and useless — work could have been prevented? It cost me
many hours to work on this.

> Also, I'm really not sure it makes much sense to have this tool.  It could
> easily be replaced by a shell script that calls openssl and shred
> appropriately.
I do agree, but that would break at least around 300 installations
(according to popcon). Consider that bcrypt could be part of custom
backup scripts.

In case you want to provide a wrapper script which is binary-compatible
with the original file format, feel free, but I think that is definitely
a too large change for wheezy either ;-).

-- 
Best regards,
Michael

Message #32 received at 694091@bugs.debian.org (full text, mbox, reply):

From: Margarita Manterola <marga@debian.org>

To: Michael Stapelberg <stapelberg@debian.org>

Cc: 694091@bugs.debian.org, 693460@bugs.debian.org

Subject: Re: bcrypt: Tries to load whole file into memory regardless of the size

Date: Sun, 25 Nov 2012 14:14:35 +0100

Hola Michael Stapelberg!

> I have to wonder: Since you seem opposed to fixing this for wheezy, why
> did you not update your bugreport accordingly so that my — apparently
> unnecessary and useless — work could have been prevented? It cost me
> many hours to work on this.

I'm neither the maintainer nor part of the release team, so I have no
authority to decide anything.  However, I thought it was clear from my
first mail that I considered that the package was in a very bad state and
that a fix would be too disruptive for wheezy.  I did want to get the
package fixed for the future, though.

Your patch does seem to work on fixing most of the issues that I had
identified, and you have tested it thoroughly (I haven't run the tests
myself, though), so I guess it would be alright to upload it to unstable.
However, I doubt that such a big change for such a small package would be
granted an unblock for testing.

> I do agree, but that would break at least around 300 installations
> (according to popcon). Consider that bcrypt could be part of custom
> backup scripts.

Well, 293 installations, with "61" votes (regular use).  I really think it
would be a bad idea to have a tool that deletes and shreds files as an
automatic backup tool, but other people might have other ideas...

> In case you want to provide a wrapper script which is binary-compatible
> with the original file format, feel free, but I think that is definitely
> a too large change for wheezy either ;-).

Right, I didn't think about binary compatibility, and I wasn't really
planning on getting a block exception for that one either.  Do you think
it's not possible to achieve binary compatibility using the openssl
command?

-- 
Love,
Marga

Message #37 received at 694091@bugs.debian.org (full text, mbox, reply):

From: Michael Stapelberg <stapelberg@debian.org>

To: Margarita Manterola <marga@debian.org>

Cc: 694091@bugs.debian.org, 693460@bugs.debian.org, Kevin Coyner <kcoyner@debian.org>

Subject: Re: bcrypt: Tries to load whole file into memory regardless of the size

Date: Sun, 25 Nov 2012 18:29:50 +0100

Hi Margarita,

Margarita Manterola <marga@debian.org> writes:
> I'm neither the maintainer nor part of the release team, so I have no
> authority to decide anything.  However, I thought it was clear from my
> first mail that I considered that the package was in a very bad state
> and that a fix would be too disruptive for wheezy.  I did want to get
> the package fixed for the future, though.
If by "first mail" you refer to your message in an entirely unrelated
bugreport which I only found after I had done all the work, then I’d
recommend that you update all reports which might be affected by your
findings in the future :-).

> Your patch does seem to work on fixing most of the issues that I had
> identified, and you have tested it thoroughly (I haven't run the tests
> myself, though), so I guess it would be alright to upload it to unstable.
> However, I doubt that such a big change for such a small package would be
> granted an unblock for testing.
We will never know if we don’t try.

Kevin, what do you think? Could you upload a new version or do you want
us to NMU?

> Well, 293 installations, with "61" votes (regular use).  I really
> think it would be a bad idea to have a tool that deletes and shreds
> files as an automatic backup tool, but other people might have other
> ideas...
Encrypting a backup after creating it is not such a weird idea. The
detail that the tool shreds old data might have been missed or accepted
by our hypothetical backup script writers.

Also keep in mind how popcon votes are counted: It means that people
have bcrypt mapped into memory at the time when popcon runs. For such a
small tool (in contrary to a daemon), the votes are not really helpful.

> Right, I didn't think about binary compatibility, and I wasn't really
> planning on getting a block exception for that one either.  Do you think
> it's not possible to achieve binary compatibility using the openssl
> command?
I am sure that it is not possible to achieve binary compatibility when
using only the openssl cli. You need at least a wrapper which takes care
of the custom file format that bcrypt uses, and when you write that, you
can really just keep bcrypt. It’s not much more than that.

-- 
Best regards,
Michael

Message #42 received at 694091@bugs.debian.org (full text, mbox, reply):

From: Kevin Coyner <kcoyner@debian.org>

To: 694091@bugs.debian.org, Michael Stapelberg <stapelberg@debian.org>

Cc: 693460@bugs.debian.org, Margarita Manterola <marga@debian.org>

Subject: Re: Bug#694091: bcrypt: Tries to load whole file into memory regardless of the size

Date: Sun, 25 Nov 2012 13:16:20 -0500

[Message part 1 (text/plain, inline)]

-- 
Kevin Coyner. GnuPG key: 2048R/C85D8F71
On Nov 25, 2012 12:33 PM, "Michael Stapelberg" <stapelberg@debian.org>
wrote:
>
> > Your patch does seem to work on fixing most of the issues that I had
> > identified, and you have tested it thoroughly (I haven't run the tests
> > myself, though), so I guess it would be alright to upload it to
unstable.
> > However, I doubt that such a big change for such a small package would
be
> > granted an unblock for testing.
> We will never know if we don’t try.
>
> Kevin, what do you think? Could you upload a new version or do you want
> us to NMU?

I'm on the road right now, but would be happy to upload a new version later
tonight or early tomorrow morning.

Kevin


>
> > Well, 293 installations, with "61" votes (regular use).  I really
> > think it would be a bad idea to have a tool that deletes and shreds
> > files as an automatic backup tool, but other people might have other
> > ideas...
> Encrypting a backup after creating it is not such a weird idea. The
> detail that the tool shreds old data might have been missed or accepted
> by our hypothetical backup script writers.
>
> Also keep in mind how popcon votes are counted: It means that people
> have bcrypt mapped into memory at the time when popcon runs. For such a
> small tool (in contrary to a daemon), the votes are not really helpful.
>
> > Right, I didn't think about binary compatibility, and I wasn't really
> > planning on getting a block exception for that one either.  Do you think
> > it's not possible to achieve binary compatibility using the openssl
> > command?
> I am sure that it is not possible to achieve binary compatibility when
> using only the openssl cli. You need at least a wrapper which takes care
> of the custom file format that bcrypt uses, and when you write that, you
> can really just keep bcrypt. It’s not much more than that.
>
> --
> Best regards,
> Michael

[Message part 2 (text/html, inline)]

Message #47 received at 694091@bugs.debian.org (full text, mbox, reply):

From: Michael Stapelberg <stapelberg@debian.org>

To: Kevin Coyner <kcoyner@debian.org>, 694091@bugs.debian.org

Cc: 693460@bugs.debian.org, Margarita Manterola <marga@debian.org>

Subject: Re: Bug#694091: bcrypt: Tries to load whole file into memory regardless of the size

Date: Mon, 26 Nov 2012 14:07:26 +0100

Hi Kevin,

Kevin Coyner <kcoyner@debian.org> writes:
> I'm on the road right now, but would be happy to upload a new version later
> tonight or early tomorrow morning.
Thanks. Please let us know when you are done.

In the meantime, new upstream has merged the fix and also improved error
messages and fixed a memory leak:

https://github.com/casta/bcrypt/commit/64d9a16b158f2872bf3af80c2f01ee400c04c381

-- 
Best regards,
Michael

Message #52 received at 694091@bugs.debian.org (full text, mbox, reply):

From: Michael Stapelberg <stapelberg@debian.org>

To: Kevin Coyner <kcoyner@debian.org>, 694091@bugs.debian.org

Cc: 693460@bugs.debian.org, Margarita Manterola <marga@debian.org>

Subject: Re: Bug#694091: bcrypt: Tries to load whole file into memory regardless of the size

Date: Fri, 30 Nov 2012 10:37:16 +0100

Hi Kevin,

Michael Stapelberg <stapelberg@debian.org> writes:
>> I'm on the road right now, but would be happy to upload a new version later
>> tonight or early tomorrow morning.
> Thanks. Please let us know when you are done.
Any news on that?

If you need any help, please tell us.

-- 
Best regards,
Michael

Message #57 received at 694091-close@bugs.debian.org (full text, mbox, reply):

From: Kevin Coyner <kcoyner@debian.org>

To: 694091-close@bugs.debian.org

Subject: Bug#694091: fixed in bcrypt 1.1-7

Date: Sun, 02 Dec 2012 16:32:29 +0000

Source: bcrypt
Source-Version: 1.1-7

We believe that the bug you reported is fixed in the latest version of
bcrypt, which is due to be installed in the Debian FTP archive.

A summary of the changes between this version and the previous one is
attached.

Thank you for reporting the bug, which will now be closed.  If you
have further comments please address them to 694091@bugs.debian.org,
and the maintainer will reopen the bug report if appropriate.

Debian distribution maintenance software
pp.
Kevin Coyner <kcoyner@debian.org> (supplier of updated bcrypt package)

(This message was generated automatically at their request; if you
believe that there is a problem with it please contact the archive
administrators by mailing ftpmaster@debian.org)


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Format: 1.8
Date: Sun, 02 Dec 2012 14:53:40 +0000
Source: bcrypt
Binary: bcrypt
Architecture: source amd64
Version: 1.1-7
Distribution: unstable
Urgency: low
Maintainer: Kevin Coyner <kcoyner@debian.org>
Changed-By: Kevin Coyner <kcoyner@debian.org>
Description: 
 bcrypt     - Cross platform file encryption utility using blowfish
Closes: 694091
Changes: 
 bcrypt (1.1-7) unstable; urgency=low
 .
   * Added new patch to deal with bcrypt trying to load entire file into memory
     regardless of size. Thanks to Michael Stapelberg. Closes: #694091.
   * Removed unneeded amd_memory patch.
Checksums-Sha1: 
 b530af856d88d32c00a678d6dfc735c5408ccc7f 1315 bcrypt_1.1-7.dsc
 47b763595b73c8ed97383690dd895f7423cfa318 7134 bcrypt_1.1-7.diff.gz
 588dcee3f6cbc81a78d0dccf2c40ac5e6c9c7c4d 20132 bcrypt_1.1-7_amd64.deb
Checksums-Sha256: 
 02b37941fcc776263baeedcdc9f8da4b92b8f23d242e0686c7373af47385ef79 1315 bcrypt_1.1-7.dsc
 5879d21b9cec1d4c723cd88303cf3dde3c310ec25ecb8c01b1e99010ab9a3402 7134 bcrypt_1.1-7.diff.gz
 e1e21274948bd23af39ec93050608469cba279a362937acd404601147b80bebe 20132 bcrypt_1.1-7_amd64.deb
Files: 
 3fe988c2ae554f015a2e8477216a9f9b 1315 utils optional bcrypt_1.1-7.dsc
 0a6ffda97851a6eedba768b2da2dee5c 7134 utils optional bcrypt_1.1-7.diff.gz
 771413d79d56fd0037258ca2e2aaee83 20132 utils optional bcrypt_1.1-7_amd64.deb

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)

iQEcBAEBCAAGBQJQu4FoAAoJEOuzGmLIXY9xeSQH/jFqwLy62Q6MrD76TzLAVu9x
BwhC1/Q852lvlaQcL91fhrmZdMN+pZFUmB8y+xFwvKcWfAxre3NLgpjMpZ0V7eiW
9HkShomLjMIJps0vPkXMWg/CYps5u8TNUHK7YlbxDDW716Wij7yLi43cGBJW7Wuz
p7g7C6+KELraNHBpY8y26jCKrcFH5GMoszIW3HRc71oN1sNglhH9fed8K+8Wq2Jb
z9Ut+aYWgVVzExFk7Byf+7gcH6jWwRsBqGul4Unmd0DLPSsY8K0odqz0QpCv5nht
GyGatQvdbpQiJdZAS8mw4YSfYpwyvlPMHhVZWURZTzH4cpzBYOCKe/RwSq7BpfI=
=LW9D
-----END PGP SIGNATURE-----

Send a report that this bug log contains spam.