Re: [PATCH] Initial Vector Fix for loop.c.

Re: [PATCH] Initial Vector Fix for loop.c.

[Andi Kleen]

Fruhwirth Clemens <clemens-dated-1056963973.bf26@endorphin.org> writes:

> So go for it. Fix it before 2.6.x is out and we're stuck with this crap
> again. 

This will break existing crypto loop installations, making
the existing encrypted image unreadable. After all this is Linux
here, not HackOS where you can break user file system formats at will.
The only way to implement this is with a new flag that is set by losetup
and keep the old behaviour by default.

-Andi

Re: [PATCH] Initial Vector Fix for loop.c.

[Fruhwirth Clemens]

[-- Attachment #1: Type: text/plain, Size: 1114 bytes --]

On Fri, Jun 20, 2003 at 11:30:03AM +0200, Andi Kleen wrote:
> Fruhwirth Clemens <clemens-dated-1056963973.bf26@endorphin.org> writes:
> 
> > So go for it. Fix it before 2.6.x is out and we're stuck with this crap
> > again. 
> 
> This will break existing crypto loop installations, making
> the existing encrypted image unreadable. After all this is Linux
> here, not HackOS where you can break user file system formats at will.
> The only way to implement this is with a new flag that is set by losetup
> and keep the old behaviour by default.

There is no cryptoloop installation which is affected by this. Read my mail
properly. Every cryptoloop setup out there uses loop-AES or kerneli's
patch-int. And both fixed this issue a _long_ time ago. (Have a look at
the linux-crypto archives of 2001).

Adam Richter's version of loop.c which is in his ygg tree is quite nice,
but his changes are HUGE. Nobody is going to split them up in small patches.

Again: _no_ userbase is affected by this change. Every userbase which
could have ever been affected has done the fix for itself.

Clemens

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] Initial Vector Fix for loop.c.

[Andi Kleen]

