From 6af4d1d409509e873def7b8cba12e84e98e5314b Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 13 Jan 2023 19:16:14 -0500 Subject: [PATCH 1/2] Make AutoCommissioner::SetCommissioningParameters safer. Right now, we copy all members of the incoming CommissioningParameters (which might include pointers to buffers that we don't own), then copy some of those external buffers into our own buffers. Then we also hand-copy some scalar values we have already copied. Changes in this PR: * Stop copying scalar values that operator= already handled. * Clear out all buffer references from our copy of CommissioningParameters before we start copying things into our buffers, so we don't end up with dangling pointers. * Add the missing early return when an incoming country code value is too long (used to end up with a dangling pointer). --- src/controller/AutoCommissioner.cpp | 39 ++++---------------------- src/controller/CommissioningDelegate.h | 20 +++++++++++++ 2 files changed, 26 insertions(+), 33 deletions(-) diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 982f62765bace6..bee53857eced38 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -48,23 +48,13 @@ void AutoCommissioner::SetOperationalCredentialsDelegate(OperationalCredentialsD CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParameters & params) { mParams = params; - if (params.GetFailsafeTimerSeconds().HasValue()) - { - ChipLogProgress(Controller, "Setting failsafe timer from parameters"); - mParams.SetFailsafeTimerSeconds(params.GetFailsafeTimerSeconds().Value()); - } - if (params.GetCASEFailsafeTimerSeconds().HasValue()) - { - ChipLogProgress(Controller, "Setting CASE failsafe timer from parameters"); - mParams.SetCASEFailsafeTimerSeconds(params.GetCASEFailsafeTimerSeconds().Value()); - } + // Make sure any members that point to buffers that we are not pointing to + // our own buffers are not going to dangle. + mParams.ClearExternalBufferDependentValues(); - if (params.GetAdminSubject().HasValue()) - { - ChipLogProgress(Controller, "Setting adminSubject from parameters"); - mParams.SetAdminSubject(params.GetAdminSubject().Value()); - } + // For members of params that point to some sort of buffer, we have to copy + // the data over into our own buffers. if (params.GetThreadOperationalDataset().HasValue()) { @@ -79,12 +69,6 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam mParams.SetThreadOperationalDataset(ByteSpan(mThreadOperationalDataset, dataset.size())); } - if (params.GetAttemptThreadNetworkScan().HasValue()) - { - ChipLogProgress(Controller, "Setting attempt thread scan from parameters"); - mParams.SetAttemptThreadNetworkScan(params.GetAttemptThreadNetworkScan().Value()); - } - if (params.GetWiFiCredentials().HasValue()) { WiFiCredentials creds = params.GetWiFiCredentials().Value(); @@ -101,12 +85,6 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam WiFiCredentials(ByteSpan(mSsid, creds.ssid.size()), ByteSpan(mCredentials, creds.credentials.size()))); } - if (params.GetAttemptWiFiNetworkScan().HasValue()) - { - ChipLogProgress(Controller, "Setting attempt wifi scan from parameters"); - mParams.SetAttemptWiFiNetworkScan(params.GetAttemptWiFiNetworkScan().Value()); - } - if (params.GetCountryCode().HasValue()) { auto code = params.GetCountryCode().Value(); @@ -118,6 +96,7 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam else { ChipLogError(Controller, "Country code is too large: %u", static_cast(code.size())); + return CHIP_ERROR_INVALID_ARGUMENT; } } @@ -148,12 +127,6 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam } mParams.SetCSRNonce(ByteSpan(mCSRNonce, sizeof(mCSRNonce))); - if (params.GetSkipCommissioningComplete().HasValue()) - { - ChipLogProgress(Controller, "Setting PASE-only commissioning from parameters"); - mParams.SetSkipCommissioningComplete(params.GetSkipCommissioningComplete().Value()); - } - return CHIP_NO_ERROR; } diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 868f27f2f39f94..99f55b50e104a4 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -425,6 +425,26 @@ class CommissioningParameters return *this; } + // Clear all members that depend on some sort of external buffer. Can be + // used to make sure that we are not holding any dangling pointers. + void ClearExternalBufferDependentValues() + { + mCSRNonce.ClearValue(); + mAttestationNonce.ClearValue(); + mWiFiCreds.ClearValue(); + mCountryCode.ClearValue(); + mThreadOperationalDataset.ClearValue(); + mNOCChainGenerationParameters.ClearValue(); + mRootCert.ClearValue(); + mNoc.ClearValue(); + mIcac.ClearValue(); + mIpk.ClearValue(); + mAttestationElements.ClearValue(); + mAttestationSignature.ClearValue(); + mPAI.ClearValue(); + mDAC.ClearValue(); + } + private: // Items that can be set by the commissioner Optional mFailsafeTimerSeconds; From ade65af37d80f4d343dd23c9057cb09871e92f07 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 18 Jan 2023 12:28:21 -0500 Subject: [PATCH 2/2] Address review comment. --- src/controller/AutoCommissioner.cpp | 54 +++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 3 deletions(-) diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index bee53857eced38..a1bc3b1d6b72c4 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -45,13 +45,55 @@ void AutoCommissioner::SetOperationalCredentialsDelegate(OperationalCredentialsD mOperationalCredentialsDelegate = operationalCredentialsDelegate; } +// Returns true if maybeUnsafeSpan is pointing to a buffer that we're not sure +// will live for long enough. knownSafeSpan, if it has a value, points to a +// buffer that we _are_ sure will live for long enough. +template +static bool IsUnsafeSpan(const Optional & maybeUnsafeSpan, const Optional & knownSafeSpan) +{ + if (!maybeUnsafeSpan.HasValue()) + { + return false; + } + + if (!knownSafeSpan.HasValue()) + { + return true; + } + + return maybeUnsafeSpan.Value().data() != knownSafeSpan.Value().data(); +} + CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParameters & params) { + // Make sure any members that point to buffers that we are not pointing to + // our own buffers are not going to dangle. We can skip this step if all + // the buffers pointers that we don't plan to re-point to our own buffers + // below are already pointing to the same things as our own buffer pointers + // (so that we know they have to be safe somehow). + // + // The checks are a bit painful, because Span does not have a usable + // operator==, and in any case, we want to compare for pointer equality, not + // data equality. + bool haveMaybeDanglingBufferPointers = + ((params.GetNOCChainGenerationParameters().HasValue() && + (!mParams.GetNOCChainGenerationParameters().HasValue() || + params.GetNOCChainGenerationParameters().Value().nocsrElements.data() != + mParams.GetNOCChainGenerationParameters().Value().nocsrElements.data() || + params.GetNOCChainGenerationParameters().Value().signature.data() != + mParams.GetNOCChainGenerationParameters().Value().signature.data())) || + IsUnsafeSpan(params.GetRootCert(), mParams.GetRootCert()) || IsUnsafeSpan(params.GetNoc(), mParams.GetNoc()) || + IsUnsafeSpan(params.GetIcac(), mParams.GetIcac()) || IsUnsafeSpan(params.GetIpk(), mParams.GetIpk()) || + IsUnsafeSpan(params.GetAttestationElements(), mParams.GetAttestationElements()) || + IsUnsafeSpan(params.GetAttestationSignature(), mParams.GetAttestationSignature()) || + IsUnsafeSpan(params.GetPAI(), mParams.GetPAI()) || IsUnsafeSpan(params.GetDAC(), mParams.GetDAC())); + mParams = params; - // Make sure any members that point to buffers that we are not pointing to - // our own buffers are not going to dangle. - mParams.ClearExternalBufferDependentValues(); + if (haveMaybeDanglingBufferPointers) + { + mParams.ClearExternalBufferDependentValues(); + } // For members of params that point to some sort of buffer, we have to copy // the data over into our own buffers. @@ -62,6 +104,8 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam if (dataset.size() > CommissioningParameters::kMaxThreadDatasetLen) { ChipLogError(Controller, "Thread operational data set is too large"); + // Make sure our buffer pointers don't dangle. + mParams.ClearExternalBufferDependentValues(); return CHIP_ERROR_INVALID_ARGUMENT; } memcpy(mThreadOperationalDataset, dataset.data(), dataset.size()); @@ -76,6 +120,8 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam creds.credentials.size() > CommissioningParameters::kMaxCredentialsLen) { ChipLogError(Controller, "Wifi credentials are too large"); + // Make sure our buffer pointers don't dangle. + mParams.ClearExternalBufferDependentValues(); return CHIP_ERROR_INVALID_ARGUMENT; } memcpy(mSsid, creds.ssid.data(), creds.ssid.size()); @@ -96,6 +142,8 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam else { ChipLogError(Controller, "Country code is too large: %u", static_cast(code.size())); + // Make sure our buffer pointers don't dangle. + mParams.ClearExternalBufferDependentValues(); return CHIP_ERROR_INVALID_ARGUMENT; } }