Re: [PATCH 16/20] PKCS#7: Add an optional authenticated attribute to hold firmware name [ver #5]

[Andy Lutomirski <luto@kernel.org>]

[resending with further gmane screwups fixed]

On 05/28/2015 08:48 AM, David Howells wrote:
> Modify the sign-file program to take a "-F <firmware name>" parameter.  The
> name is a utf8 string that, if given, is inserted in a PKCS#7 authenticated
> attribute from where it can be extracted by the kernel.  Authenticated
> attributes are added to the signature digest.
>
> If the attribute is present, the signature would be assumed to be for
> firmware and would not be permitted with module signing or kexec.  The name
> associated with the attribute would be compared to the name passed to
> request_firmware() and the load request would be denied if they didn't
> match.

This is insecure because PKCS#7 authenticated attributes are broken (see 
RFC2315 section 9.4 note 4).  You need to either require that everything 
have authenticated attributes or require that nothing have authenticated 
attributes.   Maybe this insecurity doesn't matter in practice, but I 
don't wouldn't want to bet on it.

On top of that, this is a ton of code to support something trivial.  And 
it requires an OID to be registered (ick).

Earlier you suggested just appending the signature purpose to the thing 
being signed.  What's wrong with that?  It's probably much less code, it 
doesn't require reviewing details of the godawful PKCS#7 spec, and it 
will continue working if and when someone adds a more sensible signature 
format.  And you could tear out PKCS#7 authenticated attribute support 
on top of it.

P.S.  Or you could stop using PKCS#7 if possible.  Holy crap, maybe it's 
a standard, but it's a standard that we don't actually have to follow 
and it *has trivial collisions by construction*.

For Pete's sake, there are already 1262 lines of code just implementing 
PKCS#7.  In contrast, the *entire* tweetnacl library, which uses better 
crypto and is actually correct (no built-in collisions) fits a complete 
signature implementation and the underlying crypto in barely half that 
many lines.  This is actually unfair to tweetnacl, as tweetnacl also 
includes an AEAD, which could be removed to shorten it by a decent amount.

--Andy