On Fri, Jun 20, 2003 at 12:14:52PM +0200, Fruhwirth Clemens wrote:
> On Fri, Jun 20, 2003 at 11:30:03AM +0200, Andi Kleen wrote:
> > Fruhwirth Clemens <clemens-dated-1056963973.bf26@endorphin.org> writes:
> > 
> > > So go for it. Fix it before 2.6.x is out and we're stuck with this crap
> > > again. 
> > 
> > This will break existing crypto loop installations, making
> > the existing encrypted image unreadable. After all this is Linux
> > here, not HackOS where you can break user file system formats at will.
> > The only way to implement this is with a new flag that is set by losetup
> > and keep the old behaviour by default.
> 
> There is no cryptoloop installation which is affected by this. Read my mail
> properly. Every cryptoloop setup out there uses loop-AES or kerneli's
> patch-int. And both fixed this issue a _long_ time ago. (Have a look at

That's completely wrong. I know of several independent implementation
and installations.

> Again: _no_ userbase is affected by this change. Every userbase which
> could have ever been affected has done the fix for itself.

That's also incorrect.

-Andi

Re: [PATCH] Initial Vector Fix for loop.c.

[Fruhwirth Clemens]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

On Fri, Jun 20, 2003 at 12:24:55PM +0200, Andi Kleen wrote:
> On Fri, Jun 20, 2003 at 12:14:52PM +0200, Fruhwirth Clemens wrote:
> > There is no cryptoloop installation which is affected by this. Read my mail
> > properly. Every cryptoloop setup out there uses loop-AES or kerneli's
> > patch-int. And both fixed this issue a _long_ time ago. (Have a look at
> 
> That's completely wrong. I know of several independent implementation
> and installations.

Can't you just name them?

Clemens

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] Initial Vector Fix for loop.c.

[Jörn Engel]

On Fri, 20 June 2003 12:24:55 +0200, Andi Kleen wrote:
> On Fri, Jun 20, 2003 at 12:14:52PM +0200, Fruhwirth Clemens wrote:
> > On Fri, Jun 20, 2003 at 11:30:03AM +0200, Andi Kleen wrote:
> > > Fruhwirth Clemens <clemens-dated-1056963973.bf26@endorphin.org> writes:
> > > 
> > > > So go for it. Fix it before 2.6.x is out and we're stuck with this crap
> > > > again. 
> > > 
> > > This will break existing crypto loop installations, making
> > > the existing encrypted image unreadable. After all this is Linux
> > > here, not HackOS where you can break user file system formats at will.
> > > The only way to implement this is with a new flag that is set by losetup
> > > and keep the old behaviour by default.
> > 
> > There is no cryptoloop installation which is affected by this. Read my mail
> > properly. Every cryptoloop setup out there uses loop-AES or kerneli's
> > patch-int. And both fixed this issue a _long_ time ago. (Have a look at
> 
> That's completely wrong. I know of several independent implementation
> and installations.

That leaves the question of what the default behaviour should be.  If
we have to switch to 512Byte in the long run anyway, there is little
point in postponing the pain.  Make it the default, and old behaviour
depends on the flag.

If we can avoid the pain completely, use that better fix instead, even
if it isn't ready before 2.7, and ignore the problem until then.

Can we?

Jörn

-- 
Fantasy is more important than knowlegde. Knowlegde is limited,
while fantasy embraces the whole world.
-- Albert Einstein

Re: [PATCH] Initial Vector Fix for loop.c.

[Andi Kleen]

> That leaves the question of what the default behaviour should be.  If
> we have to switch to 512Byte in the long run anyway, there is little
> point in postponing the pain.  Make it the default, and old behaviour
> depends on the flag.

In my opinion it doesn't make much difference. crypto-loop 
has broken beyond belief[1] IV anyways, so they will
eventually need to change it. Or just use CBC, which is simpler 
and compatible and has nearly equivalent security to the easily 
predictable IV :-) And when they change it they can as well set the flag.

Also I think Clemens is exaggerating the problem too. 
The old 2.2 behaviour of using absolute IVs caused quite
some problems, but the relative IVs used in 2.4 are
not that bad because it is near always used with 4K 
blocks (there are exceptions to this, but they're quite
rare assuming your file systems are all big enough 
and you don't use a S390) 

-Andi

[1] the problem is that it is too predictable. consider block 0,
which is usually filled with zeros. It also has IV==0. This means
it it 100% equivalent to CBC and worse even has known plain text.
Same problem applies to other blocks - the layout of most 
installations generated by standard installers is quite predictible.
Fixing it is simple, but requires a new secret per file system.

Re: [PATCH] Initial Vector Fix for loop.c.

[Andi Kleen]

Typo in there.

On Fri, Jun 20, 2003 at 12:49:53PM +0200, Andi Kleen wrote:
> In my opinion it doesn't make much difference. crypto-loop 
> has broken beyond belief[1] IV anyways, so they will
> eventually need to change it. Or just use CBC, which is simpler 
                                            ^^^
											should be ECB of course.
											
> 
> [1] the problem is that it is too predictable. consider block 0,
> which is usually filled with zeros. It also has IV==0. This means
> it it 100% equivalent to CBC and worse even has known plain text.
                           ^^^
						   also ECB here.

-Andi

Re: [PATCH] Initial Vector Fix for loop.c.

[Fruhwirth Clemens]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

On Fri, Jun 20, 2003 at 12:49:53PM +0200, Andi Kleen wrote:

Comment: [1] stands for cryptoloop's CBC mode.

> [1] the problem is that it is too predictable. consider block 0,
> which is usually filled with zeros. It also has IV==0. This means
> it it 100% equivalent to CBC and worse even has known plain text.
> Same problem applies to other blocks - the layout of most 
> installations generated by standard installers is quite predictible.
> Fixing it is simple, but requires a new secret per file system.

Adding another secret doesn't improve security in that case. 
Of course the first block is vulnerable to known plaintext attacks, but you
can only prevent those if you make the IV dependend on another secret.. the
key for example. But then you could have also just increased the key size,
which somehow automatically leads to the conclusion: the key is the only
secret which matters. You don't add security if you split the secret.

Clemens



[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] Initial Vector Fix for loop.c.

[Fruhwirth Clemens]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

On Fri, Jun 20, 2003 at 12:35:38PM +0200, Jörn Engel wrote:
> On Fri, 20 June 2003 12:24:55 +0200, Andi Kleen wrote:
> > On Fri, Jun 20, 2003 at 12:14:52PM +0200, Fruhwirth Clemens wrote:
> > > There is no cryptoloop installation which is affected by this. Read my mail
> > > properly. Every cryptoloop setup out there uses loop-AES or kerneli's
> > > patch-int. And both fixed this issue a _long_ time ago. (Have a look at
> > 
> > That's completely wrong. I know of several independent implementation
> > and installations.
> 
> That leaves the question of what the default behaviour should be.  If
> we have to switch to 512Byte in the long run anyway, there is little
> point in postponing the pain.  Make it the default, and old behaviour
> depends on the flag.

Let's see. If there is flag based fix and we make the old behavior default
we will trick many new users into using the old broken IV metric. If we make
the new metric default the user can't mount his old images. 

As there is no autodetection for default behavior, there will be no way to
avoid the pain. Well, almost. We could allocate a new major for a different
metric, and create  /dev/loop-ng*. That'd be feasible, but actually just a
pain transfer .. from the user to the developer :)

> If we can avoid the pain completely, use that better fix instead, even
> if it isn't ready before 2.7, and ignore the problem until then.

No please, I don't wanna patch my kernel for another 2 years. Andrew Morton
is right when he puts this issue on his must-fix list.

Clemens

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] Initial Vector Fix for loop.c.

[Jörn Engel]

On Fri, 20 June 2003 12:51:20 +0200, Fruhwirth Clemens wrote:
> 
> Let's see. If there is flag based fix and we make the old behavior default
> we will trick many new users into using the old broken IV metric. If we make
> the new metric default the user can't mount his old images. 

Unless he starts thinking and reads up how it works in 2.6.

> As there is no autodetection for default behavior, there will be no way to
> avoid the pain. Well, almost. We could allocate a new major for a different
> metric, and create  /dev/loop-ng*. That'd be feasible, but actually just a
> pain transfer .. from the user to the developer :)

