Re: [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface

[Elena Petrova <lenaptr@google.com>]

Hi Stephan,

On Tue, 14 Jul 2020 at 16:34, Stephan Mueller <smueller@chronox.de> wrote:
>
> Am Dienstag, 14. Juli 2020, 17:23:05 CEST schrieb Elena Petrova:
>
> Hi Elena,
>
> > Hi Stephan,
> >
> > On Tue, 14 Jul 2020 at 06:17, Stephan Mueller <smueller@chronox.de> wrote:
> > > Am Montag, 13. Juli 2020, 18:48:56 CEST schrieb Elena Petrova:
> > >
> > > Hi Elena,
> > >
> > > > This patch extends the userspace RNG interface to make it usable for
> > > > CAVS testing. This is achieved by adding ALG_SET_DRBG_ENTROPY
> > > > option to the setsockopt interface for specifying the entropy, and using
> > > > sendmsg syscall for specifying the additional data.
> > > >
> > > > See libkcapi patch [1] to test the added functionality. The libkcapi
> > > > patch is not intended for merging into libkcapi as is: it is only a
> > > > quick plug to manually verify that the extended AF_ALG RNG interface
> > > > generates the expected output on DRBG800-90A CAVS inputs.
> > >
> > > As I am responsible for developing such CAVS/ACVP harness as well, I
> > > played
> > > with the idea of going through AF_ALG. I discarded it because I do not see
> > > the benefit why we should add an interface solely for the purpose of
> > > testing. Further, it is a potentially dangerous one because the created
> > > instance of the DRBG is "seeded" from data provided by the caller.
> > >
> > > Thus, I do not see the benefit from adding that extension, widening a user
> > > space interface solely for the purpose of CAVS testing. I would not see
> > > any
> > > other benefit we have with this extension. In particular, this interface
> > > would then be always there. What I could live with is an interface that
> > > can be enabled at compile time for those who want it.
> >
> > Thanks for reviewing this patch. I understand your concern about the
> > erroneous use of the entropy input by non-testing applications. This was an
> > approach that I had discussed with Ard. I should have included you, my
> > apologies. I'll  post v2 with the CAVS testing stuff hidden under CONFIG_
> > option with appropriate help text.
> >
> > With regards to the usefulness, let me elaborate. This effort of extending
> > the drbg interface is driven by Android needs for having the kernel crypto
> > certified. I started from having an out-of-tree chrdev driver for Google
> > Pixel kernels that was exposing the required crypto functionality, and it
> > wasn't ideal in the following ways:
> >  * it primarily consisted of copypasted code from testmgr.c
> >  * it was hard for me to keep the code up to date because I'm not aware of
> >    ongoing changes to crypto
> >  * it is hard for other people and/or organisations to re-use it, hense a
> > lot of duplicated effort is going into CAVS: you have a private driver, I
> > have mine, Jaya from HP <jayalakshmi.bhat@hp.com>, who's been asking
> > linux-crypto a few CAVS questions, has to develop theirs...
> >
> > In general Android is trying to eliminate out-of-tree code. CAVS testing
> > functionality in particular needs to be upstream because we are switching
> > all Android devices to using a Generic Kernel Image (GKI)
> > [https://lwn.net/Articles/771974/] based on the upstream kernel.
>
> Thank you for the explanation.
> >
> > > Besides, when you want to do CAVS testing, the following ciphers are still
> > > not testable and thus this patch would only be a partial solution to get
> > > the testing covered:
> > >
> > > - AES KW (you cannot get the final IV out of the kernel - I played with
> > > the
> > > idea to return the IV through AF_ALG, but discarded it because of the
> > > concern above)
> > >
> > > - OFB/CFB MCT testing (you need the IV from the last round - same issue as
> > > for AES KW)
> > >
> > > - RSA
> > >
> > > - DH
> > >
> > > - ECDH
> >
> > For Android certification purposes, we only need to test the modules which
> > are actually being used. In our case it's what af_alg already exposes plus
> > DRBG. If, say, HP would need RSA, they could submit their own patch.
> >
> > As for exposing bits of the internal state for some algorithms, I hope
> > guarding the testing functionality with a CONFIG_ option would solve the
> > security part of the problem.
>
> Yes, for all other users.
>
> But if you are planning to enable this option for all Android devices across
> the board I am not sure here. In this case, wouldn't it make sense to require
> capable(CAP_SYS_ADMIN) for the DRBG reset operation just as an additional
> precaution? Note, the issue with the reset is that you loose all previous
> state (which is good for testing, but bad for security as I guess you agree
> :-) ).

I believe that for Android, since CAVS is a one-off on demand
operation, we can create a separate build with CONFIG_CRYPTO_CAVS_DRBG
enabled, which won't go to the users. Thanks for suggesting
capabilities check, happy to add `capable(CAP_SYS_ADMIN)` regardless.

> > > With these issues, I would assume you are better off creating your own
> > > kernel module just like I did that externalizes the crypto API to user
> > > space but is only available on your test kernel and will not affect all
> > > other users.
> > I considered publishing my kernel driver on GitHub, but there appears to be
> > a sufficiently large number of users to justify having this functionality
> > upstream.
>
> So, I should then dust off my AF_ALG KPP and AF_ALG akcipher patches then? :-D

Sure :)

> >
> > Hope that addresses your concerns.
> >
> > > Ciao
> > > Stephan
> >
> > Thanks,
> > Elena
>
>
> Ciao
> Stephan
>
>