From 43b193dce2542c13c3075cef97f9718a58d6a22d Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 7 Apr 2022 21:31:00 -0400 Subject: [PATCH] Add ClearSecretData calls for IPK bits on Darwin. (#17179) Keypairs do this automatically, but we're using a raw buffer for the IPK. --- src/crypto/CHIPCryptoPAL.h | 9 ++++ .../CHIP/CHIPOperationalCredentialsDelegate.h | 4 +- .../CHIPOperationalCredentialsDelegate.mm | 42 ++++++++++++++----- src/protocols/secure_channel/CASESession.cpp | 2 +- 4 files changed, 43 insertions(+), 14 deletions(-) diff --git a/src/crypto/CHIPCryptoPAL.h b/src/crypto/CHIPCryptoPAL.h index 6ff6e6b5606cfe..d7942116619b94 100644 --- a/src/crypto/CHIPCryptoPAL.h +++ b/src/crypto/CHIPCryptoPAL.h @@ -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 +void ClearSecretData(uint8_t (&buf)[N]) +{ + ClearSecretData(buf, N); +} + template class ECPKey { diff --git a/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.h b/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.h index ae72033b6704d5..a5156b28d5cad5 100644 --- a/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.h +++ b/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.h @@ -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(); @@ -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"; diff --git a/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.mm b/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.mm index 7f8caa95b3f958..a6430f910a571c 100644 --- a/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.mm +++ b/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.mm @@ -46,6 +46,11 @@ static BOOL isRunningTests(void) return (environment[@"XCTestConfigurationFilePath"] != nil); } +static void ClearSecretData(NSMutableData * data) +{ + Crypto::ClearSecretData(static_cast([data mutableBytes]), [data length]); +} + CHIP_ERROR CHIPOperationalCredentialsDelegate::init( CHIPPersistentStorageDelegateBridge * storage, ChipP256KeypairPtr nocSigner, NSData * _Nullable ipk) { @@ -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(); @@ -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) { @@ -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); } @@ -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) { @@ -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; } @@ -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()) { @@ -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()) { diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 72dd67f8e46483..3e518b80ef232a 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -108,7 +108,7 @@ void CASESession::Clear() PairingSession::Clear(); mState = kInitialized; - Crypto::ClearSecretData(&mIPK[0], sizeof(mIPK)); + Crypto::ClearSecretData(mIPK); AbortExchange(); }