Re: [jose] [COSE] HPKE PartyU / PartyV

[Ilari Liusvaara <[e-mail]>]

On Thu, Feb 29, 2024 at 04:30:57PM -0600, Orie Steele wrote:
> Inline:
> 
> On Thu, Feb 29, 2024 at 2:03 PM Ilari Liusvaara <[e-mail]>
> wrote:
> 
> > On Thu, Feb 29, 2024 at 11:04:57AM -0600, Orie Steele wrote:
> >
> >
> > > 2. New Enc_structure, (fix disconnected layers vulnerability), protect
> > > "Enc_structure" with AAD (via HPKE AAD).
> >
> > impl MultiRecipientEncrypt
> > {
> >         pub fn new(alg: EncryptionAlgorithm, rng: &mut
> > TemporaryRandomStream) ->
> >                 Result<MultiRecipientEncrypt, String>
> >         {
> >                 //...
> >         }
> >         pub fn get_key(&self) -> &[u8] { &self.key }
> >         pub fn add_recipient(&mut self, recipient: &[CBOR])
> >         {
> >                 //...
> >
> 
> Here is where your recipient code checks the algorithm.
> Constructs the Enc_Structure (either fixing, or not fixing the AES-CBC
> issue).
> Encrypts the content encryption key, using Enc_structure as AAD.
> And sets the encrypted_key of the recipient structure.

This method can not do anything like that.

1) It does not know anything about algorithms.
2) It has no key to encrypt available.
3) The top-level protected header does not even exist yet.

 
> IMO, it's a mistake to encrypt a symmetric key if you are not sure what it
> will be used for...

That is fundamental to design of both COSE and JOSE.


> especially if the attacker can change its use from AES-GCM to AES-CBC.

In JOSE, that is not possible. In COSE it is a missing critical security
requrement.


> You can call refusing to encrypt a key without knowing the algorithm it
> will be used with a layer violation.
> ... I call it a security oriented design improvement.

It would not have been layer violation if COSE and JOSE did it from the
very beginning. It is not something one can bolt on later.

However, it turns out that requires all key managment methods to be
AEAD-capable, which is not feasible (e.g., can not use ECDH-ES+A128KW).
So that leaves encrypting keys for unknown algorithms.


> >         }
> >         pub fn finish<M:MessageSink>(self, ad: &dyn EncryptAdditionalInfo,
> > msg: &[u8], to: M) ->
> >                 Result<<<M as MessageSink>::Buffer as
> > MessageBuffer>::Residual, String>
> >         {
> >                 //...
> >         }
> > }
> >
> > Any current other-type recipient (which is pretty much anything that is
> > not a Direct Encryption or Direct Key Agreement) can be supported via
> > that interface.
> >
> > But it can not support anything that mixes layers. Hence mixing layers
> > is a breaking change.
> >
> 
> I don't get why it cannot, if it's because the protected header is not
> known yet... You could refactor your code.

It is a library interface. Refactoring it is breaking change by
definition.


> Why do all the "recipient work", only to figure out that AES-CBC is
> requested and your library has decided to not implement it...
> or is prohibited from implementing it... as is the case for JOSE.

EncryptionAlgorithm does not have variants for AES-CBC, so it is
impossible to request it.


> > And COSE design clearly intends for the layers to be kept separate.
> >
> >
> Then its design needs to account for attacks that exploit the layer
> separation.

The only mistake there was leaving one critical security requirement
implicit.


> > Yes, as long as the HPKE enc is not in protected headers, that is
> > completely reasonable thing to do.
> >
> 
> I've implemented both HPKE modes in JOSE and COSE.

There is a big difference between doing that from scratch, or starting
from existing library with its existing API, with only non-breaking
changes allowed.

 
> It's working fine in protected headers for me...
> 
> I don't think it needs to be in protected headers, but in compact JWE...
> that's the only header, so that's where it goes... ( encrypted JWT ...
> digital identity / credential use cases ).

There is also JWE Encrypted Key.

Or heck, even JWE Initialization Vector (sure, not meant for that, but
it is otherwise unused).


> This also lines up with traditional compact JWE encryption...
> 
>   const key1 = await jose.generateKeyPair('ECDH-ES+A128KW', { crv: 'P-256',
> extractable: true })

This is much closer to Key Encryption, which does not use headers.

 
> I am trying to make HPKE AAD take protected headers, because that's how
> they are protected in JOSE.

Only for topmost encryption.

Protected headers are not protected in alg processing, even if alg would
be capable of doing that (see how A128GCMKW works).

 
> I'm also trying to place "encapsulated keys" in protected and unprotected
> headers, because that's where "epk" goes in JOSE.

Key Encryption (which this is) traditionally does not use headers for
material.


> > And JWE clearly intends an API analogous to the above COSE example to
> > work. Again, it is broken by mixing layers. There is even a precedent on
> > how AEAD-capable algorithm behaves: It does not mix layers.
> >
> 
> counterpoint, see the JOSE examples above:
> 
> expect(protectedHeader.alg).toBe('ECDH-ES+A128KW')
> expect(protectedHeader.enc).toBe('A128GCM')
> 
> If you set A128GCM AAD to be the protected header, your AEAD is providing
> integrity for your key agreement and key wrapping algorithm identifier.

