Add support for EC256 keys that are stored inside the Secure Enclave #156

mschwaig · 2019-06-07T14:48:32Z

This PR changes the EC crypto implemenation to use SecKeyCreateSignature and SecKeyVerfiySignature for signature creation and verification and adds the required conversions to and from BER encoded ASN.1 format for the signatures.
It also introduces negative tests for EC signature verification, to detect trivial false acceptances for the signature validation. This is important because the tests for the signing also rely on the validation implementation.

See #155 for the discussion which lead to this PR.

Please be thorough with the iOS side of the review, since I do not have that much experience as developer for iOS.

* This is meant to enable keeping keys inside the Secure Enclave. * Parses ASN1 signature and extracts raw parameters R and S with fixed length. * Implementation passes tests, but does not seem to work with keys generated inside the Secure Enclave for some reason. * EC signature verification is not changed yet and still uses SecKeyRawVerify.

JOSESwift/Sources/CryptoImplementation/EC.swift

Jose-ElRobot · 2019-06-07T14:55:13Z

JOSESwift/Sources/CryptoImplementation/EC.swift

+        var cfErrorRef: Unmanaged<CFError>?
+        let signature = SecKeyCreateSignature(privateKey, secKeyAlgorithm, signingInput as CFData, &cfErrorRef)
+        if cfErrorRef != nil {
+            throw ECError.signingFailed(description: "Error creating signature. (CFError: \(cfErrorRef!.takeRetainedValue()))")


⚠️ Force unwrapping should be avoided.
force_unwrapping EC.swift:188

Your code is perfectly fine. A little trick to get rid of the force unwrapped warnings would be to use a let binding.

For example something like this (taken from Apple's documentation):

var error: Unmanaged<CFError>? guard let signature = SecKeyCreateSignature(privateKey, secKeyAlgorithm, signingInput as CFData, &error) as Data? else { // swiftlint:disable:next force_unwrapping throw error!.takeRetainedValue() as Error }

Now you can use signature without casting and force unwrapping below.

Should I opt to cast and throw the retained value or add it to the description string like I am currently doing?

I did the description string thing for now.

Jose-ElRobot · 2019-06-07T14:55:14Z

JOSESwift/Sources/CryptoImplementation/EC.swift

        }

-        return signature as Data
+        // unpack BER encoded ASN.1 format signature to raw format as specified for JWS
+        let ecSignatureTLV = [UInt8](signature! as Data)


⚠️ Force unwrapping should be avoided.
force_unwrapping EC.swift:192

Is there a preferable alternative to the force unwrap here?
The signature should always be non-null at this point since the contract of SecKeyCreateSignature only allow nil here on failure.
If not I would add a comment and disable the linter here.

Found another way to fix address this with an if let inside a guard let and a fatalError if they mismatch.

JOSESwift/Sources/CryptoImplementation/EC.swift

Jose-ElRobot · 2019-06-07T14:55:16Z

	1 Message
📖	Any non-trivial changes to code should be reflected in the changelog. Please consider adding a note in the Unreleased section of the CHANGELOG.md.

Generated by 🚫 Danger

ghost · 2019-06-07T15:55:51Z

Awesome, thanks @mschwaig! 🎉

I'll get back to you with a review early next week.

ghost

Pretty awesome, great job @mschwaig! 👏 Also thanks for extending the tests!

I left a couple of clarification questions just to make sure I understand everything correctly.

Your implementation looks perfectly fine to me. If possible, it would be nice to get rid of the force unwrappings wherever possible. Wherever you feel that they are justified, feel free to leave a clarifying comment and disable the linter for that line. See here on how to do that.

One more suggestion from my side: What would you think about moving the DER/ASN.1 packing and unpacking code to functions that can be tested independently from the signing/verifying code
logic? Since this is the crucial feature of this pull request, it would be awesome to be able to test them independently.

Let me know what you think! After that, we'll be able to merge and release an update right away!

Again, great job and thanks for taking care of this.

JOSESwift/Sources/CryptoImplementation/EC.swift

ghost · 2019-06-11T11:53:14Z

JOSESwift/Sources/CryptoImplementation/EC.swift

+        var cfErrorRef: Unmanaged<CFError>?
+        let signature = SecKeyCreateSignature(privateKey, secKeyAlgorithm, signingInput as CFData, &cfErrorRef)
+        if cfErrorRef != nil {
+            throw ECError.signingFailed(description: "Error creating signature. (CFError: \(cfErrorRef!.takeRetainedValue()))")


Your code is perfectly fine. A little trick to get rid of the force unwrapped warnings would be to use a let binding.

For example something like this (taken from Apple's documentation):

var error: Unmanaged<CFError>? guard let signature = SecKeyCreateSignature(privateKey, secKeyAlgorithm, signingInput as CFData, &error) as Data? else { // swiftlint:disable:next force_unwrapping throw error!.takeRetainedValue() as Error }

Now you can use signature without casting and force unwrapping below.

ghost · 2019-06-11T11:54:04Z

JOSESwift/Sources/CryptoImplementation/EC.swift

+            let fixlenR = varlenR.withLengthFixedTo(curveType.coordinateOctetLength)
+            let fixlenS = varlenS.withLengthFixedTo(curveType.coordinateOctetLength)
+
+            return fixlenR + fixlenS


☝️ That's pretty nice. 👏

Glad to see that the existing ASN.1 utilities could be reused.

One more suggestion from my side: What would you think about moving the DER/ASN.1 packing and unpacking code to functions that can be tested independently from the signing/verifying code
logic? Since this is the crucial feature of this pull request, it would be awesome to be able to test them independently.

I think testing this logic independently would be a good idea, but I would not want to extend the implementation to support parsing more than what is strictly required for this use case or have these functions exposed to other parts of the code in a way that suggests they can be reused elsewhere when actually right now they only work within the constraints that apply for this use case.

I have to look into how to scope things in Swift to achieve this and have it be testable.

That is a good point. For testing, we would have to raise the access control level from fileprivate to internal. I totally agree with you that that might suggest that the functions could be used from elsewhere in the code base- That's certainly not what we want.

What would you think of having them as an extension of ECCurveType that would clearly namespace it as something EC internal and would even give you direct access to the coordinateOctetLength. Just an idea. Let me know what you think, I'm open for suggestions.

I am halfway-done with the tests on my work machine. Currently I am using another struct inside of the EC struct with two static functions to avoid the extension getting exposed everywhere, what you are suggesting sounds like it may be a better idea.

I'll leave it up to you how to best handle this. 👍

ghost · 2019-06-11T11:54:30Z

JOSESwift/Sources/CryptoImplementation/EC.swift

+
+            return fixlenR + fixlenS
+        } catch {
+            throw ECError.signingFailed(description: "could not unpack ASN.1 EC signature")


Suggested change

throw ECError.signingFailed(description: "could not unpack ASN.1 EC signature")

throw ECError.signingFailed(description: "Could not unpack ASN.1 EC signature.")

JOSESwift/Sources/CryptoImplementation/EC.swift

Jose-ElRobot · 2019-06-11T13:59:00Z

JOSESwift/Sources/CryptoImplementation/EC.swift

        }

-        return signature as Data
+        // unpack BER encoded ASN.1 format signature to raw format as specified for JWS
+        let ecSignatureTLV = [UInt8](signature! as Data)


⚠️ Force unwrapping should be avoided.
force_unwrapping EC.swift:204

Jose-ElRobot · 2019-06-11T13:59:01Z

JOSESwift/Sources/CryptoImplementation/EC.swift

+        guard let secKeyAlgorithm = algorithm.secKeyAlgorithm else {
+            throw ECError.algorithmNotSupported
+        }
+        if (signature.count != curveType.coordinateOctetLength * 2) {


⚠️ if, for, guard, switch, while, and catch statements shouldn’t unnecessarily wrap their conditionals or arguments in parentheses.
control_statement EC.swift:235

ghost

Great job @mschwaig, thanks for going over it again and resolving all remaining issues.

Let me know what you think regarding testing the two asn1/fixedlength functions. If you want we can also track an issue for that and add it in a separate pull request. If you want I can handle that for you.

JOSESwift/Sources/CryptoImplementation/EC.swift

ghost · 2019-06-12T12:04:50Z

JOSESwift/Sources/CryptoImplementation/EC.swift

+            let fixlenR = varlenR.withLengthFixedTo(curveType.coordinateOctetLength)
+            let fixlenS = varlenS.withLengthFixedTo(curveType.coordinateOctetLength)
+
+            return fixlenR + fixlenS


That is a good point. For testing, we would have to raise the access control level from fileprivate to internal. I totally agree with you that that might suggest that the functions could be used from elsewhere in the code base- That's certainly not what we want.

What would you think of having them as an extension of ECCurveType that would clearly namespace it as something EC internal and would even give you direct access to the coordinateOctetLength. Just an idea. Let me know what you think, I'm open for suggestions.

mschwaig · 2019-06-12T18:13:30Z

Great job @mschwaig, thanks for going over it again and resolving all remaining issues.

Let me know what you think regarding testing the two asn1/fixedlength functions. If you want we can also track an issue for that and add it in a separate pull request. If you want I can handle that for you.

I am about halfway done with those tests. I prepared test data for all of the cases I think are relevant, but I did not implement any actual test cases yet.
I will only be able to continue working on them after NovaRock. Adding the tests in a seperate PR would be good I think, but either way is fine for me. Feel free to proceed with this however you like.

mschwaig · 2019-06-12T18:27:27Z

One thing I am still concerned about is if the EC implementation still behaves in the same way as before for negative cases like invalid signatures or blatantly incorrectly formatted signatures, since the verify method can return false but also throw errors. This negative case behaviour might be inconsistent between the old and new implementation. I cannot judge if this would be an issue, I remember thinking maybe also differs for RSA and EC right now, but I did not investigate further.

ghost · 2019-06-14T07:24:11Z

One thing I am still concerned about is if the EC implementation still behaves in the same way as before for negative cases like invalid signatures or blatantly incorrectly formatted signatures, since the verify method can return false but also throw errors.

Good point. Your implementation now behaves as its RSA counterpart. It returns true/false for valid/invalid signatures and throws in case of an error. The old SecKeyRawVerify did not return a boolean but only a status code. I think mirroring the behavior of the newer SecKeyVerifySignature, as you did, is the way to go. What do you think?

I will only be able to continue working on them after NovaRock. Adding the tests in a seperate PR would be good I think, but either way is fine for me. Feel free to proceed with this however you like.

Ok, cool! I'll merge this pull request now but will hold off on releasing the change until the tests are in. That way we get smaller and easier reviewable pull requests while still maintaining the quality of the release.

Have fun at Nova Rock! ☀️ 🇦🇹 🎸 🍻

mschwaig · 2019-06-14T07:30:07Z

Sounds great, thanks. 😁

ghost · 2019-06-14T12:26:53Z

For completeness, I tracked #157. Let me know if you need anything!

Martin Schwaighofer added 4 commits May 29, 2019 16:15

make BER parsing more robust, update comments and error messages

c6d9498

add negative tests for EC signature verification

2970920

switch EC verification implementation to SecKeyVerfiySignature

98f7c96

Jose-ElRobot reviewed Jun 7, 2019

View reviewed changes

JOSESwift/Sources/CryptoImplementation/EC.swift Outdated Show resolved Hide resolved

Jose-ElRobot reviewed Jun 7, 2019

View reviewed changes

JOSESwift/Sources/CryptoImplementation/EC.swift Outdated Show resolved Hide resolved

Jose-ElRobot reviewed Jun 7, 2019

View reviewed changes

JOSESwift/Sources/CryptoImplementation/EC.swift Outdated Show resolved Hide resolved

ghost self-assigned this Jun 7, 2019

ghost reviewed Jun 11, 2019

View reviewed changes

JOSESwift/Sources/CryptoImplementation/EC.swift Outdated Show resolved Hide resolved

address review comments

d5682db

Jose-ElRobot reviewed Jun 11, 2019

View reviewed changes

Martin Schwaighofer added 2 commits June 11, 2019 16:20

update changelog

dd7e767

address more review comments by robot

212382f

ghost approved these changes Jun 12, 2019

View reviewed changes

ghost merged commit af77cc8 into airsidemobile:master Jun 14, 2019

This was referenced Jun 14, 2019

Add tests for encoding and decoding of raw ECDSA signatures to and from DER ASN.1 #157

Closed

JWS Signing with Secure Enclave keys #155

Closed

mschwaig mentioned this pull request Jun 19, 2019

Add tests for conversion from and to ASN.1 Integers for EC signatures #160

Merged

ghost mentioned this pull request Jun 27, 2019

Prepare 1.8.1 #161

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for EC256 keys that are stored inside the Secure Enclave #156

Add support for EC256 keys that are stored inside the Secure Enclave #156

mschwaig commented Jun 7, 2019

Jose-ElRobot Jun 7, 2019

ghost Jun 11, 2019

mschwaig Jun 11, 2019

mschwaig Jun 12, 2019

Jose-ElRobot Jun 7, 2019

mschwaig Jun 11, 2019

mschwaig Jun 12, 2019

Jose-ElRobot commented Jun 7, 2019 •

edited

ghost commented Jun 7, 2019

ghost left a comment •

edited by ghost

ghost Jun 11, 2019

ghost Jun 11, 2019

mschwaig Jun 11, 2019

ghost Jun 12, 2019

mschwaig Jun 12, 2019

ghost Jun 14, 2019

ghost Jun 11, 2019

Jose-ElRobot Jun 11, 2019 •

edited

Jose-ElRobot Jun 11, 2019 •

edited

ghost left a comment

ghost Jun 12, 2019

mschwaig commented Jun 12, 2019

mschwaig commented Jun 12, 2019

ghost commented Jun 14, 2019 •

edited by ghost

mschwaig commented Jun 14, 2019

ghost commented Jun 14, 2019

	throw ECError.signingFailed(description: "could not unpack ASN.1 EC signature")
	throw ECError.signingFailed(description: "Could not unpack ASN.1 EC signature.")

Add support for EC256 keys that are stored inside the Secure Enclave #156

Add support for EC256 keys that are stored inside the Secure Enclave #156

Conversation

mschwaig commented Jun 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jose-ElRobot commented Jun 7, 2019 • edited

ghost commented Jun 7, 2019

ghost left a comment • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jose-ElRobot Jun 11, 2019 • edited

Choose a reason for hiding this comment

Jose-ElRobot Jun 11, 2019 • edited

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mschwaig commented Jun 12, 2019

mschwaig commented Jun 12, 2019

ghost commented Jun 14, 2019 • edited by ghost

mschwaig commented Jun 14, 2019

ghost commented Jun 14, 2019

Jose-ElRobot commented Jun 7, 2019 •

edited

ghost left a comment •

edited by ghost

Jose-ElRobot Jun 11, 2019 •

edited

Jose-ElRobot Jun 11, 2019 •

edited

ghost commented Jun 14, 2019 •

edited by ghost