Skip to content

Commit 470eea7

Browse files
committed
fix(rest): fix getNetworkDiagnosticTask off-by-one crash
DiagnosticTypes::kKeyMap has 27 entries (ids 0-9, 14-17, 19-21, 23-31, 34), but DiagnosticTypes::kMaxTotalCount was set to 26. This caused an assertion failure and crashed the otbr-agent root daemon whenever a REST call requested all 27 valid types. In NDEBUG builds, this led to a 1-byte out-of-bounds write. This commit addresses the issue by: 1. Increasing kMaxTotalCount to 27. 2. Implementing compile-time static_assert checks using recursive constexpr helper functions to dynamically validate kMaxTotalCount, kMaxQueryCount, and kMaxResettableCount against the key map table, preventing future off-by-one regressions. 3. Validating request types array size in NetworkDiagnostic::Validate to ensure the unique type count does not exceed kMaxTotalCount.
1 parent 07ec9a0 commit 470eea7

2 files changed

Lines changed: 43 additions & 4 deletions

File tree

src/rest/actions/network_diagnostic.cpp

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -206,10 +206,15 @@ bool NetworkDiagnostic::Validate(const cJSON &aJson)
206206
errormsg = KEY_TYPES;
207207
VerifyOrExit(types != nullptr);
208208
VerifyOrExit(cJSON_IsArray(types));
209-
cJSON_ArrayForEach(item, types)
210209
{
211-
VerifyOrExit(cJSON_IsString(item));
212-
SuccessOrExit(DiagnosticTypes::FindId(item->valuestring, id));
210+
std::set<uint8_t> uniqueTypes;
211+
cJSON_ArrayForEach(item, types)
212+
{
213+
VerifyOrExit(cJSON_IsString(item));
214+
SuccessOrExit(DiagnosticTypes::FindId(item->valuestring, id));
215+
uniqueTypes.insert(id);
216+
}
217+
VerifyOrExit(uniqueTypes.size() <= DiagnosticTypes::kMaxTotalCount);
213218
}
214219

215220
ok = true;

src/rest/diagnostic_types.hpp

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,37 @@ namespace rest {
4141
class DiagnosticTypes
4242
{
4343
public:
44-
static constexpr uint32_t kMaxTotalCount = 26; ///< Total number of recognized types
44+
static constexpr uint32_t kMaxTotalCount = 27; ///< Total number of recognized types
4545
static constexpr uint32_t kMaxQueryCount = 3; ///< Number of types that require a diag query
4646
static constexpr uint32_t kMaxResettableCount = 2; ///< Number of types that can be reset
4747

48+
static constexpr uint32_t CountTotalTypes(uint32_t aIndex)
49+
{
50+
return (aIndex >= kTypeListSize)
51+
? 0
52+
: ((kTypeInfos[aIndex].mJsonKey != nullptr) ? 1 : 0) + CountTotalTypes(aIndex + 1);
53+
}
54+
55+
static constexpr uint32_t CountQueryTypes(uint32_t aIndex)
56+
{
57+
return (aIndex >= kTypeListSize)
58+
? 0
59+
: (((kTypeInfos[aIndex].mJsonKey != nullptr) && (kTypeInfos[aIndex].mProperties & kPropertyQuery))
60+
? 1
61+
: 0) +
62+
CountQueryTypes(aIndex + 1);
63+
}
64+
65+
static constexpr uint32_t CountResettableTypes(uint32_t aIndex)
66+
{
67+
return (aIndex >= kTypeListSize)
68+
? 0
69+
: (((kTypeInfos[aIndex].mJsonKey != nullptr) && (kTypeInfos[aIndex].mProperties & kPropertyCanReset))
70+
? 1
71+
: 0) +
72+
CountResettableTypes(aIndex + 1);
73+
}
74+
4875
static const char *GetJsonKey(uint8_t aTypeId);
4976
static bool RequiresQuery(uint8_t aTypeId);
5077
static bool CanReset(uint8_t aTypeId);
@@ -109,6 +136,13 @@ class DiagnosticTypes
109136
static const std::unordered_map<std::string, uint8_t> kKeyMap;
110137
};
111138

139+
static_assert(DiagnosticTypes::kMaxTotalCount == DiagnosticTypes::CountTotalTypes(0),
140+
"kMaxTotalCount must match the number of recognized types");
141+
static_assert(DiagnosticTypes::kMaxQueryCount == DiagnosticTypes::CountQueryTypes(0),
142+
"kMaxQueryCount must match the number of query types");
143+
static_assert(DiagnosticTypes::kMaxResettableCount == DiagnosticTypes::CountResettableTypes(0),
144+
"kMaxResettableCount must match the number of resettable types");
145+
112146
} // namespace rest
113147
} // namespace otbr
114148

0 commit comments

Comments
 (0)