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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
+190 −43
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
57423cd
Initial checkin
woody-apple Oct 30, 2024
cb67d1e
Fixing review feedback
woody-apple Oct 30, 2024
cbefb5a
Adding braces
woody-apple Oct 30, 2024
456ac90
Restyled by clang-format
restyled-commits Oct 30, 2024
bc33c7b
Fixing build
woody-apple Oct 30, 2024
84ffc4e
Restyled by clang-format
restyled-commits Oct 30, 2024
6a50d9d
Fixing annotations
woody-apple Oct 30, 2024
a23b86e
Update src/darwin/Framework/CHIP/MTRKeypair.h
woody-apple Oct 30, 2024
f66949f
separate statements involving _mPublicKey to make compiler happy
kiel-apple Oct 31, 2024
de11a1e
use `CFAutorelease` on CoreFoundation typed public key copies
kiel-apple Oct 31, 2024
79aaaca
fix indent
kiel-apple Oct 31, 2024
2bf82f0
implement `copyPublicKey` for `MTRTestKeys`; add TODO note about opti…
kiel-apple Oct 31, 2024
c134be6
remove comment
kiel-apple Nov 1, 2024
8a17443
consistent formatting for `copyPublicKey` calls
kiel-apple Nov 1, 2024
8691282
reformat `copyPublicKey` in `MTRTestKeys`
kiel-apple Nov 1, 2024
47e264a
Update src/darwin/Framework/CHIP/MTRKeypair.h
woody-apple Nov 1, 2024
8dbbcaa
re-add `publicKey` in `CHIPToolKeypair`
kiel-apple Nov 2, 2024
d328eaf
use `copyPublicKey` in more places
kiel-apple Nov 3, 2024
82379eb
dedupe public key cache population for CHIPToolKeypair
kiel-apple Nov 4, 2024
fbbd06a
Restyled by clang-format
restyled-commits Nov 4, 2024
fdbd53c
try manually releasing pubkey in test
kiel-apple Nov 5, 2024
33538e4
leakfix: MTRP256KeypairBridge
kiel-apple Nov 5, 2024
1cdc2f4
leakfix: `MTRCertificateTests.testGenerateCSR`
kiel-apple Nov 5, 2024
a761b1c
remove unnecessary plain `publicKey` implementation
kiel-apple Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/darwin-framework-tool/commands/common/CHIPToolKeypair.h
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
12 changes: 11 additions & 1 deletion src/darwin/Framework/CHIP/MTRP256KeypairBridge.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
69 changes: 52 additions & 17 deletions src/darwin/Framework/CHIPTests/MTRCertificateTests.m
View file
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not copy autorelease this? Makes for much easier code...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(discussing)

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 All @@ -504,7 +532,14 @@ - (void)testGenerateCSR
__auto_type * publicKey = [MTRCertificates publicKeyFromCSR:csr error:nil];
XCTAssertNotNil(publicKey);

SecKeyRef originalKeyRef = [testKeys publicKey];
SecKeyRef originalKeyRef;
if ([testKeys respondsToSelector:@selector(copyPublicKey)]) {
originalKeyRef = [testKeys copyPublicKey];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also looks like a leak.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(added CFAutorelease(originalKeyRef); below - it has been a while, scrutiny welcome)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(specifically: is manual creation of NSAutoreleasePool or use of @autoreleasepool merited here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to nil check CFAutorelease, please just use [object autorelease] per our chat

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Otherwise you have a crash here potentially)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(discussing)

CFAutorelease(originalKeyRef);
} else {
originalKeyRef = [testKeys publicKey];
}

XCTAssertTrue(originalKeyRef != NULL);

NSData * originalPublicKey = (__bridge_transfer NSData *) SecKeyCopyExternalRepresentation(originalKeyRef, nil);
Expand Down
Loading
Loading