I don't see a problem with that, assuming there are more users than
developers. :)

Jörn

-- 
Do not stop an army on its way home.
-- Sun Tzu

Re: [PATCH] Initial Vector Fix for loop.c.

[Jan Rychter]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

Fruhwirth Clemens <clemens@endorphin.org> writes:
> On Fri, Jun 20, 2003 at 12:35:38PM +0200, Jörn Engel wrote:
[...]
> > If we can avoid the pain completely, use that better fix instead,
> > even if it isn't ready before 2.7, and ignore the problem until
> > then.
> No please, I don't wanna patch my kernel for another 2
> years. Andrew Morton is right when he puts this issue on his
> must-fix list.

FWIW, I've been patching my kernels for who knows how long now with this
fix. And I know other people who have been doing the same.

Clemens is right, it's really needed.

--J.

[-- Attachment #2: Type: application/pgp-signature, Size: 188 bytes --]

Re: [PATCH] Initial Vector Fix for loop.c.

[Bill Davidsen]

On Fri, 20 Jun 2003, Andi Kleen wrote:

> On Fri, Jun 20, 2003 at 12:14:52PM +0200, Fruhwirth Clemens wrote:

> > There is no cryptoloop installation which is affected by this. Read my mail
> > properly. Every cryptoloop setup out there uses loop-AES or kerneli's
> > patch-int. And both fixed this issue a _long_ time ago. (Have a look at
> 
> That's completely wrong. I know of several independent implementation
> and installations.

Could you point to these implementations?

> 
> > Again: _no_ userbase is affected by this change. Every userbase which
> > could have ever been affected has done the fix for itself.
> 
> That's also incorrect.

I think the point is that if moving to another device will really break
data, then this is a good time to fix the problem. Breaking things is
allowed, look at modules.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] Initial Vector Fix for loop.c.

[Andrew Morton]

Andi Kleen <ak@suse.de> wrote:
>
> > So go for it. Fix it before 2.6.x is out and we're stuck with this crap
>  > again. 
> 
>  This will break existing crypto loop installations, making
>  the existing encrypted image unreadable.

I think we should just live with that breakage Andi.  You're suggesting
that we retain compatibility with something which was never merged into the
kernel.  That is asking too much.

I'd prefer to see a good, clean, solid implementation of cryptoloop. 
Presumably migration tools from old to new will turn up.

Re: [PATCH] Initial Vector Fix for loop.c.

[David S. Miller]

On Fri, 2003-06-20 at 10:56, Andrew Morton wrote:
> Andi Kleen <ak@suse.de> wrote:
> >
> > > So go for it. Fix it before 2.6.x is out and we're stuck with this crap
> >  > again. 
> > 
> >  This will break existing crypto loop installations, making
> >  the existing encrypted image unreadable.
> 
> I think we should just live with that breakage Andi.  You're suggesting
> that we retain compatibility with something which was never merged into the
> kernel.  That is asking too much.

There was effectively no cryptoloop support in the vanilla
kernel.  Andi is totally right here.  We should be compatible
with what people actually used, which were the external cryptoloop
patches.

Nobody, and I mean nobody, has a cryptoloop based upon the IV
selection done in vanilla kernels.

Re: [PATCH] Initial Vector Fix for loop.c.

[Andries Brouwer]

On Fri, Jun 20, 2003 at 11:14:14AM -0700, David S. Miller wrote:
> On Fri, 2003-06-20 at 10:56, Andrew Morton wrote:
> > Andi Kleen <ak@suse.de> wrote:
> > >
> > > > So go for it. Fix it before 2.6.x is out and we're stuck with this crap
> > >  > again. 
> > > 
> > >  This will break existing crypto loop installations, making
> > >  the existing encrypted image unreadable.
> > 
> > I think we should just live with that breakage Andi.  You're suggesting
> > that we retain compatibility with something which was never merged into the
> > kernel.  That is asking too much.
> 
> There was effectively no cryptoloop support in the vanilla
> kernel.  Andi is totally right here.  We should be compatible
> with what people actually used, which were the external cryptoloop
> patches.
> 
> Nobody, and I mean nobody, has a cryptoloop based upon the IV
> selection done in vanilla kernels.

Ha David - you repeat what Clemens was saying and Andi disputed.

Andries


[I watch what is happening with interest - have delayed util-linux 2.12
in the hope of being able to come with something that works out of the
box together with a sufficiently recent kernel. A handful of loop
patches are needed. I hope they will be applied and documented one
by one.]

Re: [PATCH] Initial Vector Fix for loop.c

[Fruhwirth Clemens]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

Andrew Morton wrote:

> Fruhwirth Clemens <clemens-dated-1056963973.bf26@endorphin.org> wrote:
>>
>> If this bug is fixed, we can go ahead and add cryptoloop which is ready
>> and tested.
>
> Does it use the crypto framework which is present in the 2.5 kernel?

Yes.

> If it does not then the cryptoloop implementation which you mention
> is inappropriate for inclusion.
> 
> If it does then it would be nice to see the full patchset.

http://therapy.endorphin.org/patches/cryptoloop-0.2-2.5.58.diff

It's basically a stub. The lock of the cipher_context can be removed since
post-2.5.58 a new call has been added which makes the IV an argument.
However, that's a minor change.

In case you want to test it, you need to patch losetup too, since it needs
to parse /proc/crypto. 

http://therapy.endorphin.org/patches/losetup-2.5.diff

Regards, Clemens

p.s.: Andrew please send me carbon copies of your mails. Thanks.

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

[PATCH] Initial Vector Fix for loop.c.

[Fruhwirth Clemens]

[-- Attachment #1.1: Type: text/plain, Size: 2022 bytes --]

Meta information: 

This mail proposes a fix for: 
must-fix list item "drivers/block/loop.c" and
should-fix list item "drivers/block/cryptoloop"

Content:

loop.c is broken. It's broken since 2.4. In particular the initial vector
calculation which every cryptographic loop transformation uses is broken.

What's the problem you might ask.. It's that that the IV calculation is
based on the logical block sector of the block device. Just think of what
happends if you move your image to another device with a different block
size..

Correct! The IV metric changes and everyone who knows what's CBC
encryption will correctly conclude: you can't read your image. 

That's bad. And that's the reason why every project which provides you with
encryption via loop.c will also provide you with a patch for this broken
loop.c behavior. 

The fix is quite simple. Just switch to a fixed IV metric. Which is: the
smallest granulity -> fixed 512-byte and kerneli.org's cryptoloop as well as
Jari's loop-AES do exactly that.

These two packages are the only facilities I'm aware of that provides
harddisc encryption for the 2.4. kernel. However marcello refused to switch
the IV metric for 2.4. because it will - yes that's correct - break any
image which has been created with a IV-sensitiv loop transformation..

..BUT except for cryptoloop and loop-AES there are no such loop
transformation modules! And those two projects have already fixed this
issue. So we're providing backward compatiblity for nothing here. No user
base could ever benefit from this backward compatiblity, since the only
reason the initial vector calculation is done is for crypto and every crypto
project out there already uses the 512-byte IV-metric.

So go for it. Fix it before 2.6.x is out and we're stuck with this crap
again. 

http://bugzilla.kernel.org/show_bug.cgi?id=192

Just apply the patch. 

If this bug is fixed, we can go ahead and add cryptoloop which is ready and
tested.

Regards, Clemens

[-- Attachment #1.2: loop-clemens-2.5.70.diff --]
[-- Type: text/plain, Size: 1681 bytes --]

--- drivers/block/loop.c.orig	Fri Jun 20 11:02:10 2003
+++ drivers/block/loop.c	Fri Jun 20 11:02:49 2003
@@ -79,6 +79,8 @@
 
 #include <asm/uaccess.h>
 
+#define LOOP_IV_SECTOR_BITS 9
+
 static int max_loop = 8;
 static struct loop_device *loop_dev;
 static struct gendisk **disks;
@@ -196,7 +198,7 @@
 	data = kmap(bvec->bv_page) + bvec->bv_offset;
 	len = bvec->bv_len;
 	while (len > 0) {
-		sector_t IV = index * (PAGE_CACHE_SIZE/bsize) + offset/bsize;
+		sector_t IV = (index << (PAGE_CACHE_SHIFT - LOOP_IV_SECTOR_BITS)) + (offset >> LOOP_IV_SECTOR_BITS);
 		int transfer_result;
 
 		size = PAGE_CACHE_SIZE - offset;
@@ -279,7 +281,7 @@
 	unsigned long count = desc->count;
 	struct lo_read_data *p = (struct lo_read_data*)desc->buf;
 	struct loop_device *lo = p->lo;
-	int IV = page->index * (PAGE_CACHE_SIZE/p->bsize) + offset/p->bsize;
+	int IV =  (page->index << (PAGE_CACHE_SHIFT - LOOP_IV_SECTOR_BITS)) + (offset >> LOOP_IV_SECTOR_BITS);
 
 	if (size > count)
 		size = count;
@@ -484,7 +486,7 @@
 bio_transfer(struct loop_device *lo, struct bio *to_bio,
 			      struct bio *from_bio)
 {
-	unsigned long IV = loop_get_iv(lo, from_bio->bi_sector);
+	unsigned long IV = from_bio->bi_sector + (lo->lo_offset >> LOOP_IV_SECTOR_BITS);
 	struct bio_vec *from_bvec, *to_bvec;
 	char *vto, *vfrom;
 	int ret = 0, i;
@@ -545,7 +547,7 @@
 	 * piggy old buffer on original, and submit for I/O
 	 */
 	new_bio = loop_get_buffer(lo, old_bio);
-	IV = loop_get_iv(lo, old_bio->bi_sector);
+	IV = old_bio->bi_sector + (lo->lo_offset >> LOOP_IV_SECTOR_BITS);
 	if (rw == WRITE) {
 		if (bio_transfer(lo, new_bio, old_bio))
 			goto err;

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] Initial Vector Fix for loop.c.

[Andrew Morton]

Fruhwirth Clemens <clemens-dated-1056963973.bf26@endorphin.org> wrote:
>
> If this bug is fixed, we can go ahead and add cryptoloop which is ready and
>  tested.

Does it use the crypto framework which is present in the 2.5 kernel?

If it does not then the cryptoloop implementation which you mention
is inappropriate for inclusion.

If it does then it would be nice to see the full patchset.