Skip to content

Commit 726eff9

Browse files
author
Hasnain Virk
committed
Proper size checking for link ADR commands
In a specific branch path 'adr_settings' in link_adr_request() API, the structure adr_settings of type link_adr_params_t will be rendered uninitialized. To prevent this we initialize the construct as zero. In addition to that, to handle the case properly we should check for the command identifier and the command payload length anticipating contiguous blocks of adr commands. If we find a discrepency in size, we should abort.
1 parent d257d72 commit 726eff9

File tree

6 files changed

+61
-15
lines changed

6 files changed

+61
-15
lines changed

‎features/lorawan/lorastack/mac/LoRaMacCommand.cpp

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ lorawan_status_t LoRaMacCommand::process_mac_commands(const uint8_t *payload, ui
149149
int8_t link_adr_dr = DR_0;
150150
int8_t link_adr_txpower = TX_POWER_0;
151151
uint8_t link_adr_nbtrans = 0;
152-
uint8_t link_adr_nb_bytes_pasred = 0;
152+
uint8_t link_adr_nb_bytes_parsed = 0;
153153

154154
// Fill parameter structure
155155
link_adr_req.payload = &payload[mac_index - 1];
@@ -165,7 +165,14 @@ lorawan_status_t LoRaMacCommand::process_mac_commands(const uint8_t *payload, ui
165165
&link_adr_dr,
166166
&link_adr_txpower,
167167
&link_adr_nbtrans,
168-
&link_adr_nb_bytes_pasred);
168+
&link_adr_nb_bytes_parsed);
169+
170+
// If nothing was consumed, we have a malformed packet at our hand
171+
// we bin everything and return. link_adr_nb_bytes_parsed being 0 is
172+
// a magic identifier letting us know that there are payload inconsistencies
173+
if (link_adr_nb_bytes_parsed == 0) {
174+
return LORAWAN_STATUS_UNSUPPORTED;
175+
}
169176

