Re: [PATCH 0/1] dm-integrity: integrity protection device-mappertarget

[Milan Broz]

On 09/24/2012 06:20 PM, Kasatkin, Dmitry wrote:

>> So it can provide confidentiality but it CANNOT provide integrity protection.
>>
> Yes, it provides confidentiality and via encryption it provides
> certain level of integrity protection.
> Data cannot be modified without being detected.
> Decryption will result in garbage...

It should, but it is not cryptographic integrity protection.
Moreover, you can revert ciphertext sector content.

Btw I think dm-integrity doesn't solve the reply problem as well.

(IOW can you replace/revert both data and hash, still using the same key
in keyring, without dm-integrity able to detect it?)

In dm-verity, root hash will change after such data tampering.

>> Obvious question: can be dm-verity extended to provide read-write integrity?
>>
> 
> I think re-calculating hash trees all the time and syncing block
> hashes and tree itself is heavier operation...

Then why not extend dm-verity to use you hash format? There are options
for that and we can redefine table line if necessary.

(You are duplicating a lot of code here otherwise, not counting userspace yet.)

Whatever, I shortly read the code and have some notes.

First, device-mapper is variable system, you can stack devices in arbitrary order.

Unfortunately, this is something what can be dangerous in crypto, here you can e.g.
 - do mac (integrity) then encrypt
 - do encrypt, then check integrity

In many implementations mac-then-encrypt system was not secure.
Are we sure that it cannot provide side channels here?

Note read-only integrity target (like dm-verity) is specific case, you should
not be able to run any chosen ciphertext attacks here, it is read-only device.

In fact, I am not sure if we should provide separate read-write block integrity
(wihtout encryption) target at all.

Either it should use standard mode of authenticated encryption (like GCM)
or data integrity should be upper layer business (which knows better which data
really need integrity checking and which areas are just unused garbage.
If you consider hashing and encryption as "heavy operation" you should minimize
it to only really used area - and only upper layer know about real used data content.)

(Again, this is different for read-only target, where it usually uses compressed
RO fs image which uses all available space.)


1) discards
It seems the dm-integrity target doesn't solve problem with discards.

If you send discard request to underlying device, data content of discarded area
is undefined (or zeroed if discards zeroes data) but is is definitely "invalid"
form the dm-integrity point of view. And you are allowing discard IOs there.

I am not sure what should happen, but the behaviour should be documented (at least)
or disabled.

2)
All used hash algorithms must be configurable.

>From your doc:

+While NIST does recommend to use SHA256 hash algorithm instead of SHA1,
+this does not apply to HMAC(SHA1), because of keying.
+This target uses HMAC(SHA1), because it takes much less space and it is
+faster to calculate. There is no parameter to change hmac hash algorithm.

While this is technically true, http://csrc.nist.gov/groups/ST/hash/policy.html
disallowing use of another hash algorithm is wrong, and in fact it is regression
in comparison with dm-verity (which already implements all needed calculations
and uses hash name as table line option).

+HMAC(SHA1) size is 20 bytes. So every 4k block on the integrity device can
+store 204 hmacs. In order to get the required size of the integrity device,
+it is necessary to divide data device size by 204.

Why are you hardcoding block and hash sizes?
Again, dm-verity have this configurable with some good default.

ditto for kernel keyring:
+       const char *hmac_algo = "hmac(sha1)"; /* uncompromized yet */

3)
I like that this target can use kernel keyring, in fact, we should implement
similar option to dm crypt/verity. But the target should not be limited
to use TPM keys only.
(And key is stored directly in kernel memory later for hash calculation anyway
allowing hw attacks reading it from RAM.)

4)
reboot notifier cannot be used this way in generic storage stacking IMHO,
but I think there was discussion with Mikulas already
http://www.redhat.com/archives/dm-devel/2012-March/msg00164.html

5)
output target table should not translate device to device symbolic names

Anyway, code can be changed. But the high level questions remains...

Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/