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

Add ClearSecretData calls for IPK bits on Darwin. #17179

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions src/crypto/CHIPCryptoPAL.h
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,15 @@ enum class SupportedECPKeyTypes : uint8_t
**/
void ClearSecretData(uint8_t * buf, size_t len);

/**
* Helper for clearing a C array which auto-deduces the size.
*/
template <size_t N>
void ClearSecretData(uint8_t (&buf)[N])
{
ClearSecretData(buf, N);
}

template <typename Sig>
class ECPKey
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class CHIPOperationalCredentialsDelegate : public chip::Controller::OperationalC
const chip::Crypto::P256PublicKey & pubkey, chip::MutableByteSpan & rcac, chip::MutableByteSpan & icac,
chip::MutableByteSpan & noc);

const chip::Crypto::AesCcm128KeySpan GetIPK() { return chip::Crypto::AesCcm128KeySpan(mIPK); }
const chip::Crypto::AesCcm128KeySpan GetIPK() { return mIPK.Span(); }

private:
CHIP_ERROR GenerateRootCertKeys();
Expand All @@ -83,7 +83,7 @@ class CHIPOperationalCredentialsDelegate : public chip::Controller::OperationalC
ChipP256KeypairPtr mIssuerKey;
uint64_t mIssuerId = 1234;

uint8_t mIPK[chip::Crypto::CHIP_CRYPTO_SYMMETRIC_KEY_LENGTH_BYTES];
chip::Crypto::AesCcm128Key mIPK;

const uint32_t kCertificateValiditySecs = 365 * 24 * 60 * 60;
const NSString * kCHIPCAKeyChainLabel = @"matter.nodeopcerts.CA:0";
Expand Down
42 changes: 31 additions & 11 deletions src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.mm
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ static BOOL isRunningTests(void)
return (environment[@"XCTestConfigurationFilePath"] != nil);
}

static void ClearSecretData(NSMutableData * data)
{
Crypto::ClearSecretData(static_cast<uint8_t *>([data mutableBytes]), [data length]);
}

CHIP_ERROR CHIPOperationalCredentialsDelegate::init(
CHIPPersistentStorageDelegateBridge * storage, ChipP256KeypairPtr nocSigner, NSData * _Nullable ipk)
{
Expand Down Expand Up @@ -80,11 +85,11 @@ static BOOL isRunningTests(void)
}

if (ipk) {
if ([ipk length] != sizeof(mIPK)) {
if ([ipk length] != mIPK.Length()) {
CHIP_LOG_ERROR("CHIPOperationalCredentialsDelegate::init provided IPK is wrong size");
return CHIP_ERROR_INVALID_ARGUMENT;
}
memcpy(mIPK, [ipk bytes], [ipk length]);
memcpy(mIPK.Bytes(), [ipk bytes], [ipk length]);
} else {
CHIP_ERROR err = LoadIPKFromKeyChain();

Expand Down Expand Up @@ -141,6 +146,8 @@ static BOOL isRunningTests(void)
(id) kSecReturnData : @YES,
};

// The CFDataRef we get from SecItemCopyMatching allocates its
// buffer in a way that zeroes it when deallocated.
CFDataRef keyDataRef;
OSStatus status = SecItemCopyMatching((CFDictionaryRef) query, (CFTypeRef *) &keyDataRef);
if (status == errSecItemNotFound || keyDataRef == nil) {
Expand All @@ -151,17 +158,20 @@ static BOOL isRunningTests(void)
CHIP_LOG_ERROR("Found an existing self managed keypair in the keychain");
NSData * keyData = CFBridgingRelease(keyDataRef);

NSData * keypairData = [[NSData alloc] initWithBase64EncodedData:keyData options:0];
NSMutableData * keypairData = [[NSMutableData alloc] initWithBase64EncodedData:keyData options:0];

chip::Crypto::P256SerializedKeypair serialized;
if ([keypairData length] != serialized.Capacity()) {
NSLog(@"Keypair length %zu does not match expected length %zu", [keypairData length], serialized.Capacity());
ClearSecretData(keypairData);
return CHIP_ERROR_INTERNAL;
}

std::memmove((uint8_t *) serialized, [keypairData bytes], [keypairData length]);
serialized.SetLength([keypairData length]);

ClearSecretData(keypairData);

CHIP_LOG_ERROR("Deserializing the key");
return mIssuerKey->Deserialize(serialized);
}
Expand All @@ -175,6 +185,8 @@ static BOOL isRunningTests(void)
(id) kSecReturnData : @YES,
};

// The CFDataRef we get from SecItemCopyMatching allocates its
// buffer in a way that zeroes it when deallocated.
CFDataRef keyDataRef;
OSStatus status = SecItemCopyMatching((CFDictionaryRef) query, (CFTypeRef *) &keyDataRef);
if (status == errSecItemNotFound || keyDataRef == nil) {
Expand All @@ -185,13 +197,15 @@ static BOOL isRunningTests(void)
CHIP_LOG_ERROR("Found an existing IPK in the keychain");
NSData * keyData = CFBridgingRelease(keyDataRef);

NSData * ipkData = [[NSData alloc] initWithBase64EncodedData:keyData options:0];
if ([ipkData length] != sizeof(mIPK)) {
NSLog(@"IPK length %zu does not match expected length %zu", [ipkData length], sizeof(mIPK));
NSMutableData * ipkData = [[NSMutableData alloc] initWithBase64EncodedData:keyData options:0];
if ([ipkData length] != mIPK.Length()) {
NSLog(@"IPK length %zu does not match expected length %zu", [ipkData length], mIPK.Length());
ClearSecretData(ipkData);
return CHIP_ERROR_INTERNAL;
}

memcpy(mIPK, [ipkData bytes], [ipkData length]);
memcpy(mIPK.Bytes(), [ipkData bytes], [ipkData length]);
ClearSecretData(ipkData);
return CHIP_NO_ERROR;
}

Expand All @@ -209,15 +223,18 @@ static BOOL isRunningTests(void)
return errorCode;
}

NSData * keypairData = [NSData dataWithBytes:serializedKey.Bytes() length:serializedKey.Length()];
NSMutableData * keypairData = [NSMutableData dataWithBytes:serializedKey.Bytes() length:serializedKey.Length()];

const NSDictionary * addParams = @{
(id) kSecClass : (id) kSecClassGenericPassword,
(id) kSecAttrService : kCHIPCAKeyChainLabel,
(id) kSecAttrSynchronizable : @YES,
// TODO: Figure out how to ClearSecretData on the base-64 encoded data?
(id) kSecValueData : [keypairData base64EncodedDataWithOptions:0],
};

ClearSecretData(keypairData);

OSStatus status = SecItemAdd((__bridge CFDictionaryRef) addParams, nullptr);
// TODO: Enable SecItemAdd for Darwin unit tests
if (status != errSecSuccess && !isRunningTests()) {
Expand All @@ -232,20 +249,23 @@ static BOOL isRunningTests(void)

CHIP_ERROR CHIPOperationalCredentialsDelegate::GenerateIPK()
{
CHIP_ERROR errorCode = DRBG_get_bytes(mIPK, sizeof(mIPK));
CHIP_ERROR errorCode = DRBG_get_bytes(mIPK.Bytes(), mIPK.Length());
if (errorCode != CHIP_NO_ERROR) {
return errorCode;
}

NSData * ipkAdata = [NSData dataWithBytes:mIPK length:sizeof(mIPK)];
NSMutableData * ipkData = [NSMutableData dataWithBytes:mIPK.Bytes() length:mIPK.Length()];

const NSDictionary * addParams = @{
(id) kSecClass : (id) kSecClassGenericPassword,
(id) kSecAttrService : kCHIPIPKKeyChainLabel,
(id) kSecAttrSynchronizable : @YES,
(id) kSecValueData : [ipkAdata base64EncodedDataWithOptions:0],
// TODO: Figure out how to ClearSecretData on the base-64 encoded data?
(id) kSecValueData : [ipkData base64EncodedDataWithOptions:0],
};

ClearSecretData(ipkData);

OSStatus status = SecItemAdd((__bridge CFDictionaryRef) addParams, nullptr);
// TODO: Enable SecItemAdd for Darwin unit tests
if (status != errSecSuccess && !isRunningTests()) {
Expand Down
2 changes: 1 addition & 1 deletion src/protocols/secure_channel/CASESession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ void CASESession::Clear()
PairingSession::Clear();

mState = kInitialized;
Crypto::ClearSecretData(&mIPK[0], sizeof(mIPK));
Crypto::ClearSecretData(mIPK);
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved

AbortExchange();
}
Expand Down