| Message ID | 04f372f3-bc26-c629-2269-0e5d258f9d8f@maciej.szmigiero.name (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
On 19.05.2018 14:23, Maciej S. Szmigiero wrote: > The signatureValue field of a X.509 certificate is encoded as a BIT STRING. > For RSA signatures this BIT STRING is of so-called primitive subtype, which > contains a u8 prefix indicating a count of unused bits in the encoding. > > We have to strip this prefix from signature data, just as we already do for > key data in x509_extract_key_data() function. > > This wasn't noticed earlier because this prefix byte is zero for RSA key > sizes divisible by 8. Since BIT STRING is a big-endian encoding adding zero > prefixes has no bearing on its value. > > The signature length, however was incorrect, which is a problem for RSA > implementations that need it to be exactly correct (like AMD CCP). Any progress here? This simple patch has already been submitted 3 times in last 3+ months...
On 02.06.2018 21:12, Maciej S. Szmigiero wrote: > On 19.05.2018 14:23, Maciej S. Szmigiero wrote: >> The signatureValue field of a X.509 certificate is encoded as a BIT STRING. >> For RSA signatures this BIT STRING is of so-called primitive subtype, which >> contains a u8 prefix indicating a count of unused bits in the encoding. >> >> We have to strip this prefix from signature data, just as we already do for >> key data in x509_extract_key_data() function. >> >> This wasn't noticed earlier because this prefix byte is zero for RSA key >> sizes divisible by 8. Since BIT STRING is a big-endian encoding adding zero >> prefixes has no bearing on its value. >> >> The signature length, however was incorrect, which is a problem for RSA >> implementations that need it to be exactly correct (like AMD CCP). > > Any progress here? > This simple patch has already been submitted 3 times in last 3+ months... > A friendly ping here. @AMD people: Without this patch, in-kernel X.509 certificate verification is broken on AMD CCP RSA implementation. For example, loading wireless regulatory database gives the following errors: > [ 21.310361] cfg80211: Problem loading in-kernel X.509 certificate (-22) > [ 21.351717] cfg80211: loaded regulatory.db is malformed or signature is missing/invalid Kernel modules signature verification probably has similar problem, too. That's why it would be nice if you could ack this patch, please. Maciej
On Wed, Jun 20, 2018 at 02:24:54PM +0200, Maciej S. Szmigiero wrote: > > A friendly ping here. > > @AMD people: > Without this patch, in-kernel X.509 certificate verification is broken > on AMD CCP RSA implementation. > > For example, loading wireless regulatory database gives the following > errors: > > [ 21.310361] cfg80211: Problem loading in-kernel X.509 certificate (-22) > > [ 21.351717] cfg80211: loaded regulatory.db is malformed or signature is missing/invalid > > Kernel modules signature verification probably has similar problem, too. > > That's why it would be nice if you could ack this patch, please. David/James, is there an issue with the patch? Thanks,
On Wed, 20 Jun 2018, Herbert Xu wrote: > On Wed, Jun 20, 2018 at 02:24:54PM +0200, Maciej S. Szmigiero wrote: > > > > A friendly ping here. > > > > @AMD people: > > Without this patch, in-kernel X.509 certificate verification is broken > > on AMD CCP RSA implementation. > > > > For example, loading wireless regulatory database gives the following > > errors: > > > [ 21.310361] cfg80211: Problem loading in-kernel X.509 certificate (-22) > > > [ 21.351717] cfg80211: loaded regulatory.db is malformed or signature is missing/invalid > > > > Kernel modules signature verification probably has similar problem, too. > > > > That's why it would be nice if you could ack this patch, please. > > David/James, is there an issue with the patch? Not from my POV.
On Thu, Jun 21, 2018 at 11:44:29AM +1000, James Morris wrote: > On Wed, 20 Jun 2018, Herbert Xu wrote: > > > On Wed, Jun 20, 2018 at 02:24:54PM +0200, Maciej S. Szmigiero wrote: > > > > > > A friendly ping here. > > > > > > @AMD people: > > > Without this patch, in-kernel X.509 certificate verification is broken > > > on AMD CCP RSA implementation. > > > > > > For example, loading wireless regulatory database gives the following > > > errors: > > > > [ 21.310361] cfg80211: Problem loading in-kernel X.509 certificate (-22) > > > > [ 21.351717] cfg80211: loaded regulatory.db is malformed or signature is missing/invalid > > > > > > Kernel modules signature verification probably has similar problem, too. > > > > > > That's why it would be nice if you could ack this patch, please. > > > > David/James, is there an issue with the patch? > > Not from my POV. Hi James: I presume you will pick this up then? Thanks,
On Thu, 21 Jun 2018, Herbert Xu wrote: > Hi James: > > I presume you will pick this up then? I will -- not sure why David hasn't merged it into his tree. Can I add your acked or reviewed by?
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c index 7d81e6bb461a..b6cabac4b62b 100644 --- a/crypto/asymmetric_keys/x509_cert_parser.c +++ b/crypto/asymmetric_keys/x509_cert_parser.c @@ -249,6 +249,15 @@ int x509_note_signature(void *context, size_t hdrlen, return -EINVAL; } + if (strcmp(ctx->cert->sig->pkey_algo, "rsa") == 0) { + /* Discard the BIT STRING metadata */ + if (vlen < 1 || *(const u8 *)value != 0) + return -EBADMSG; + + value++; + vlen--; + } + ctx->cert->raw_sig = value; ctx->cert->raw_sig_size = vlen; return 0;
The signatureValue field of a X.509 certificate is encoded as a BIT STRING. For RSA signatures this BIT STRING is of so-called primitive subtype, which contains a u8 prefix indicating a count of unused bits in the encoding. We have to strip this prefix from signature data, just as we already do for key data in x509_extract_key_data() function. This wasn't noticed earlier because this prefix byte is zero for RSA key sizes divisible by 8. Since BIT STRING is a big-endian encoding adding zero prefixes has no bearing on its value. The signature length, however was incorrect, which is a problem for RSA implementations that need it to be exactly correct (like AMD CCP). Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name> Fixes: c26fd69fa009 ("X.509: Add a crypto key parser for binary (DER) X.509 certificates") Cc: stable@vger.kernel.org --- This is a resend of a patch that was previously submitted in one series with CCP driver changes since this particular patch should go through the security (rather than crypto) tree. Changes from v1: Change '!' to '== 0'. crypto/asymmetric_keys/x509_cert_parser.c | 9 +++++++++ 1 file changed, 9 insertions(+)