diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 4be7e8ce01c005..1f0c930383d90a 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -45,33 +45,67 @@ void AutoCommissioner::SetOperationalCredentialsDelegate(OperationalCredentialsD mOperationalCredentialsDelegate = operationalCredentialsDelegate; } -CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParameters & params) +// 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) { - mParams = params; - if (params.GetFailsafeTimerSeconds().HasValue()) + if (!maybeUnsafeSpan.HasValue()) { - ChipLogProgress(Controller, "Setting failsafe timer from parameters"); - mParams.SetFailsafeTimerSeconds(params.GetFailsafeTimerSeconds().Value()); + return false; } - if (params.GetCASEFailsafeTimerSeconds().HasValue()) + if (!knownSafeSpan.HasValue()) { - ChipLogProgress(Controller, "Setting CASE failsafe timer from parameters"); - mParams.SetCASEFailsafeTimerSeconds(params.GetCASEFailsafeTimerSeconds().Value()); + return true; } - if (params.GetAdminSubject().HasValue()) + 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; + + if (haveMaybeDanglingBufferPointers) { - ChipLogProgress(Controller, "Setting adminSubject from parameters"); - mParams.SetAdminSubject(params.GetAdminSubject().Value()); + mParams.ClearExternalBufferDependentValues(); } + // 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()) { ByteSpan dataset = params.GetThreadOperationalDataset().Value(); 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()); @@ -79,12 +113,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(); @@ -92,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()); @@ -101,12 +131,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 +142,9 @@ 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; } } @@ -148,12 +175,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 75344ca5eb6e12..ccc8954cb842a2 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;