[cryptography] [zfs] [Review] 4185 New hash algorithm support

Eugen Leitl eugen at leitl.org
Mon Oct 7 08:42:49 EDT 2013


----- Forwarded message from Saso Kiselkov <skiselkov.ml at gmail.com> -----

Date: Mon, 07 Oct 2013 13:04:52 +0100
From: Saso Kiselkov <skiselkov.ml at gmail.com>
To: zfs at lists.illumos.org
CC: Pawel Jakub Dawidek <pjd at FreeBSD.org>
Subject: Re: [zfs] [Review] 4185 New hash algorithm support
Message-ID: <5252A364.4000108 at 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: zfs at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.randombit.net/pipermail/cryptography/attachments/20131007/21d88188/attachment.asc>


More information about the cryptography mailing list