Enforce length constraint for CountryCode in SetRegulatoryConfig. (#2…

…7949) We were not checking the length (which must be 2), so would allow 1-char or 0-char values. Also aligns the exact logic with the Location attribute write code and adds some error logging.
project-chip · Jan 24, 2024 · 1955106 · 1955106
1 parent 10771c4
commit 1955106
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 4 deletions.
diff --git a/src/app/clusters/basic-information/basic-information.cpp b/src/app/clusters/basic-information/basic-information.cpp
@@ -344,7 +344,11 @@ CHIP_ERROR BasicAttrAccess::WriteLocation(AttributeValueDecoder & aDecoder)
     ReturnErrorOnFailure(aDecoder.Decode(location));
 
     bool isValidLength = location.size() == DeviceLayer::ConfigurationManager::kMaxLocationLength;
-    VerifyOrReturnError(isValidLength, StatusIB(Protocols::InteractionModel::Status::InvalidValue).ToChipError());
+    if (!isValidLength)
+    {
+        ChipLogError(Zcl, "Invalid country code: '%.*s'", static_cast<int>(location.size()), location.data());
+        return CHIP_IM_GLOBAL_STATUS(ConstraintError);
+    }
 
     return DeviceLayer::ConfigurationMgr().StoreCountryCode(location.data(), location.size());
 }

diff --git a/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp b/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp
@@ -287,10 +287,21 @@ bool emberAfGeneralCommissioningClusterSetRegulatoryConfigCallback(app::CommandH
     DeviceControlServer * server = &DeviceLayer::DeviceControlServer::DeviceControlSvr();
     Commands::SetRegulatoryConfigResponse::Type response;
 
+    auto & countryCode = commandData.countryCode;
+    bool isValidLength = countryCode.size() == DeviceLayer::ConfigurationManager::kMaxLocationLength;
+    if (!isValidLength)
+    {
+        ChipLogError(Zcl, "Invalid country code: '%.*s'", static_cast<int>(countryCode.size()), countryCode.data());
+        commandObj->AddStatus(commandPath, Protocols::InteractionModel::Status::ConstraintError);
+        return true;
+    }
+
     if (commandData.newRegulatoryConfig > RegulatoryLocationTypeEnum::kIndoorOutdoor)
     {
         response.errorCode = CommissioningErrorEnum::kValueOutsideRange;
-        response.debugText = commandData.countryCode;
+        // TODO: How does using the country code in debug text make sense, if
+        // the real issue is the newRegulatoryConfig value?
+        response.debugText = countryCode;
     }
     else
     {
@@ -304,11 +315,13 @@ bool emberAfGeneralCommissioningClusterSetRegulatoryConfigCallback(app::CommandH
         if ((locationCapability != to_underlying(RegulatoryLocationTypeEnum::kIndoorOutdoor)) && (location != locationCapability))
         {
             response.errorCode = CommissioningErrorEnum::kValueOutsideRange;
-            response.debugText = commandData.countryCode;
+            // TODO: How does using the country code in debug text make sense, if
+            // the real issue is the newRegulatoryConfig value?
+            response.debugText = countryCode;
         }
         else
         {
-            CheckSuccess(server->SetRegulatoryConfig(location, commandData.countryCode), Failure);
+            CheckSuccess(server->SetRegulatoryConfig(location, countryCode), Failure);
             Breadcrumb::Set(commandPath.mEndpointId, commandData.breadcrumb);
             response.errorCode = CommissioningErrorEnum::kOk;
         }