Skip to content

Commit

Permalink
Add new API so it's possible to not leak a key (#36299)
Browse files Browse the repository at this point in the history 
* 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]>

---------

Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
Co-authored-by: Kiel Oleson <[email protected]>
  • Loading branch information
@woody-apple @restyled-commits
@bzbarsky-apple @kiel-apple
4 people authored Nov 1, 2024
1 parent 4efe777 commit 08024d2
Show file tree
Hide file tree
Showing 12 changed files with 170 additions and 41 deletions.
2 changes: 1 addition & 1 deletion examples/darwin-framework-tool/commands/common/CHIPToolKeypair.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
10 changes: 8 additions & 2 deletions examples/darwin-framework-tool/commands/common/CHIPToolKeypair.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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
Expand Down
19 changes: 18 additions & 1 deletion src/darwin/Framework/CHIP/MTRCertificates.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
19 changes: 18 additions & 1 deletion src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
15 changes: 11 additions & 4 deletions src/darwin/Framework/CHIP/MTRKeypair.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

#import <Foundation/Foundation.h>
#import <Matter/Matter.h>
#import <Security/Security.h>

NS_ASSUME_NONNULL_BEGIN
Expand All @@ -31,13 +32,19 @@ 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_DEPRECATED("Please implement copyPublicKey, this will leak otherwise", ios(16.1, 18.3), macos(13.0, 15.3), watchos(9.1, 11.3), tvos(16.1, 18.3));