That alg is not capable of protecting anything.

And having such algorithms forces the COSE and JOSE design.


> > > The logic for COSE is the same, regarding proposals 1 and 2.
> >
> > With unfortunate difference that RFC 8152 (nor does RFC 9052) did not
> > explicitly state a critical security requirement. And then some folks
> > went ahead and added a thing that makes it insecure.
> >
> > (Bonus points for making a thing I can't figure out how to use
> > correctly in any useful way...)
> >
> 
> And this is why we have 2 lists included on this thread.
> 
> We don't want to end up making JOSE vulnerable by following COSE advice,
> or make COSE vulnerable by following JOSE advice... There are important
> security differences.

Security-wise COSE and JOSE seem very similar.

 
> We also don't want encapsulated keys treated differently in ways that make
> security analysis difficult.
> 
> Part of the reason why is that we already have security analysis for "epk"
> in both JOSE and COSE,
> and that's conceptually what DHKems are outputting via "enc".

There are major problems with the way "epk" is used. It is just that
ECDH seems rather resilient to exploiting those problems in practice.
Other algorithms might not be so lucky.


> > >From draft-ietf-jose-fully-specified-algorithms, section 5.
> >
> > "So for instance, for JOSE, alg values and enc values MUST each be
> > fully specified, and their behaviors MUST NOT depend upon one another."
> >
> > This means that legal enc and legal alg values can be freely combined.
> >
> 
> I don't follow... is your point that:
> 
> {
>      "alg": "Ed25519",
>      "kid": "77c7e2b8-6e13-45cf-8672-617b5b45243a",
>      "enc": "A128GCM"
> }
> 
> Make no sense?...

Ed25519 is not a legal alg in JWE.


> > > One is fully specified by listing a single AEAD algorithm.
> > >
> > > The other is fully specified by listing an HPKE SUITE, that bundles KEM,
> > > KDF and AEAD together.
> >
> > The problem is that once you have "enc", it will do its thing. Any
> > interference from "alg" is prohibited.
> >
> 
> I think that's an argument in favor of :
> 
> {
>      "alg": "dir",
>      "kid": "77c7e2b8-6e13-45cf-8672-617b5b45243a",
>      "enc": "HPKE-Base-P256-SHA256-AES128GCM"
> }
> 
> For direct / integrated encryption in JOSE?

No. Because anything that works with "dir" has to work with e.g.,
"ECDH-ES".

And JWE requires that "dir" is only used with symmetric keys.

 
> > And once you have "alg", it will do its thing, any interference from
> > "enc" is prohibited.
> 
> What do I do with "dir", if I can't look at "enc" ?

Use the symmetric key given as CEK.

JWE section 5.1. step 6, section 5.2. step 11.


> > > In the context of COSE, we don't have "enc", but we do have fully
> > specified
> > > algorithms for COSE HPKE.
> >
> > COSE has layers. And it is clear that those are not allowed to interfere
> > with one another.
> >
> I'd say it's clear they interfered with each other in ways that produced
> vulnerabilities.

No, the layers don't interfere with each other. That design imposes a
security constraint that was left implicit and then got violated.

 
> Let's return to the solution:
> 
> HPKE AAD = CBOR ( [ "Encrypt1", CBOR ( RecipientProtectedHeader ),  CBOR (
> ProtectedHeader ), external_aad ])

Nothing currently in COSE requires layer 0 protected header to exist
when encrypting at layer 1. And that will make its way into many
implementations.

And what to do if that thing appears at layer 2? There is nothing in
COSE that prohibits anything that can appear at layer 1 from also being
used at layer 2.

And then what if somebody adds some bulk AE (RFC 9052 explicitly defines
the behavior of such thing)? The main protected headers are required to
be empty if that is used!


Security must never rely on anything optional.


> If you think Encrypt1 should never have external aad, you don't need a new
> structure, you can use Encrypt0 with the layer0 protected header AS
> external AAD.

That sounds something very unsafe.


> HPKE AAD = CBOR ( [ "Encrypt0", CBOR ( RecipientProtectedHeader ),  CBOR (
> ProtectedHeader ) ])

COSE_Encrypt0? That only ever has at most single set of protected
headers.


> Logically you are encrypting a content encryption key to a single
> recipient... so this seems... not that terrible.

No, it is logically asymmetric encryption in a single step.


> This won't stop an attacker from ignoring HPKE recipients and toggling
> A128GCM to A128CBC...

The correct way to stop that is to not implement the insecure algorithms
in the first place.

Seriously, it is extremely difficult to reliably disable an algorithm.
Especially so unless the programming language has a powerful type
system. To reliably get rid of it, the implementation must be removed.

There are a lot of CVEs involving "disabled" algorithms rising from the
grave like zombies when you least want that.




-Ilari