linux-kernel - Re: [PATCH v6 1/4] crypto: AF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <29582980.qoHS2EjmLy@tachyon.chronox.de>
Date:	Mon, 29 Dec 2014 16:05:40 +0100
From:	Stephan Mueller <smueller@...onox.de>
To:	Herbert Xu <herbert@...dor.apana.org.au>
Cc:	Daniel Borkmann <dborkman@...hat.com>,
	'Quentin Gouchet' <quentin.gouchet@...il.com>,
	'LKML' <linux-kernel@...r.kernel.org>,
	linux-crypto@...r.kernel.org, linux-api@...r.kernel.org
Subject: Re: [PATCH v6 1/4] crypto: AF_ALG: add AEAD support

Am Montag, 29. Dezember 2014, 21:33:19 schrieb Herbert Xu:

Hi Herbert,

> On Thu, Dec 25, 2014 at 11:01:47PM +0100, Stephan Mueller wrote:
> > +	err = -ENOMEM;
> 
> This should be EINVAL.

Changed
> 
> > +	if (!aead_sufficient_data(ctx))
> > +		goto unlock;
> 
> So we're checking two things here, one that we have enough data
> for AD and two we have the authentication tag.  The latter is
> redundant as the underlying implementation should be able to cope
> with short input so we should only check the assoclen here.

Agreed, will change it to

if (ctx->used < ctx->aead_assoclen)

> 
> Also this check should be moved to the sendmsg side as that'll
> make it more obvious as to what went wrong.

I would be a bit uneasy about that as this would open up a potential kernel 
crasher: the sleep in aead_readable() can wake up recvmsg in two conditions: 
either we received sufficient data or we do not expect more data (due to !ctx-
>more). If the latter triggers, we still may have insufficient AD data. Yet, 
the following code now sets the AD with aead_request_set_assoc using the 
initially expected data. So, the data buffer provided to  
aead_request_set_assoc is not long enough. The mentioned check shall prevent 
this problem.

In addition, I do not see how we can move that check to the sendmsg/sendpage 
side: the code currently allows the caller to freely invoke the syscall 
arbitrary amount of times. Thus, one particular invocation of sendmsg/sendpage 
does not mean we receive all AD.

Again, to allow the caller the greatest degree of freedom, you can call 
sendmsg with an arbitrary amount of bytes as often as you want (until we fill 
up all buffers) before the recvmsg is triggered. So, there is no need to send 
the entire AD (or even AD+message) buffer in one sendmsg call. Compare the 
AEAD interface with a hash interface:

- the AEAD sendmsg/sendpage is logically equivalent to a hash update that you 
can call an arbitrary number of times with an arbitrary number of bytes.

- the AEAD recvmsg is logically equivalent to the hash final.

This would mean that the check must stay in recvmsg as only here we know that 
the caller wants data to be processed.

> 
> PS we should add a length check for missing/partial auth tags
> to crypto_aead_decrypt.  We can then remove such checks from
> individual implementations.

I agree in full here. Shall I create such a patch together with the AEAD 
AF_ALG interface, or can we merge the AEAD without that patch now and create a 
separate patch later?
> 
> Thanks,


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