----- Forwarded message from Saso Kiselkov <skiselkov...@gmail.com> -----
Date: Mon, 07 Oct 2013 13:04:52 +0100 From: Saso Kiselkov <skiselkov...@gmail.com> To: z...@lists.illumos.org CC: Pawel Jakub Dawidek <p...@freebsd.org> Subject: Re: [zfs] [Review] 4185 New hash algorithm support Message-ID: <5252a364.4000...@gmail.com> User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:17.0) Gecko/20130801 Thunderbird/17.0.8 Reply-To: z...@lists.illumos.org On 10/7/13 10:44 AM, Pawel Jakub Dawidek wrote: > I do like your patch and addition of first two algorithms, but I have to > comment on the third one. I sort of expected you'd be among the first ones :) > By adding a salt to Edon-R-512 you assume that prerequisite to creating > collision is guessing the entire salt, so having 256bit salt makes it > safe. I'd say this is brave assumption. We (at least I) don't know how > this algorithm treats those first 256 bits. Maybe for large blocks some > (most?) of the salt enropy is lost? If we think Edon-R-512 is not > cryptographically secure, making any assumptions that weren't properly > analysed comes at too high risk, IMHO. > > You are also calling your salting method being a MAC, where we do have > well-known and secure MAC algorithms that are based on hash functions > (ie. HMAC). Both Edon-R and Skein are resistant to length-extension attacks, so simply prepending the salt to the input is sufficient (in fact, I use Skein's built-in support for MACs, which does just that). There is, as far as I can tell, one paper which demonstrates a practical secret-IV recovery attack on Edon-R, but it requires that the attacker be able to observe the algorithm's output (which in our case they can't) and it only works on Edonr-R-256 and doesn't work on the tweaked version (which is part of this webrev). The paper in question is: http://www.di.ens.fr/~leurent/files/Edon_RSA10.pdf It is possible trivially avert this attack by switching over to using HMAC, but I see this as pointless, as so many of the attack's fundamental requirements aren't met in our environment. > Personally I'd love to have an option to use HMAC/SHA256 for example > with secret key stored in pool. Currently in our product we put ZFS with > SHA256 on top of block-level disk encryption. I'd feel much better to > have proper data authentication using HMAC. At some point I may find > time to implement that based on your patch. You should probably best look at implementing straight encryption when you're already doing this. The target isn't that far removed. > Do we want to allow to use different algorithms for using deduplication > within send/recv streams? I think SHA256 is now hardcoded? > It can of course be implemented separately, just wondering. That would require a version bump of the send stream format, which might make interoperability a bit more thorny... not too hard I guess, but I don't currently see a need for it (perhaps somebody might request it later on). > Few minor observations: > > --- old/usr/src/lib/libzfs/common/libzfs_dataset.c Mon Oct 7 09:23:07 2013 > +++ new/usr/src/lib/libzfs/common/libzfs_dataset.c Mon Oct 7 09:23:07 2013 > @@ -1377,6 +1377,12 @@ > "property setting is not allowed on " > "bootable datasets")); > (void) zfs_error(hdl, EZFS_NOTSUP, errbuf); > + } else if (prop == ZFS_PROP_CHECKSUM || > + prop == ZFS_PROP_DEDUP) { > + (void) zfs_error_aux(hdl, dgettext(TEXT_DOMAIN, > + "property setting is not allowed on " > + "root pools")); > + (void) zfs_error(hdl, EZFS_NOTSUP, errbuf); > } else { > (void) zfs_standard_error(hdl, err, errbuf); > } > > When I read this change it looks like we don't allow to change checksum > property on root pools at all? No, this is to cover the ERANGE return from zfs_ioctl.c:3784 where we check to make sure nobody enables any salted checksums on a root pool, since GRUB doesn't have support for that feature. Bootloader support on FreeBSD might differ, though. > --- /dev/null Mon Oct 7 09:23:09 2013 > +++ new/usr/src/uts/common/fs/zfs/edonr_zfs.c Mon Oct 7 09:23:09 2013 > [...] > +void * > +zio_checksum_edonr_tmpl_init(const zio_cksum_salt_t *salt) > +{ > [...] > + /*LINTED E_TRUE_LOGICAL_EXPR*/ > + ASSERT(EDONR_BLOCK_SIZE == 2 * (EDONR_MODE / 8)); > > There is CTASSERT() macro in FreeBSD that can be used for checks like > that, which generates compile-time error if it isn't true. Nice macro, but on Illumos it doesn't appear to exist. So I copied it and used it :) Thanks for the suggestion. Webrev updated. > + ctx = kmem_zalloc(sizeof (*ctx), KM_SLEEP); > > For Skein you also use KM_PUSHPAGE flag. That was a leftover, removed it now. Thanks for spotting this. Cheers, -- Saso ------------------------------------------- illumos-zfs Archives: https://www.listbox.com/member/archive/182191/=now RSS Feed: https://www.listbox.com/member/archive/rss/182191/22842876-6fe17e6f Modify Your Subscription: https://www.listbox.com/member/?member_id=22842876&id_secret=22842876-a25d3366 Powered by Listbox: http://www.listbox.com ----- End forwarded message ----- -- Eugen* Leitl <a href="http://leitl.org";>leitl</a> http://leitl.org ______________________________________________________________ ICBM: 48.07100, 11.36820 http://ativel.com http://postbiota.org AC894EC5: 38A5 5F46 A4FF 59B8 336B 47EE F46E 3489 AC89 4EC5
signature.asc
Description: Digital signature
_______________________________________________ cryptography mailing list cryptography@randombit.net http://lists.randombit.net/mailman/listinfo/cryptography
