Re: [RFC PATCH 4/4] crypto: Add EC-RDSA algorithm

From: Vitaly Chikunov <vt@altlinux.org>
To: Stephan Mueller <smueller@chronox.de>
Cc: David Howells <dhowells@redhat.com>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Mimi Zohar <zohar@linux.vnet.ibm.com>,
	Dmitry Kasatkin <dmitry.kasatkin@gmail.com>,
	linux-integrity@vger.kernel.org, keyrings@vger.kernel.org,
	linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 4/4] crypto: Add EC-RDSA algorithm
Date: Mon, 7 Jan 2019 12:04:49 +0300	[thread overview]
Message-ID: <20190107090449.d364ii24zervlsfq@sole.flsd.net> (raw)
In-Reply-To: <1714084.mfT8VG1pOj@tauon.chronox.de>

Stephan,

On Mon, Jan 07, 2019 at 09:31:40AM +0100, Stephan Mueller wrote:
> Am Montag, 7. Januar 2019, 09:07:10 CET schrieb Vitaly Chikunov:
> 
> > > Why do you manually parse the ASN.1 structure instead of using the ASN.1
> > > parser?
> > 
> > I am not sure this worth effort and will not be most degenerate use of
> > asn1_ber_decoder, since 1) I only need to parse one type in each case:
> > OCTET STRING string above code, and OIDs in below code; 2) this data is
> > said to be in DER format, which asn1_ber_decoder can not enforce. Surely
> > this will also produce more code and files.
> 
> RSA public keys also only contain n and e in the ASN.1 structure for which the 
> ASN.1 parser is used (see linux/crypto/rsapubkey.asn1).

Yes I saw it, but at least there is two values (n and e), while EC-RDSA
pub_key is just one OCTET STRING value (which internally is not defined
to be ASN.1).

> As ASN.1 parsing is always having security issues, I would rather suggest to
> have this parsing implemented in one spot and not here and there.

Yes. So I tried to parse it very carefully and strictly.

> Regarding your comment (2), I am not sure I understand. Why do you say that 
> the DER format cannot be parsed by the kernel's ASN.1 parser? For example, 

It can, but DER is stricter than BER. For example, in DER 'OCTET STRING'
length field should be encoded in just one byte if it's smaller than
128, in BER it could be encoded in multiple encodings. This does not
seem like a big deal, though. With BER decoder an improperly DER-encoded
certificate could be successfully parsed.

> when you generate RSA keys in DER format with, say, openssl, the kernel ASN.1 
> parser will happily use them. Also, when I created my (not accepted) patch to 
> load PQG domain parameters for DH using the ASN.1 parser, the PQG domain 
> parameters created by openssl in DER format were processed well.
> 
> Ciao
> Stephan
> 