Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue with ECDH-1PU-A256KW key agreement algorithm #1

Open
jay-jay-lama opened this issue Jun 21, 2023 · 8 comments
Open

Issue with ECDH-1PU-A256KW key agreement algorithm #1

jay-jay-lama opened this issue Jun 21, 2023 · 8 comments

Comments

@jay-jay-lama
Copy link

Hello
I tried version from main branch
And got an error during encryption process

-CryptoKitError
  - underlyingCoreCryptoError : 1 element
    - error : -7

Code

let privateKey = P256.KeyAgreement.PrivateKey()
let publicKey = privateKey.publicKey
let header: JOSEHeader =  .init(algorithm: .ecdh1PUA256KW, encryptionAlgorithm: .a256CBCHS512)

try JWE.encrypt(
             plaintext: "Test".data(using: .utf8)!,
             to: publicKey.jwkRepresentation,
             from: privateKey.jwkRepresentation,
             protectedHeader: header)

We got this issue here
encryptedKey = try AES.KeyWrap.wrap(.init(data: contentEncryptionKey), using: .init(data: keyEncryptionKey))
When we are trying to create encryptedKey for receiver.

@jay-jay-lama
Copy link
Author

jay-jay-lama commented Jun 22, 2023

I think that the problem is with the contentEncryptionKey

@zssz
Copy link
Collaborator

zssz commented Jun 23, 2023

The key management algorithm, ecdh1PUA256KW, is incompatible with the content encryption algorithm a256CBCHS512. This is due to the fact that a256CBCHS512 generates a 512-bit content encryption key, which ecdh1PUA256KW cannot accommodate because CryptoKit.AES.KeyWrap only supports keys of up to 256 bits.

To resolve this, you have two options:

Option 1: Switch to using a128CBCHS256:

let header: JOSEHeader = .init(algorithm: .ecdh1PUA256KW, encryptionAlgorithm: .a128CBCHS256)

Option 2: Opt out of AES key wrapping:

let header: JOSEHeader = .init(algorithm: .ecdh1PU, encryptionAlgorithm: .a256CBCHS512)

@jay-jay-lama
Copy link
Author

jay-jay-lama commented Jun 24, 2023

Thank you a lot for your answer
Option 1 is working.

I'm sorry for one more question. I'm new to cryptography, so if anything you just can tell that this question is bullshit ))))

I see that during the using a256CBCHS512 we generate a key with length 512 witch is divided into 2 parts :
MAC_KEY and ENC_KEY the length of which is 32 and 32 bytes. (According to documentation)

MAC_KEY - is used to calculate Authentication Tag
ENC_KEY - is used to encrypt plainText

Shouldn't ENC_KEY be used to wrap the key?

And also i looked into the implementation of this a256CBCHS512 algorithm in the library jose swift

Here you can see that even with using A256CBCHS512 they get key for key wrapping with length 256

And here i can see that they are using ENC_KEY to implement key wrapping.
Function

func encrypt(_ plaintext: Data, additionalAuthenticatedData: Data) throws -> ContentEncryptionContext 

Default CryptoKit.AES.KeyWrap and custom wrap implementation in this library returns the equal result

Implementation of key generating

func retrieveKeys(from inputKey: Data) throws -> (hmacKey: Data, encryptionKey: Data) {
           switch self {
           case .A256CBCHS512:
               guard checkKeyLength(for: inputKey) else {
                   throw JWEError.keyLengthNotSatisfied
               }

               return (inputKey.subdata(in: 0..<32), inputKey.subdata(in: 32..<64))

           case .A128CBCHS256:
               guard checkKeyLength(for: inputKey) else {
                   throw JWEError.keyLengthNotSatisfied
               }
               return (inputKey.subdata(in: 0..<16), inputKey.subdata(in: 16..<32))
           }
       } 

I just want to understand witch approach is right...
Thank you

@zssz
Copy link
Collaborator

zssz commented Jun 26, 2023

The confusion is understandable, cryptography can be a complex field! To start with, it's important to note that the key used for AES Key Wrapping and the encryption key used for the content encryption process are not the same.

When using AES Key Wrap, you're wrapping (encrypting) the content encryption key (CEK). The wrapped CEK is then transported securely to the recipient, who unwraps it (decrypts it) to recover the original CEK. This CEK is what's used for encrypting and decrypting the content.

Now, coming to your question about the a256CBCHS512 algorithm, it does indeed generate a 512-bit key which is divided into two 256-bit halves, one for MAC (Message Authentication Code) and one for encryption. However, the key used for encryption here is specifically for encrypting the plaintext data, not for wrapping the CEK.

The function you referred to in the jose swift library is used to generate the MAC_KEY and ENC_KEY from the provided input key (the CEK). This is done after the CEK has been unwrapped.

The key for AES Key Wrap (the key encryption key, or KEK) comes from the key agreement process (ecdh1PU or ecdh1PUA256KW in your example). This is a separate key that's generated by each party independently, using their own private key and the other party's public key.

The observation you made about the jose swift library using ENC_KEY for key wrapping appears to be a misunderstanding. The key wrapping process should be using the KEK derived from the key agreement process, not the ENC_KEY.

In summary, the approach where a 512-bit key is split into two 256-bit halves, one for MAC and one for encryption, is correct. The important thing to remember is that the keys for AES Key Wrap and content encryption are derived from different processes and serve different purposes.

I hope this helps to clarify things!

@jay-jay-lama
Copy link
Author

Hello
Thank you a lot for this answer and for your time.
You helped me to understand more in this topic 🙏

@jay-jay-lama
Copy link
Author

jay-jay-lama commented Jul 6, 2023

Hello
I just wanted to put your attention to that fact that ecdh1PUA256KW and a256CBCHS512 should work.

We have the problem with this function CryptoKit.AES.KeyWrap right?
It works with 2 values:
key to wrap - which is our content key with 512 length (this length can be any)
key encryption key - which is our agreed key... and it's length shouldn't be more than 256

So the problem is with the key encryption key because its length now depends on content alg key length. But according to documentation

keydatalen  This is set to the number of bits in the desired output
      key.  For "ECDH-1PU", this is the length of the key used by the
      "enc" algorithm.  For "ECDH-1PU+A128KW", "ECDH-1PU+A192KW", and
      "ECDH-1PU+A256KW", this is 128, 192, and 256, respectively.

It should be equal to 256 because of A256KW... we should take the "enc" key length only for "ECDH-1PU"
So the problem in deriveKey function
line 71 let keyDataLen = encryptionAlgorithm.keySizeInBits

@jay-jay-lama jay-jay-lama reopened this Jul 6, 2023
@beatt83
Copy link

beatt83 commented Dec 28, 2023

Hi there. :)
First let me say I really like a few components on this framework. They are really well designed.

I had the same problem and actually was checking a few things, I noticed that there is actually a bug in this key management algorithm, in the deriveKey. Since this algorithm requires you to encrypt the data BEFORE generating the shared key, because you need the authenticationTag when deriving the shared key.

This is documented in the ECDH-1PU draft rfc https://datatracker.ietf.org/doc/html/draft-madden-jose-ecdh-1pu-04#section-2.3

@jay-jay-lama
Copy link
Author

@beatt83 Hey
Thank you a lot
Wow i've really missed it in the documentation
I hope that developers of this awesome library will fix these tiny things
Thank you @zssz one more time. Your implementation is really helped me so much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants