[Darwin] add API to get public key without leaks (with fixes) (#36340)

* Initial checkin * Fixing review feedback * Adding braces * Restyled by clang-format * Fixing build * Restyled by clang-format * Fixing annotations * Update src/darwin/Framework/CHIP/MTRKeypair.h Co-authored-by: Boris Zbarsky <[email protected]> * separate statements involving _mPublicKey to make compiler happy * use `CFAutorelease` on CoreFoundation typed public key copies * fix indent * implement `copyPublicKey` for `MTRTestKeys`; add TODO note about optional method calls * remove comment it's a test; this is the best we can do with an optional protocol method * consistent formatting for `copyPublicKey` calls * reformat `copyPublicKey` in `MTRTestKeys` remove comment - not this method's job to worry about other implementation's potential side-effects * Update src/darwin/Framework/CHIP/MTRKeypair.h Co-authored-by: Boris Zbarsky <[email protected]> * re-add `publicKey` in `CHIPToolKeypair` check compatibility in CI before going back and removing * use `copyPublicKey` in more places * dedupe public key cache population for CHIPToolKeypair * Restyled by clang-format * try manually releasing pubkey in test * leakfix: MTRP256KeypairBridge * leakfix: `MTRCertificateTests.testGenerateCSR` * remove unnecessary plain `publicKey` implementation * temporarily do the inelegant patch on CFAutorelease * fix another remaining `publicKey` instance * Restyled by clang-format * fix `publicKey` deprecation, marking for future release --------- Co-authored-by: Justin Wood <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Nov 15, 2024 · e6bbe66 · e6bbe66
1 parent 4c53f7d
commit e6bbe66
Show file tree

Hide file tree

Showing 14 changed files with 199 additions and 44 deletions.
diff --git a/examples/darwin-framework-tool/commands/common/CHIPCommandBridge.mm b/examples/darwin-framework-tool/commands/common/CHIPCommandBridge.mm
@@ -172,10 +172,17 @@
 
         NSError * error;
         __auto_type * operationalKeypair = [certificateIssuer issueOperationalKeypairWithControllerStorage:controllerStorage error:&error];
+        SecKeyRef publicKey = [operationalKeypair copyPublicKey];
+
         __auto_type * operational = [certificateIssuer issueOperationalCertificateForNodeID:nodeId
                                                                                    fabricID:fabricId
-                                                                                  publicKey:operationalKeypair.publicKey
+                                                                                  publicKey:publicKey
                                                                                       error:&error];
+
+        if (publicKey != NULL) {
+            CFAutorelease(publicKey);
+        }
+
         VerifyOrReturnError(nil == error, MTRErrorToCHIPErrorCode(error), ChipLogError(chipTool, "Can not issue an operational certificate: %@", error));
 
         __auto_type * controllerStorageQueue = dispatch_queue_create("com.chip.storage", DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);

diff --git a/examples/darwin-framework-tool/commands/common/CHIPToolKeypair.h b/examples/darwin-framework-tool/commands/common/CHIPToolKeypair.h
@@ -22,7 +22,7 @@
 @interface CHIPToolKeypair : NSObject <MTRKeypair>
 - (BOOL)initialize;
 - (NSData *)signMessageECDSA_RAW:(NSData *)message;
-- (SecKeyRef)publicKey;
+- (SecKeyRef)copyPublicKey;
 - (CHIP_ERROR)Serialize:(chip::Crypto::P256SerializedKeypair &)output;
 - (CHIP_ERROR)Deserialize:(chip::Crypto::P256SerializedKeypair &)input;
 - (CHIP_ERROR)createOrLoadKeys:(id)storage;

diff --git a/examples/darwin-framework-tool/commands/common/CHIPToolKeypair.mm b/examples/darwin-framework-tool/commands/common/CHIPToolKeypair.mm
@@ -65,7 +65,7 @@ - (NSData *)signMessageECDSA_RAW:(NSData *)message
     return out_signature;
 }
 
-- (SecKeyRef)publicKey
+- (SecKeyRef)copyPublicKey
 {
     if (_mPublicKey == nil) {
         chip::Crypto::P256PublicKey publicKey = _mKeyPair.Pubkey();
@@ -79,7 +79,13 @@ - (SecKeyRef)publicKey
         };
         _mPublicKey = SecKeyCreateWithData((__bridge CFDataRef) publicKeyNSData, (__bridge CFDictionaryRef) attributes, nullptr);
     }
-    return _mPublicKey;
+
+    if (_mPublicKey) {
+        CFRetain(_mPublicKey);
+        return _mPublicKey;
+    }
+
+    return NULL;
 }
 
 - (CHIP_ERROR)Deserialize:(chip::Crypto::P256SerializedKeypair &)input

