Linux-Crypto Archive on lore.kernel.org
 help / color / Atom feed
From: Stephan Mueller <smueller@chronox.de>
To: Elena Petrova <lenaptr@google.com>
Cc: "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
	<linux-crypto@vger.kernel.org>,
	Eric Biggers <ebiggers@kernel.org>,
	Ard Biesheuvel <ardb@kernel.org>
Subject: Re: [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface
Date: Tue, 14 Jul 2020 17:34:34 +0200
Message-ID: <3312053.iIbC2pHGDl@tauon.chronox.de> (raw)
In-Reply-To: <CABvBcwauK_JyVzONdwJRGU81ZH5sYuiJSH0F2g+i5qCe363+fQ@mail.gmail.com>

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 
:-) ).
> 
> > 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
> 
> Hope that addresses your concerns.
> 
> > Ciao
> > Stephan
> 
> Thanks,
> Elena


Ciao
Stephan



  reply index

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-13 16:48 Elena Petrova
2020-07-13 16:48 ` [PATCH 1/1] " Elena Petrova
2020-07-13 17:10   ` Eric Biggers
2020-07-16 14:23     ` Elena Petrova
2020-07-16 16:40       ` [PATCH v2] " Elena Petrova
2020-07-20 17:35         ` Stephan Mueller
2020-07-21 12:55           ` Elena Petrova
2020-07-21 13:18             ` Stephan Mueller
2020-07-28 16:16               ` Elena Petrova
2020-07-20 17:42         ` Stephan Müller
2020-07-22 15:59         ` Eric Biggers
2020-07-28 15:51           ` [PATCH v3] " Elena Petrova
2020-07-28 17:36             ` Eric Biggers
2020-07-29 15:45               ` [PATCH v4] " Elena Petrova
2020-07-29 19:26                 ` Stephan Müller
2020-07-31  7:23                 ` Herbert Xu
2020-08-03 14:48                   ` Elena Petrova
2020-08-03 15:10                     ` Stephan Mueller
2020-08-03 15:30                       ` Elena Petrova
2020-08-04  2:18                     ` Herbert Xu
2020-07-13 17:25   ` [PATCH 1/1] " Eric Biggers
2020-07-31  7:26     ` Herbert Xu
2020-08-13 16:00       ` Elena Petrova
2020-08-13 16:01         ` [PATCH v4] " Elena Petrova
2020-08-13 16:04           ` Elena Petrova
2020-08-13 16:08             ` [PATCH v5] " Elena Petrova
2020-08-13 19:32               ` Eric Biggers
2020-08-21  4:24                 ` Herbert Xu
2020-09-08 17:04                   ` [PATCH v6] " Elena Petrova
2020-09-09  4:35                     ` Eric Biggers
2020-09-09 18:29                       ` [PATCH v7] " Elena Petrova
2020-09-09 21:00                         ` Eric Biggers
2020-09-16 11:07                           ` [PATCH v8] " Elena Petrova
2020-09-18  6:43                             ` Herbert Xu
2020-09-18 15:42                               ` [PATCH v9] " Elena Petrova
2020-09-25  8:16                                 ` Herbert Xu
2020-09-08 17:23                   ` [PATCH v5] " Elena Petrova
2020-09-08 17:18                 ` Elena Petrova
2020-07-14  5:17 ` [PATCH 0/1] " Stephan Mueller
2020-07-14 15:23   ` Elena Petrova
2020-07-14 15:34     ` Stephan Mueller [this message]
2020-07-16 14:41       ` Elena Petrova
2020-07-16 14:49         ` Stephan Mueller
2020-07-16 14:59           ` Stephan Mueller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3312053.iIbC2pHGDl@tauon.chronox.de \
    --to=smueller@chronox.de \
    --cc=ardb@kernel.org \
    --cc=ebiggers@kernel.org \
    --cc=lenaptr@google.com \
    --cc=linux-crypto@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Crypto Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-crypto/0 linux-crypto/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-crypto linux-crypto/ https://lore.kernel.org/linux-crypto \
		linux-crypto@vger.kernel.org
	public-inbox-index linux-crypto

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-crypto


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git