Skip to content

Commit 5afc5c9

Browse files
skargskarg
authored
fix: address out-of-bounds read in lighting_command_decode and add bounds checking tests (bacnet-stack#1412)
* fix: address out-of-bounds read in lighting_command_decode and add bounds checking tests * fix: FADE_TO and RAMP_TO required levels in lighting_command_decode
1 parent 1945f83 commit 5afc5c9

2 files changed

Lines changed: 157 additions & 83 deletions

File tree

‎src/bacnet/lighting.c‎

Lines changed: 92 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -192,129 +192,139 @@ int lighting_command_decode(
192192
case BACNET_LIGHTS_NONE:
193193
break;
194194
case BACNET_LIGHTS_FADE_TO:
195-
if ((apdu_size - apdu_len) == 0) {
196-
return BACNET_STATUS_REJECT;
197-
}
198-
/* target-level [1] REAL (0.0..100.0) OPTIONAL */
195+
/* target-level [1] REAL (0.0..100.0) */
199196
len = bacnet_real_context_decode(
200-
&apdu[apdu_len], apdu_size, 1, &real_value);
197+
&apdu[apdu_len], apdu_size - apdu_len, 1, &real_value);
201198
if (len > 0) {
202199
apdu_len += len;
203200
if (data) {
204201
data->target_level = real_value;
205202
data->use_target_level = true;
206203
}
204+
} else {
205+
return BACNET_STATUS_ERROR;
207206
}
208-
if ((apdu_size - apdu_len) > 0) {
209-
/* Tag 4: fade-time - OPTIONAL */
210-
len = bacnet_unsigned_context_decode(
211-
&apdu[apdu_len], apdu_size, 4, &unsigned_value);
212-
if (len > 0) {
213-
apdu_len += len;
214-
if (data) {
215-
data->fade_time = (uint32_t)unsigned_value;
216-
data->use_fade_time = true;
217-
}
218-
} else {
219-
return BACNET_STATUS_ERROR;
207+
/* Tag 4: fade-time - OPTIONAL */
208+
len = bacnet_unsigned_context_decode(
209+
&apdu[apdu_len], apdu_size - apdu_len, 4, &unsigned_value);
210+
if (len > 0) {
211+
apdu_len += len;
212+
if (data) {
213+
data->fade_time = (uint32_t)unsigned_value;
214+
data->use_fade_time = true;
220215
}
216+
} else if (len == 0) {
217+
/* fade-time is optional, so do nothing */
218+
} else {
219+
return BACNET_STATUS_ERROR;
221220
}
222-
if ((apdu_size - apdu_len) > 0) {
223-
/* priority [5] Unsigned (1..16) OPTIONAL */
224-
len = bacnet_unsigned_context_decode(
225-
&apdu[apdu_len], apdu_size, 5, &unsigned_value);
226-
if (len > 0) {
227-
apdu_len += len;
228-
if (data) {
229-
data->priority = (uint8_t)unsigned_value;
230-
data->use_priority = true;
231-
}
221+
/* priority [5] Unsigned (1..16) OPTIONAL */
222+
len = bacnet_unsigned_context_decode(
223+
&apdu[apdu_len], apdu_size - apdu_len, 5, &unsigned_value);
224+
if (len > 0) {
225+
apdu_len += len;
226+
if (data) {
227+
data->priority = (uint8_t)unsigned_value;
228+
data->use_priority = true;
232229
}
230+
} else if (len == 0) {
231+
/* priority is optional, so do nothing */
232+
} else {
233+
return BACNET_STATUS_ERROR;
233234
}
234235
break;
235236
case BACNET_LIGHTS_RAMP_TO:
236-
if ((apdu_size - apdu_len) == 0) {
237-
return BACNET_STATUS_REJECT;
238-
}
239-
/* target-level [1] REAL (0.0..100.0) OPTIONAL */
237+
/* target-level [1] REAL (0.0..100.0) */
240238
len = bacnet_real_context_decode(
241-
&apdu[apdu_len], apdu_size, 1, &real_value);
239+
&apdu[apdu_len], apdu_size - apdu_len, 1, &real_value);
242240
if (len > 0) {
243241
apdu_len += len;
244242
if (data) {
245243
data->target_level = real_value;
246244
data->use_target_level = true;
247245
}
246+
} else {
247+
return BACNET_STATUS_ERROR;
248248
}
249-
if ((apdu_size - apdu_len) > 0) {
250-
/* ramp-rate [2] REAL (0.1..100.0) OPTIONAL */
251-
len = bacnet_real_context_decode(
252-
&apdu[apdu_len], apdu_size, 2, &real_value);
253-
if (len > 0) {
254-
apdu_len += len;
255-
if (data) {
256-
data->ramp_rate = real_value;
257-
data->use_ramp_rate = true;
258-
}
249+
/* ramp-rate [2] REAL (0.1..100.0) OPTIONAL */
250+
len = bacnet_real_context_decode(
251+
&apdu[apdu_len], apdu_size - apdu_len, 2, &real_value);
252+
if (len > 0) {
253+
apdu_len += len;
254+
if (data) {
255+
data->ramp_rate = real_value;
256+
data->use_ramp_rate = true;
259257
}
258+
} else if (len == 0) {
259+
/* ramp-rate is optional, so do nothing */
260+
} else {
261+
return BACNET_STATUS_ERROR;
260262
}
261-
if ((apdu_size - apdu_len) > 0) {
262-
/* priority [5] Unsigned (1..16) OPTIONAL */
263-
len = bacnet_unsigned_context_decode(
264-
&apdu[apdu_len], apdu_size, 5, &unsigned_value);
265-
if (len > 0) {
266-
apdu_len += len;
267-
if (data) {
268-
data->priority = (uint8_t)unsigned_value;
269-
data->use_priority = true;
270-
}
263+
/* priority [5] Unsigned (1..16) OPTIONAL */
264+
len = bacnet_unsigned_context_decode(
265+
&apdu[apdu_len], apdu_size - apdu_len, 5, &unsigned_value);
266+
if (len > 0) {
267+
apdu_len += len;
268+
if (data) {
269+
data->priority = (uint8_t)unsigned_value;
270+
data->use_priority = true;
271271
}
272+
} else if (len == 0) {
273+
/* priority is optional, so do nothing */
274+
} else {
275+
return BACNET_STATUS_ERROR;
272276
}
273277
break;
274278
case BACNET_LIGHTS_STEP_UP:
275279
case BACNET_LIGHTS_STEP_DOWN:
276280
case BACNET_LIGHTS_STEP_ON:
277281
case BACNET_LIGHTS_STEP_OFF:
278-
if ((apdu_size - apdu_len) > 0) {
279-
/* step-increment [3] REAL (0.1..100.0) OPTIONAL */
280-
len = bacnet_real_context_decode(
281-
&apdu[apdu_len], apdu_size, 3, &real_value);
282-
if (len > 0) {
283-
apdu_len += len;
284-
if (data) {
285-
data->step_increment = real_value;
286-
data->use_step_increment = true;
287-
}
282+
/* step-increment [3] REAL (0.1..100.0) OPTIONAL */
283+
len = bacnet_real_context_decode(
284+
&apdu[apdu_len], apdu_size - apdu_len, 3, &real_value);
285+
if (len > 0) {
286+
apdu_len += len;
287+
if (data) {
288+
data->step_increment = real_value;
289+
data->use_step_increment = true;
288290
}
291+
} else if (len == 0) {
292+
/* step-increment is optional, so do nothing */
293+
} else {
294+
return BACNET_STATUS_ERROR;
289295
}
290-
if ((apdu_size - apdu_len) > 0) {
291-
/* priority [5] Unsigned (1..16) OPTIONAL */
292-
len = bacnet_unsigned_context_decode(
293-
&apdu[apdu_len], apdu_size, 5, &unsigned_value);
294-
if (len > 0) {
295-
apdu_len += len;
296-
if (data) {
297-
data->priority = (uint8_t)unsigned_value;
298-
data->use_priority = true;
299-
}
296+
/* priority [5] Unsigned (1..16) OPTIONAL */
297+
len = bacnet_unsigned_context_decode(
298+
&apdu[apdu_len], apdu_size - apdu_len, 5, &unsigned_value);
299+
if (len > 0) {
300+
apdu_len += len;
301+
if (data) {
302+
data->priority = (uint8_t)unsigned_value;
303+
data->use_priority = true;
300304
}
305+
} else if (len == 0) {
306+
/* priority is optional, so do nothing */
307+
} else {
308+
return BACNET_STATUS_ERROR;
301309
}
302310
break;
303311
case BACNET_LIGHTS_WARN:
304312
case BACNET_LIGHTS_WARN_OFF:
305313
case BACNET_LIGHTS_WARN_RELINQUISH:
306314
case BACNET_LIGHTS_STOP:
307-
if ((apdu_size - apdu_len) > 0) {
308-
/* priority [5] Unsigned (1..16) OPTIONAL */
309-
len = bacnet_unsigned_context_decode(
310-
&apdu[apdu_len], apdu_size, 5, &unsigned_value);
311-
if (len > 0) {
312-
apdu_len += len;
313-
if (data) {
314-
data->priority = (uint8_t)unsigned_value;
315-
data->use_priority = true;
316-
}
315+
/* priority [5] Unsigned (1..16) OPTIONAL */
316+
len = bacnet_unsigned_context_decode(
317+
&apdu[apdu_len], apdu_size - apdu_len, 5, &unsigned_value);
318+
if (len > 0) {
319+
apdu_len += len;
320+
if (data) {
321+
data->priority = (uint8_t)unsigned_value;
322+
data->use_priority = true;
317323
}
324+
} else if (len == 0) {
325+
/* priority is optional, so do nothing */
326+
} else {
327+
return BACNET_STATUS_ERROR;
318328
}
319329
break;
320330
default:

‎test/bacnet/lighting/src/main.c‎

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
* @brief test BACnet integer encode/decode APIs
99
*/
1010

11+
#include <string.h>
1112
#include <zephyr/ztest.h>
1213
#include <bacnet/bacdef.h>
1314
#include <bacnet/bactext.h>
@@ -264,6 +265,68 @@ static void testBACnetXYColor(void)
264265
}
265266
}
266267

268+
/**
269+
* @brief Test bounds checking for lighting_command_decode
270+
* Validates fix for CWE-125 out-of-bounds read.
271+
* Old code passed apdu_size instead of (apdu_size - apdu_len) to nested
272+
* decoders, causing them to read past buffer when operation field advanced the
273+
* pointer.
274+
*/
275+
#if defined(CONFIG_ZTEST_NEW_API)
276+
ZTEST(lighting_tests, testBACnetLightingCommandBoundsCheck)
277+
#else
278+
static void testBACnetLightingCommandBoundsCheck(void)
279+
#endif
280+
{
281+
BACNET_LIGHTING_COMMAND test_cases[] = {
282+
/* FADE_TO with optional fade_time and priority */
283+
{ BACNET_LIGHTS_FADE_TO, true, false, false, true, true, 50.0, 0.0, 0.0,
284+
5000, 8 },
285+
/* RAMP_TO with optional ramp_rate and priority */
286+
{ BACNET_LIGHTS_RAMP_TO, true, true, false, false, true, 25.0, 10.0,
287+
0.0, 0, 5 },
288+
/* STEP_UP with optional step_increment and priority */
289+
{ BACNET_LIGHTS_STEP_UP, false, false, true, false, true, 0.0, 0.0, 5.0,
290+
0, 12 },
291+
/* WARN with optional priority */
292+
{ BACNET_LIGHTS_WARN, false, false, false, false, true, 0.0, 0.0, 0.0,
293+
0, 1 },
294+
};
295+
unsigned i, trunc_pos;
296+
int enc_len;
297+
uint8_t apdu_full[MAX_APDU] = { 0 };
298+
uint8_t apdu_trunc[MAX_APDU] = { 0 };
299+
int decode_result;
300+
BACNET_LIGHTING_COMMAND decoded = { 0 };
301+
302+
for (i = 0; i < ARRAY_SIZE(test_cases); i++) {
303+
/* Encode full valid message */
304+
enc_len = lighting_command_encode(apdu_full, &test_cases[i]);
305+
zassert_true(
306+
enc_len > 0, "Failed encode operation %u", test_cases[i].operation);
307+
308+
/* Copy full buffer */
309+
memcpy(apdu_trunc, apdu_full, (size_t)enc_len);
310+
311+
/* Test: Progressively truncate buffer, should return error */
312+
for (trunc_pos = 1; trunc_pos < enc_len; trunc_pos++) {
313+
decode_result =
314+
lighting_command_decode(apdu_trunc, trunc_pos, &decoded);
315+
/* Decoder must not report consuming more bytes than provided */
316+
zassert_true(
317+
decode_result <= 0 || decode_result <= (int)trunc_pos,
318+
"Op %u: truncated to %u bytes returned decode_result %d",
319+
test_cases[i].operation, trunc_pos, decode_result);
320+
}
321+
322+
/* Valid decode with full buffer */
323+
decode_result = lighting_command_decode(apdu_full, enc_len, &decoded);
324+
zassert_equal(
325+
decode_result, enc_len, "Op %u: full buffer decode failed",
326+
test_cases[i].operation);
327+
}
328+
}
329+
267330
#if defined(CONFIG_ZTEST_NEW_API)
268331
ZTEST_SUITE(lighting_tests, NULL, NULL, NULL, NULL, NULL);
269332
#else
@@ -272,7 +335,8 @@ void test_main(void)
272335
ztest_test_suite(
273336
lighting_tests, ztest_unit_test(testBACnetLightingCommandAll),
274337
ztest_unit_test(testBACnetColorCommandAll),
275-
ztest_unit_test(testBACnetXYColor));
338+
ztest_unit_test(testBACnetXYColor),
339+
ztest_unit_test(testBACnetLightingCommandBoundsCheck));
276340

277341
ztest_run_test_suite(lighting_tests);
278342
}

0 commit comments

Comments
 (0)