@optional
/**
* @brief A function to sign a message using ECDSA
*
Expand Down
60 changes: 44 additions & 16 deletions src/darwin/Framework/CHIPTests/MTRCertificateTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,13 @@ - (void)testGenerateIntermediateCert
__auto_type * intermediateKeys = [[MTRTestKeys alloc] init];
XCTAssertNotNil(intermediateKeys);

__auto_type * intermediatePublicKey = [intermediateKeys copyPublicKey];
XCTAssert(intermediatePublicKey != NULL);
CFAutorelease(intermediatePublicKey);

__auto_type * intermediateCert = [MTRCertificates createIntermediateCertificate:rootKeys
rootCertificate:rootCert
intermediatePublicKey:intermediateKeys.publicKey
intermediatePublicKey:intermediatePublicKey
issuerID:nil
fabricID:nil
error:nil];
Expand All @@ -155,13 +159,16 @@ - (void)testGenerateIntermediateCertWithValidityPeriod

__auto_type * intermediateKeys = [[MTRTestKeys alloc] init];
XCTAssertNotNil(intermediateKeys);
__auto_type * intermediatePublicKey = intermediateKeys.copyPublicKey;
XCTAssert(intermediatePublicKey != NULL);
CFAutorelease(intermediatePublicKey);

__auto_type * startDate = [MTRCertificateTests startDateWithTimeIntervalSinceNow:300];
__auto_type * validityPeriod = [[NSDateInterval alloc] initWithStartDate:startDate duration:400];

__auto_type * intermediateCert = [MTRCertificates createIntermediateCertificate:rootKeys
rootCertificate:rootCert
intermediatePublicKey:intermediateKeys.publicKey
intermediatePublicKey:intermediatePublicKey
issuerID:nil
fabricID:nil
validityPeriod:validityPeriod
Expand Down Expand Up @@ -192,13 +199,16 @@ - (void)testGenerateIntermediateCertWithInfiniteValidity

__auto_type * intermediateKeys = [[MTRTestKeys alloc] init];
XCTAssertNotNil(intermediateKeys);
__auto_type * intermediatePublicKey = intermediateKeys.copyPublicKey;
XCTAssert(intermediatePublicKey != NULL);
CFAutorelease(intermediatePublicKey);

__auto_type * startDate = [MTRCertificateTests startDateWithTimeIntervalSinceNow:300];
__auto_type * validityPeriod = [[NSDateInterval alloc] initWithStartDate:startDate endDate:[NSDate distantFuture]];

__auto_type * intermediateCert = [MTRCertificates createIntermediateCertificate:rootKeys
rootCertificate:rootCert
intermediatePublicKey:intermediateKeys.publicKey
intermediatePublicKey:intermediatePublicKey
issuerID:nil
fabricID:nil
validityPeriod:validityPeriod
Expand Down Expand Up @@ -229,6 +239,9 @@ - (void)testGenerateOperationalCertNoIntermediate

__auto_type * operationalKeys = [[MTRTestKeys alloc] init];
XCTAssertNotNil(operationalKeys);
__auto_type * operationalPublicKey = [operationalKeys copyPublicKey];
XCTAssert(operationalPublicKey != NULL);
CFAutorelease(operationalPublicKey);

__auto_type * cats = [[NSMutableSet alloc] initWithCapacity:3];
// High bits are identifier, low bits are version.
Expand All @@ -238,7 +251,7 @@ - (void)testGenerateOperationalCertNoIntermediate

__auto_type * operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:operationalPublicKey
fabricID:@1
nodeID:@1
caseAuthenticatedTags:cats
Expand All @@ -265,6 +278,9 @@ - (void)testGenerateOperationalCertNoIntermediateWithValidityPeriod

__auto_type * operationalKeys = [[MTRTestKeys alloc] init];
XCTAssertNotNil(operationalKeys);
__auto_type * operationalPublicKey = [operationalKeys copyPublicKey];
XCTAssert(operationalPublicKey != NULL);
CFAutorelease(operationalPublicKey);

__auto_type * cats = [[NSMutableSet alloc] initWithCapacity:3];
// High bits are identifier, low bits are version.
Expand All @@ -277,7 +293,7 @@ - (void)testGenerateOperationalCertNoIntermediateWithValidityPeriod

__auto_type * operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:operationalPublicKey
fabricID:@1
nodeID:@1
caseAuthenticatedTags:cats
Expand Down Expand Up @@ -309,6 +325,9 @@ - (void)testGenerateOperationalCertNoIntermediateWithInfiniteValidity

__auto_type * operationalKeys = [[MTRTestKeys alloc] init];
XCTAssertNotNil(operationalKeys);
__auto_type * operationalPublicKey = [operationalKeys copyPublicKey];
XCTAssert(operationalPublicKey != NULL);
CFAutorelease(operationalPublicKey);

__auto_type * cats = [[NSMutableSet alloc] initWithCapacity:3];
// High bits are identifier, low bits are version.
Expand All @@ -321,7 +340,7 @@ - (void)testGenerateOperationalCertNoIntermediateWithInfiniteValidity

__auto_type * operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:operationalPublicKey
fabricID:@1
nodeID:@1
caseAuthenticatedTags:cats
Expand Down Expand Up @@ -353,21 +372,27 @@ - (void)testGenerateOperationalCertWithIntermediate

__auto_type * intermediateKeys = [[MTRTestKeys alloc] init];
XCTAssertNotNil(intermediateKeys);
__auto_type * intermediatePublicKey = [intermediateKeys copyPublicKey];
XCTAssert(intermediatePublicKey != NULL);
CFAutorelease(intermediatePublicKey);

__auto_type * intermediateCert = [MTRCertificates createIntermediateCertificate:rootKeys
rootCertificate:rootCert
intermediatePublicKey:intermediateKeys.publicKey
intermediatePublicKey:intermediatePublicKey
issuerID:nil
fabricID:nil
error:nil];
XCTAssertNotNil(intermediateCert);

__auto_type * operationalKeys = [[MTRTestKeys alloc] init];
XCTAssertNotNil(operationalKeys);
__auto_type * operationalPublicKey = [operationalKeys copyPublicKey];
XCTAssert(operationalPublicKey != NULL);
CFAutorelease(operationalPublicKey);

__auto_type * operationalCert = [MTRCertificates createOperationalCertificate:intermediateKeys
signingCertificate:intermediateCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:operationalPublicKey
fabricID:@1
nodeID:@1
caseAuthenticatedTags:nil
Expand All @@ -394,6 +419,9 @@ - (void)testGenerateOperationalCertErrorCases

__auto_type * operationalKeys = [[MTRTestKeys alloc] init];
XCTAssertNotNil(operationalKeys);
__auto_type * operationalPublicKey = [operationalKeys copyPublicKey];
XCTAssert(operationalPublicKey != NULL);
CFAutorelease(operationalPublicKey);

__auto_type * longCats = [[NSMutableSet alloc] initWithCapacity:4];
[longCats addObject:@0x00010001];
Expand All @@ -415,7 +443,7 @@ - (void)testGenerateOperationalCertErrorCases
// Check basic case works
__auto_type * operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:operationalPublicKey
fabricID:@1
nodeID:@1
caseAuthenticatedTags:nil
Expand All @@ -425,7 +453,7 @@ - (void)testGenerateOperationalCertErrorCases
// CATs too long
operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:operationalPublicKey
fabricID:@1
nodeID:@1
caseAuthenticatedTags:longCats
Expand All @@ -435,7 +463,7 @@ - (void)testGenerateOperationalCertErrorCases
// Multiple CATs with the same identifier but different versions
operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:operationalPublicKey
fabricID:@1
nodeID:@1
caseAuthenticatedTags:catsWithSameIdentifier
Expand All @@ -445,7 +473,7 @@ - (void)testGenerateOperationalCertErrorCases
// CAT with invalid version
operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:operationalPublicKey
fabricID:@1
nodeID:@1
caseAuthenticatedTags:catsWithInvalidVersion
Expand All @@ -455,7 +483,7 @@ - (void)testGenerateOperationalCertErrorCases
// Signing key mismatch
operationalCert = [MTRCertificates createOperationalCertificate:operationalKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:operationalPublicKey
fabricID:@1
nodeID:@1
caseAuthenticatedTags:nil
Expand All @@ -465,7 +493,7 @@ - (void)testGenerateOperationalCertErrorCases
// Invalid fabric id
operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:operationalPublicKey
fabricID:@0
nodeID:@1
caseAuthenticatedTags:nil
Expand All @@ -475,7 +503,7 @@ - (void)testGenerateOperationalCertErrorCases
// Undefined node id
operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:operationalPublicKey
fabricID:@1
nodeID:@0
caseAuthenticatedTags:nil
Expand All @@ -485,7 +513,7 @@ - (void)testGenerateOperationalCertErrorCases
// Non-operational node id
operationalCert = [MTRCertificates createOperationalCertificate:rootKeys
signingCertificate:rootCert
operationalPublicKey:operationalKeys.publicKey
operationalPublicKey:operationalPublicKey
fabricID:@1
nodeID:@(0xFFFFFFFFFFFFFFFFLLU)
caseAuthenticatedTags:nil
Expand Down
5 changes: 4 additions & 1 deletion src/darwin/Framework/CHIPTests/MTRCertificateValidityTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,10 +259,13 @@ - (void)initStack:(MTRTestCertificateIssuer *)certificateIssuer

__auto_type * controllerOperationalKeys = [[MTRTestKeys alloc] init];
XCTAssertNotNil(controllerOperationalKeys);
__auto_type * controllerPublicKey = controllerOperationalKeys.copyPublicKey;
XCTAssert(controllerPublicKey != NULL);
CFAutorelease(controllerPublicKey);

__auto_type * controllerOperationalCert =
[certificateIssuer issueOperationalCertificateForNode:@(kControllerId)
operationalPublicKey:controllerOperationalKeys.publicKey];
operationalPublicKey:controllerPublicKey];
XCTAssertNotNil(controllerOperationalCert);

__auto_type * params = [[MTRDeviceControllerStartupParams alloc] initWithIPK:certificateIssuer.rootKey.ipk
Expand Down
Loading

0 comments on commit 08024d2

Please sign in to comment.