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

Make AutoCommissioner::SetCommissioningParameters safer. #24422

Merged
merged 2 commits into from
Jan 20, 2023
Merged

Make AutoCommissioner::SetCommissioningParameters safer. #24422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6af4d1d
Make AutoCommissioner::SetCommissioningParameters safer.
bzbarsky-apple Jan 14, 2023
ade65af
Address review comment.
bzbarsky-apple Jan 18, 2023
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
79 changes: 50 additions & 29 deletions src/controller/AutoCommissioner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,53 +45,83 @@ 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 <typename SpanType>
static bool IsUnsafeSpan(const Optional<SpanType> & maybeUnsafeSpan, const Optional<SpanType> & 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());
ChipLogProgress(Controller, "Setting thread operational dataset from parameters");
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();
if (creds.ssid.size() > CommissioningParameters::kMaxSsidLen ||
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());
Expand All @@ -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();
Expand All @@ -118,6 +142,9 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam
else
{
ChipLogError(Controller, "Country code is too large: %u", static_cast<unsigned>(code.size()));
// Make sure our buffer pointers don't dangle.
mParams.ClearExternalBufferDependentValues();
return CHIP_ERROR_INVALID_ARGUMENT;
}
}

Expand Down Expand Up @@ -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;
}

Expand Down
20 changes: 20 additions & 0 deletions src/controller/CommissioningDelegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<uint16_t> mFailsafeTimerSeconds;
Expand Down