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

JWS Signing with Secure Enclave keys #155

Closed
martinmitrevski opened this issue May 22, 2019 · 10 comments
Closed

JWS Signing with Secure Enclave keys #155

martinmitrevski opened this issue May 22, 2019 · 10 comments
Labels
bug Something is not working right help wanted We'd love for someone from the community to help out here

Comments

@martinmitrevski
Copy link

Hello,

I've tried to sign JWS ES256 with a key generated from the Secure Enclave (kSecAttrTokenID = kSecAttrTokenIDSecureEnclave).

The signing fails inside EC.swift -> let status = SecKeyRawVerify(publicKey, .sigRaw, digest, digest.count, signatureBytes, curveType.signatureOctetLength).

I've also tried changing the code a bit, with:
SecKeyCreateSignature(privateKey, .ecdsaSignatureMessageX962SHA256, test as CFData, &error) instead. The signing passes, but the JWS is invalid.

Do you have any suggestions regarding this issue?

Thanks,
Martin

@ghost
Copy link

ghost commented May 24, 2019

Hi Martin!

Thanks for reporting this issue. As discussed via email I can also reproduce this behavior.

Some background

I looked around and found references to this issue on the Apple developer forums and also this issue on GitHub. From what I could gather this seems like a bug in SecKeyRawSign in combination with .sigRaw as padding and a key from the secure enclave. Apple seems to recommend SecKeyCreateSignature over SecKeyRawSign as a workaround for this bug according to this thread.

As you said, the problem with SecKeyCreateSignature is that it creates an invalid JWS signature. That, of course, doesn't mean that the signature is invalid, it's just not in the representation that the JWS RFC defines. As per RFC-7515:

The result of the digital signature is the Elliptic Curve (EC) point (R, S), where R and S are unsigned integers. (...) The JWS Signature is the value R || S.

If we were to use SecKeyRawSign with a padding other than .sigRaw or even SecKeyCreateSignature, the returned signature should be an ASN.1 object from where we would have to parse out the "raw" signature as defined by RFC-7514. For more background see the mentioned Apple developer forums thread, GitHub issue, or the SecPadding docs.

Now what can we do to fix this situation?

I would propose to update to using SecKeyCreateSignature in EC.swift since that seems to be what Apple recommends here. However, I'd also be fine with using another padding than .sigRaw together with SecKeyRawSign. As mentioned both those functions should return an ASN.1 object.

In either case, to be compliant with the standard, we'd have to then decode the resulting ASN.1 object to extract the "raw" JWS signature. We're in luck since it seems like other people have been in this exact situation. I would propose we adapt their ASN.1 decoding here for our use case.

I hope that makes sense. I'm fairly confident that this should resolve the issue.

Next steps

To be honest, at the time I'm not sure how fast we can come up with a fix for this issue. It could be that it takes a couple of weeks but could also be faster.
Of course, if you have the time, we'd be more than happy to review a pull request or proposals for other ways to resolve this.

It would be great if people could use JOSESwift with keys from the secure enclave. Let me know what you think. 🎉

@ghost ghost self-assigned this May 24, 2019
@ghost ghost added bug Something is not working right help wanted We'd love for someone from the community to help out here labels May 24, 2019
@ghost ghost mentioned this issue May 24, 2019
@mschwaig
Copy link
Contributor

mschwaig commented May 29, 2019

I am a colleague of @martinmitrevski working on implementing a fix for this issue.

I implemented your proposal, @daniel-mohemian, but for some reason the resulting signatures are still invalid.

I also did not change the verification code yet.

You can find the code here:
https://github.com/mschwaig/JOSESwift/tree/secure-enclave

@ghost
Copy link

ghost commented May 30, 2019

Hi @mschwaig,

I had a quick look at @martinmitrevski fork yesterday and used it to generate a JWS, signed by a key from the secure enclave. Then, I used Nimbus-JOSE to check the validity of the JWS for the corresponding public key and got a successful validation.

Your implementation looks very similar. Since I'm out of the office until Monday, I could not test it with a real device but only using the simulator (no secure enclave, but this should not be a problem anymore since we're now using SecKeyCreateSignature). I also could verify the signature of a JWS generated with your fork using Nimbus-JOSE.

However those were just quick tests on the go, I could very well have overlooked something. I'd love to check the signatures against OpenSSL when I'm back in the office.

for some reason the resulting signatures are still invalid.

How were you checking the signatures when you tested your implementation?

As I said I'll be out of the office until Monday. Let me know if you find out anything in the meantime!

@martinmitrevski
Copy link
Author

Yes, that's the same implementation, by @mschwaig. I just forked it from his, to quickly test it in our project.

