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

Eugen Leitl eugen at leitl.org
Mon Oct 7 05:47:42 EDT 2013


----- Forwarded message from Pawel Jakub Dawidek <pjd at FreeBSD.org> -----

Date: Mon, 7 Oct 2013 11:44:57 +0200
From: Pawel Jakub Dawidek <pjd at FreeBSD.org>
To: zfs at lists.illumos.org
Subject: Re: [zfs] [Review] 4185 New hash algorithm support
Message-ID: <20131007094456.GB1385 at garage.freebsd.pl>
User-Agent: Mutt/1.5.21 (2010-09-15)
Reply-To: zfs at lists.illumos.org

On Mon, Oct 07, 2013 at 12:47:52AM +0100, Saso Kiselkov wrote:
> Please review what frankly has become a bit of a large-ish feature:
> http://cr.illumos.org/~webrev/skiselkov/new_hashes/
> 
> This webrev implements new hash algorithms for ZFS with much improved
> performance. There are three algorithms included:
> 
>  * SHA-512/256: truncated version of SHA-512 per FIPS 180-4. Uses
> 	all existing code from the sha2 module (with new H(0) consts),
> 	but the native 64-bit arithmetic used in SHA-512 provides a
> 	~50% performance boost relative to SHA-256 on 64-bit hardware.
> 
>  * Skein-512: 80% faster than SHA-256 in optimized C implementation,
> 	and a very high security margin (Skein was a finalist in SHA-3).
> 	Also includes a KCF SW provider.
> 
>  * Edon-R-512: 350% faster than SHA-256 in optimized C implementation.
> 	Security margin lower than Skein.
> 
> To address any security concerns associated with using new algorithms
> this patch also implements salted checksum support. We store a random
> 256-bit secret key (the salt) in the MOS and use it to pre-seed the hash
> algorithms (Skein and Edon-R use this, SHA-512/256 is just a straight
> hash). Any attacker thus cannot simply mount a collision attack on the
> algorithm, since they can't completely control the input.

I do like your patch and addition of first two algorithms, but I have to
comment on the third one.

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).

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.

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.

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?

--- /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.

+	ctx = kmem_zalloc(sizeof (*ctx), KM_SLEEP);

For Skein you also use KM_PUSHPAGE flag.

-- 
Pawel Jakub Dawidek                       http://www.wheelsystems.com
FreeBSD committer                         http://www.FreeBSD.org
Am I Evil? Yes, I Am!                     http://mobter.com



-------------------------------------------
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/f6019afd/attachment.asc>


More information about the cryptography mailing list