diff --git a/src/darwin/Framework/CHIP/MTRCertificates.mm b/src/darwin/Framework/CHIP/MTRCertificates.mm
@@ -152,7 +152,24 @@ + (MTRCertificateDERBytes _Nullable)createOperationalCertificate:(id<MTRKeypair>
 + (BOOL)keypair:(id<MTRKeypair>)keypair matchesCertificate:(NSData *)certificate
 {
     P256PublicKey keypairPubKey;
-    CHIP_ERROR err = MTRP256KeypairBridge::MatterPubKeyFromSecKeyRef(keypair.publicKey, &keypairPubKey);
+    SecKeyRef publicKey = NULL;
+
+    if ([keypair respondsToSelector:@selector(copyPublicKey)]) {
+        publicKey = [keypair copyPublicKey];
+    } else {
+        publicKey = [keypair publicKey];
+        if (publicKey) {
+            CFRetain(publicKey);
+        }
+    }
+
+    CHIP_ERROR err = MTRP256KeypairBridge::MatterPubKeyFromSecKeyRef(publicKey, &keypairPubKey);
+
+    if (publicKey != NULL) {
+        CFRelease(publicKey);
+        publicKey = NULL;
+    }
+
     if (err != CHIP_NO_ERROR) {
         MTR_LOG_ERROR("Can't extract public key from keypair: %s", ErrorStr(err));
         return NO;

diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm
@@ -819,7 +819,24 @@ - (BOOL)findMatchingFabric:(FabricTable &)fabricTable
     } else {
         // No root certificate means the nocSigner is using the root keys, because
         // consumers must provide a root certificate whenever an ICA is used.
-        CHIP_ERROR err = MTRP256KeypairBridge::MatterPubKeyFromSecKeyRef(params.nocSigner.publicKey, &pubKey);
+        SecKeyRef publicKey = NULL;
+
+        if ([params.nocSigner respondsToSelector:@selector(copyPublicKey)]) {
+            publicKey = [params.nocSigner copyPublicKey];
+        } else {
+            publicKey = [params.nocSigner publicKey];
+            if (publicKey) {
+                CFRetain(publicKey);
+            }
+        }
+
+        CHIP_ERROR err = MTRP256KeypairBridge::MatterPubKeyFromSecKeyRef(publicKey, &pubKey);
+
+        if (publicKey != NULL) {
+            CFRelease(publicKey);
+            publicKey = NULL;
+        }
+
         if (err != CHIP_NO_ERROR) {
             MTR_LOG_ERROR("Can't extract public key from MTRKeypair: %s", ErrorStr(err));
             return NO;

diff --git a/src/darwin/Framework/CHIP/MTRKeypair.h b/src/darwin/Framework/CHIP/MTRKeypair.h
@@ -16,6 +16,7 @@
  */
 
 #import <Foundation/Foundation.h>
+#import <Matter/Matter.h>
 #import <Security/Security.h>
 
 NS_ASSUME_NONNULL_BEGIN
@@ -31,13 +32,18 @@ NS_ASSUME_NONNULL_BEGIN
  * framework APIs.
  */
 @protocol MTRKeypair <NSObject>
-@required
+
+@optional
 /**
- * @brief Return public key for the keypair.
+ * @brief Returns a copy of the public key for the keypair.
  */
-- (SecKeyRef)publicKey;
+- (SecKeyRef)copyPublicKey MTR_NEWLY_AVAILABLE;
+
+/**
+ * @brief Returns public key for the keypair without adding a reference. DEPRECATED - please use copyPublicKey, otherwise this will leak.
+ */
+- (SecKeyRef)publicKey MTR_NEWLY_DEPRECATED("Please implement copyPublicKey, this will leak otherwise");
 
-@optional
 /**
  * @brief A function to sign a message using ECDSA
  *

diff --git a/src/darwin/Framework/CHIP/MTRP256KeypairBridge.mm b/src/darwin/Framework/CHIP/MTRP256KeypairBridge.mm
@@ -132,7 +132,17 @@
     return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE;
 }
 
-CHIP_ERROR MTRP256KeypairBridge::setPubkey() { return MatterPubKeyFromSecKeyRef([mKeypair publicKey], &mPubkey); }
+CHIP_ERROR MTRP256KeypairBridge::setPubkey()
+{
+    if ([mKeypair respondsToSelector:@selector(copyPublicKey)]) {
+        SecKeyRef publicKey = [mKeypair copyPublicKey];
+        auto copyResult = MatterPubKeyFromSecKeyRef(publicKey, &mPubkey);
+        CFRelease(publicKey);
+        return copyResult;
+    } else {
+        return MatterPubKeyFromSecKeyRef([mKeypair publicKey], &mPubkey);
+    }
+}
 
 CHIP_ERROR MTRP256KeypairBridge::MatterPubKeyFromSecKeyRef(SecKeyRef pubkeyRef, P256PublicKey * matterPubKey)
 {