According to me, the validation should pass even with the validation methods that are already in the JOSESwift library, since at the end we are extracting the raw signature.

@mschwaig
Copy link
Contributor

Hi @daniel-mohemian!

You can disregard @martinmitrevski fork and look at mine instead.

Just to clarify the origin of the code

@martinmitrevski's fork is an earlier state of my code + some small changes he made to the error handling, since I do not have much experience with Swift yet. This fork still relies on a file from CupertinoJWT for the ASN.1 parsing, which I wanted to get rid of later.

My fork is based on that earlier state of my code, but already updated so it does the ASN.1 parsing without code from CupertinoJWT.

About what we tested

The JOSESwift tests are passing for both forks, they should both generate valid signatures.

However: Even though it should not make a difference, when @martinmitrevski tried generating a signature on an iOS device using his fork with a key that is stored inside the secure enclave he cannot validate that signature on iOS using JOSESwift and I also could not validate his signature using Nimbus JOSE+JWT. The format looks correct. We do not know why this fails, the signature format looks correct. If he only changes the key generation code to not use the secure enclave it somehow works.

I could not test my fork on an actual iOS device yet, since I also do not have access to one right now, but I think the situation should still be the same.

I also think it would be great to get this working. 👍

@ghost
Copy link

ghost commented Jun 3, 2019

Hi @mschwaig!

Back in the office, I put together a quick sample project using your fork and a key from the secure enclave. Everything seemed to work fine. Additionally, I could verify the signature generated using your fork using Nimbus.

I'll attach my sample project in case you're interested. Did you have the chance yet to verify on your end that your fork works? Better safe than sorry as my sample project really is just a quick test. 😄

Interesting that @martinmitrevski's fork doesn't seem to work. I wonder why that is. If you find out something, let me know. I'll also try to have another look at it during the next few days if possible.

In the meantime, as it looks like your fork is working (at least on my end, it would be awesome if you guys could do some tests on your end as well), would it work out for you if we aim to get your fork into the next release of JOSESwift?

I'd still love to do some more testing, ideally using OpenSSL in addition to iOS's Security.framework and Nimbus. If things work out there and verifying secure enclave signatures work on your end, we should be pretty much good to go.

Example Project: JOSESecureEnclave.zip

Nimbus code:

// The x and y coordinates can be obtained from the public key using the JOSESwift ECPublicKey class.
// See the JOSESwiftSecureEnclave example project.
Curve crv = Curve.P_256;
Base64URL x = new Base64URL("dG (...) 1o");
Base64URL y = new Base64URL("LG (...) Nc");
ECKey jwk = new ECKey(crv, x, y, null, null, null, null, null, null, null, null, null);

JWSVerifier verifier = new ECDSAVerifier(jwk);

// The compact serialization from the JOSESwiftSecureEnclave example project.
JWSObject jws = JWSObject.parse("ey (...) J9.SG (...) SE.ZO (...) yw");

System.out.println("Message: " + jws.getPayload().toString());

if (jws.verify(verifier)) {
    System.out.println("Signature is valid.");
} else {
    System.out.println("Signature is not valid!");
}

@mschwaig
Copy link
Contributor

mschwaig commented Jun 4, 2019

It would be great if we could get this into the next release.

I propose me and Martin will:

  1. Add tests that cover the EC verification code using hardcoded inputs and including negative tests with invalid signatures. Otherwise if signature creation and verification are both broken in the same way the tests would still pass.
  2. Evaluate if we can additionally replace SecKeyRawVerify in the verification code.
  3. Independently verify that things are working with keys inside the Secure Enclave. Most likely we just made an error in our initial testing on iOS and we can get it to work now.

We will let you know what we find out during testing and I will create a PR from my fork that includes the code changes that arise from the first and second point.

You can then review that PR. Is that ok?

EDIT: Looking at the code I think there may already be tests for the verification code against fixed inputs.

@ghost
Copy link

ghost commented Jun 5, 2019

That sounds great! Also, feel free to extend adapt or improve the test suite as you see fit. 👍

Let me know if you need anything.

@mschwaig
Copy link
Contributor

mschwaig commented Jun 7, 2019

Hi Daniel!

I implemented what I proposed in the post above and I verified your example project against the documentation I could find on using the Secure Enclave for signing, mostly this.

I also verified that the generated signatures are successfully validated by Nimbus JOSE+JWT.

Everything looks good to me, so I have opened a PR.
Due to unrelated issues we could not validate this with our productive project yet, but we still want to take advantage of this feature if it lands in an official release.

@ghost
Copy link

ghost commented Jun 14, 2019

Resolved, by #156. Will be released after #157.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working right help wanted We'd love for someone from the community to help out here
Projects
None yet
Development

No branches or pull requests

2 participants