170177
if ((status & 0x07) == 0x07) {
171178
mac_sys_params.channel_data_rate = link_adr_dr;
@@ -174,11 +181,11 @@ lorawan_status_t LoRaMacCommand::process_mac_commands(const uint8_t *payload, ui
174181
}
175182

176183
// Add the answers to the buffer
177-
for (uint8_t i = 0; i < (link_adr_nb_bytes_pasred / 5); i++) {
184+
for (uint8_t i = 0; i < (link_adr_nb_bytes_parsed / 5); i++) {
178185
ret_value = add_link_adr_ans(status);
179186
}
180187
// Update MAC index
181-
mac_index += link_adr_nb_bytes_pasred - 1;
188+
mac_index += link_adr_nb_bytes_parsed - 1;
182189
}
183190
break;
184191
case SRV_MAC_DUTY_CYCLE_REQ:

‎features/lorawan/lorastack/phy/LoRaPHY.cpp

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -309,11 +309,12 @@ lorawan_time_t LoRaPHY::update_band_timeoff(bool joined, bool duty_cycle,
309309
}
310310

311311
uint8_t LoRaPHY::parse_link_ADR_req(const uint8_t *payload,
312+
uint8_t payload_size,
312313
link_adr_params_t *params)
313314
{
314315
uint8_t ret_index = 0;
315316

316-
if (payload[0] == SRV_MAC_LINK_ADR_REQ) {
317+
if (payload_size >= 5) {
317318

318319
// Parse datarate and tx power
319320
params->datarate = payload[1];
@@ -973,13 +974,17 @@ uint8_t LoRaPHY::link_ADR_request(adr_req_params_t *link_adr_req,
973974

974975
verify_adr_params_t verify_params;
975976

976-
while (bytes_processed < link_adr_req->payload_size) {
977+
while (bytes_processed < link_adr_req->payload_size &&
978+
link_adr_req->payload[bytes_processed] == SRV_MAC_LINK_ADR_REQ) {
977979
// Get ADR request parameters
978980
next_index = parse_link_ADR_req(&(link_adr_req->payload[bytes_processed]),
981+
link_adr_req->payload_size - bytes_processed,
979982
&adr_settings);
980983

981984
if (next_index == 0) {
982-
break; // break loop, since no more request has been found
985+
bytes_processed = 0;
986+
// break loop, malformed packet
987+
break;
983988
}
984989

985990
// Update bytes processed
@@ -1024,6 +1029,11 @@ uint8_t LoRaPHY::link_ADR_request(adr_req_params_t *link_adr_req,
10241029
}
10251030
}
10261031

1032+
if (bytes_processed == 0) {
1033+
*nb_bytes_processed = 0;
1034+
return status;
1035+
}
1036+
10271037
if (is_datarate_supported(adr_settings.datarate)) {
10281038
verify_params.status = status;
10291039

‎features/lorawan/lorastack/phy/LoRaPHY.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -608,7 +608,8 @@ class LoRaPHY : private mbed::NonCopyable<LoRaPHY> {
608608
/**
609609
* Parses the parameter of an LinkAdrRequest.
610610
*/
611-
uint8_t parse_link_ADR_req(const uint8_t *payload, link_adr_params_t *adr_params);
611+
uint8_t parse_link_ADR_req(const uint8_t *payload, uint8_t payload_size,
612+
link_adr_params_t *adr_params);
612613

613614
/**
614615
* Verifies and updates the datarate, the TX power and the number of repetitions

‎features/lorawan/lorastack/phy/LoRaPHYAU915.cpp

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -435,12 +435,16 @@ uint8_t LoRaPHYAU915::link_ADR_request(adr_req_params_t *params,
435435
// Initialize local copy of channels mask
436436
copy_channel_mask(temp_channel_masks, channel_mask, AU915_CHANNEL_MASK_SIZE);
437437

438-
while (bytes_processed < params->payload_size) {
438+
while (bytes_processed < params->payload_size &&
439+
params->payload[bytes_processed] == SRV_MAC_LINK_ADR_REQ) {
439440
next_index = parse_link_ADR_req(&(params->payload[bytes_processed]),
441+
params->payload_size,
440442
&adr_settings);
441443

442444
if (next_index == 0) {
443-
break; // break loop, since no more request has been found
445+
bytes_processed = 0;
446+
// break loop, malformed packet
447+
break;
444448
}
445449

446450
// Update bytes processed
@@ -471,6 +475,11 @@ uint8_t LoRaPHYAU915::link_ADR_request(adr_req_params_t *params,
471475
}
472476
}
473477

478+
if (bytes_processed == 0) {
479+
*nb_bytes_parsed = 0;
480+
return status;
481+
}
482+
474483
// FCC 15.247 paragraph F mandates to hop on at least 2 125 kHz channels
475484
if ((adr_settings.datarate < DR_6)
476485
&& (num_active_channels(temp_channel_masks, 0, 4) < 2)) {

‎features/lorawan/lorastack/phy/LoRaPHYCN470.cpp

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -460,13 +460,18 @@ uint8_t LoRaPHYCN470::link_ADR_request(adr_req_params_t *params,
460460
// Initialize local copy of channels mask
461461
copy_channel_mask(temp_channel_masks, channel_mask, CN470_CHANNEL_MASK_SIZE);
462462

463-
while (bytes_processed < params->payload_size) {
463+
while (bytes_processed < params->payload_size &&
464+
params->payload[bytes_processed] == SRV_MAC_LINK_ADR_REQ) {
464465

465466
// Get ADR request parameters
466-
next_index = parse_link_ADR_req(&(params->payload[bytes_processed]), &adr_settings);
467+
next_index = parse_link_ADR_req(&(params->payload[bytes_processed]),
468+
params->payload_size,
469+
&adr_settings);
467470

468471
if (next_index == 0) {
469-
break; // break loop, since no more request has been found
472+
bytes_processed = 0;
473+
// break loop, malformed packet
474+
break;
470475
}
471476

472477
// Update bytes processed
@@ -501,6 +506,11 @@ uint8_t LoRaPHYCN470::link_ADR_request(adr_req_params_t *params,
501506
}
502507
}
503508

509+
if (bytes_processed == 0) {
510+
*nb_bytes_parsed = 0;
511+
return status;
512+
}
513+
504514
verify_params.status = status;
505515
verify_params.adr_enabled = params->adr_enabled;
506516
verify_params.datarate = adr_settings.datarate;

‎features/lorawan/lorastack/phy/LoRaPHYUS915.cpp

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -460,12 +460,16 @@ uint8_t LoRaPHYUS915::link_ADR_request(adr_req_params_t *params,
460460
// Initialize local copy of channels mask
461461
copy_channel_mask(temp_channel_masks, channel_mask, US915_CHANNEL_MASK_SIZE);
462462

463-
while (bytes_processed < params->payload_size) {
463+
while (bytes_processed < params->payload_size &&
464+
params->payload[bytes_processed] == SRV_MAC_LINK_ADR_REQ) {
464465
next_idx = parse_link_ADR_req(&(params->payload[bytes_processed]),
466+
params->payload_size - bytes_processed,
465467
&adr_settings);
466468

467469
if (next_idx == 0) {
468-
break; // break loop, since no more request has been found
470+
bytes_processed = 0;
471+
// break loop, malformed packet
472+
break;
469473
}
470474

471475
// Update bytes processed
@@ -501,6 +505,11 @@ uint8_t LoRaPHYUS915::link_ADR_request(adr_req_params_t *params,
501505
}
502506
}
503507

508+
if (bytes_processed == 0) {
509+
*nb_bytes_parsed = 0;
510+
return status;
511+
}
512+
504513
// FCC 15.247 paragraph F mandates to hop on at least 2 125 kHz channels
505514
if ((adr_settings.datarate < DR_4) &&
506515
(num_active_channels(temp_channel_masks, 0, 4) < 2)) {

0 commit comments

Comments
 (0)