Re: [PATCH 3/4] crypto: inside-secure - only update the result buffer when provided
Hi Herbert, On Mon, Dec 11, 2017 at 06:29:46PM +1100, Herbert Xu wrote: > On Tue, Nov 28, 2017 at 10:05:17AM +0100, Antoine Tenart wrote: > > > > if (sreq->finish) > > result_sz = crypto_ahash_digestsize(ahash); > > - memcpy(sreq->state, areq->result, result_sz); > > + > > + /* The called isn't required to supply a result buffer. Updated it only > > +* when provided. > > +*/ > > + if (areq->result) > > + memcpy(sreq->state, areq->result, result_sz); > > I don't think you should use whether areq->result is NULL to > determine whether to copy data into it. For example, I could > be making an update call with a non-NULL areq->result and it would > be completely wrong if you overwrote that with the above memcpy. > > IOW areq->result should not be touched at all unless you are doing > a finalisation. I didn't know that. It means the SafeXcel driver logic is wrong regarding this point, as areq->result is DMA mapped and used of all hash operations (including updates). So this patch is indeed fixing an issue, which should probably not be there in the first place. I guess you recommend using a buffer local to the driver instead, and only update areq->request on completion (final). Thanks! Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
Re: [RFC PATCH] crypto: chacha20 - add implementation using 96-bit nonce
Hi, > Anyway, I actually thought it was intentional that the ChaCha > implementations in the Linux kernel allowed specifying the block > counter, and therefore allowed seeking to any point in the keystream, > exposing the full functionality of the cipher. If I remember correctly, it was indeed intentional. When building the chacha20poly1305 AEAD both in [1] and [2], a block counter of 0 is used to generate the Poly1305 key. For the ChaCha20 encryption, an explicit initial block counter of 1 is used to avoid reusing the same counter. Maybe it would be possible to implement this with implicit counters, but doing this explicitly looked much clearer to me. So I guess there are use cases for explicit block counters in ChaCha20. Best regards Martin [1] https://tools.ietf.org/html/rfc7539#section-2.8 [2] https://tools.ietf.org/html/rfc7634#section-2
Re: [PATCH 3/4] crypto: inside-secure - only update the result buffer when provided
On Tue, Nov 28, 2017 at 10:05:17AM +0100, Antoine Tenart wrote:
> The patch fixes the ahash support by only updating the result buffer
> when provided. Otherwise the driver could crash with NULL pointer
> exceptions, because the ahash caller isn't required to supply a result
> buffer on all calls.
>
> Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto
> engine driver")
> Signed-off-by: Antoine Tenart
> ---
> drivers/crypto/inside-secure/safexcel_hash.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/crypto/inside-secure/safexcel_hash.c
> b/drivers/crypto/inside-secure/safexcel_hash.c
> index 6135c9f5742c..424f4c5d4d25 100644
> --- a/drivers/crypto/inside-secure/safexcel_hash.c
> +++ b/drivers/crypto/inside-secure/safexcel_hash.c
> @@ -150,7 +150,12 @@ static int safexcel_handle_req_result(struct
> safexcel_crypto_priv *priv, int rin
>
> if (sreq->finish)
> result_sz = crypto_ahash_digestsize(ahash);
> - memcpy(sreq->state, areq->result, result_sz);
> +
> + /* The called isn't required to supply a result buffer. Updated it only
> + * when provided.
> + */
> + if (areq->result)
> + memcpy(sreq->state, areq->result, result_sz);
I don't think you should use whether areq->result is NULL to
determine whether to copy data into it. For example, I could
be making an update call with a non-NULL areq->result and it would
be completely wrong if you overwrote that with the above memcpy.
IOW areq->result should not be touched at all unless you are doing
a finalisation.
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Crypto Fixes for 4.15
Hi Linus: This push fixes the following issues: - Buffer overread in RSA. - Potential use after free in algif_aead. - Error path null pointer dereference in af_alg. - Forbid combinations such as hmac(hmac(sha3)) which may crash. - Crash in salsa20 due to incorrect API usage. Please pull from git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git linus Eric Biggers (5): crypto: rsa - fix buffer overread when stripping leading zeroes crypto: algif_aead - fix reference counting of null skcipher crypto: af_alg - fix NULL pointer dereference in crypto: hmac - require that the underlying hash algorithm is unkeyed crypto: salsa20 - fix blkcipher_walk API usage arch/x86/crypto/salsa20_glue.c |7 --- crypto/af_alg.c| 13 +++-- crypto/algif_aead.c|2 +- crypto/hmac.c |6 +- crypto/rsa_helper.c|2 +- crypto/salsa20_generic.c |7 --- crypto/shash.c |5 +++-- include/crypto/internal/hash.h |8 8 files changed, 25 insertions(+), 25 deletions(-) Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [RFC PATCH] crypto: chacha20 - add implementation using 96-bit nonce
Ard Biesheuvel wrote: > As pointed out by Eric [0], the way RFC7539 was interpreted when creating > our implementation of ChaCha20 creates a risk of IV reuse when using a > little endian counter as the IV generator. The reason is that the low end > bits of the counter get mapped onto the ChaCha20 block counter, which > advances every 64 bytes. This means that the counter value that gets > selected as IV for the next input block will collide with the ChaCha20 > block counter of the previous block, basically recreating the same > keystream but shifted by 64 bytes. As Eric pointed out for steram ciphers such as chacha20 our policy is to expose the raw IV in the base algorithm, and then layer more restrictive implementations on top that can then be used in different scenarios such as IPsec or disk encryption. For example, with CTR, ctr(aes) is the base algorithm and places no restrictions on the IV, while rfc3686(ctr(aes)) is the more restrictive version that's used by IPsec. Within the kernel I don't really see an issue with abuse because all users are hopefully reviewed by the community. If you're worried about incorrect use in user-space we could think about restricting access to these base implementations. For chacha20 we did not add a restrictive template because the primary user IPsec uses it only through AEAD where the IV restriction is in place. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] fscrypt: add support for ChaCha20 contents encryption
On Fri, Dec 08, 2017 at 07:48:54PM -0500, Jeffrey Walton wrote: > > Still, a stream cipher is sufficient to protect data confidentiality in > > the event of a single point-in-time permanent offline compromise of the > > disk, which currently is the primary threat model for fscrypt. Thus, > > when the alternative is quite literally *no encryption*, we might as > > well use a stream cipher. > > The "single point in time" requirement is kind of interesting. I > believe you are saying the scheme lacks semantic security. Well, it is semantically secure when looking at encryptions/decryptions done in the context of different blocks (different IVs) but not semantically secure when looking at encryptions/decryptions done in the context of the same block (same IV). But in that regard it is the same as the other modes such as AES-XTS or AES-CBC. So I think you are missing the point, which is that a stream cipher fails more catastrophically than the other modes when IVs are reused. > > Forgive my ignorance... Does that mean this cipher should not be used > when backups are in effect; or sync'ing to provider> happens? Normally backup or sync software will operate on the plaintext, which makes whatever type of filesystem-level or disk encryption you happen to be using irrelevant. But at a more abstract level, intentional copies (in addition to "unintentional" copies that may be done by the filesystem or flash storage) are certainly a way that you could end up with multiple ciphertexts for the same block in existence at the same time, so that would indeed need to be accounted for in the assumptions. Eric
