From 726eff9305e22ae6ccdb2f6f4b15446f40eb9153 Mon Sep 17 00:00:00 2001 From: Hasnain Virk Date: Mon, 4 Feb 2019 15:01:35 +0200 Subject: [PATCH] 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. --- .../lorawan/lorastack/mac/LoRaMacCommand.cpp | 15 +++++++++++---- features/lorawan/lorastack/phy/LoRaPHY.cpp | 16 +++++++++++++--- features/lorawan/lorastack/phy/LoRaPHY.h | 3 ++- features/lorawan/lorastack/phy/LoRaPHYAU915.cpp | 13 +++++++++++-- features/lorawan/lorastack/phy/LoRaPHYCN470.cpp | 16 +++++++++++++--- features/lorawan/lorastack/phy/LoRaPHYUS915.cpp | 13 +++++++++++-- 6 files changed, 61 insertions(+), 15 deletions(-) diff --git a/features/lorawan/lorastack/mac/LoRaMacCommand.cpp b/features/lorawan/lorastack/mac/LoRaMacCommand.cpp index b3447cde7f..d2ebf397ff 100644 --- a/features/lorawan/lorastack/mac/LoRaMacCommand.cpp +++ b/features/lorawan/lorastack/mac/LoRaMacCommand.cpp @@ -149,7 +149,7 @@ lorawan_status_t LoRaMacCommand::process_mac_commands(const uint8_t *payload, ui int8_t link_adr_dr = DR_0; int8_t link_adr_txpower = TX_POWER_0; uint8_t link_adr_nbtrans = 0; - uint8_t link_adr_nb_bytes_pasred = 0; + uint8_t link_adr_nb_bytes_parsed = 0; // Fill parameter structure link_adr_req.payload = &payload[mac_index - 1]; @@ -165,7 +165,14 @@ lorawan_status_t LoRaMacCommand::process_mac_commands(const uint8_t *payload, ui &link_adr_dr, &link_adr_txpower, &link_adr_nbtrans, - &link_adr_nb_bytes_pasred); + &link_adr_nb_bytes_parsed); + + // If nothing was consumed, we have a malformed packet at our hand + // we bin everything and return. link_adr_nb_bytes_parsed being 0 is + // a magic identifier letting us know that there are payload inconsistencies + if (link_adr_nb_bytes_parsed == 0) { + return LORAWAN_STATUS_UNSUPPORTED; + } if ((status & 0x07) == 0x07) { 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 } // Add the answers to the buffer - for (uint8_t i = 0; i < (link_adr_nb_bytes_pasred / 5); i++) { + for (uint8_t i = 0; i < (link_adr_nb_bytes_parsed / 5); i++) { ret_value = add_link_adr_ans(status); } // Update MAC index - mac_index += link_adr_nb_bytes_pasred - 1; + mac_index += link_adr_nb_bytes_parsed - 1; } break; case SRV_MAC_DUTY_CYCLE_REQ: diff --git a/features/lorawan/lorastack/phy/LoRaPHY.cpp b/features/lorawan/lorastack/phy/LoRaPHY.cpp index 1e6cc64169..dd3cba9217 100644 --- a/features/lorawan/lorastack/phy/LoRaPHY.cpp +++ b/features/lorawan/lorastack/phy/LoRaPHY.cpp @@ -309,11 +309,12 @@ lorawan_time_t LoRaPHY::update_band_timeoff(bool joined, bool duty_cycle, } uint8_t LoRaPHY::parse_link_ADR_req(const uint8_t *payload, + uint8_t payload_size, link_adr_params_t *params) { uint8_t ret_index = 0; - if (payload[0] == SRV_MAC_LINK_ADR_REQ) { + if (payload_size >= 5) { // Parse datarate and tx power params->datarate = payload[1]; @@ -973,13 +974,17 @@ uint8_t LoRaPHY::link_ADR_request(adr_req_params_t *link_adr_req, verify_adr_params_t verify_params; - while (bytes_processed < link_adr_req->payload_size) { + while (bytes_processed < link_adr_req->payload_size && + link_adr_req->payload[bytes_processed] == SRV_MAC_LINK_ADR_REQ) { // Get ADR request parameters next_index = parse_link_ADR_req(&(link_adr_req->payload[bytes_processed]), + link_adr_req->payload_size - bytes_processed, &adr_settings); if (next_index == 0) { - break; // break loop, since no more request has been found + bytes_processed = 0; + // break loop, malformed packet + break; } // Update bytes processed @@ -1024,6 +1029,11 @@ uint8_t LoRaPHY::link_ADR_request(adr_req_params_t *link_adr_req, } } + if (bytes_processed == 0) { + *nb_bytes_processed = 0; + return status; + } + if (is_datarate_supported(adr_settings.datarate)) { verify_params.status = status; diff --git a/features/lorawan/lorastack/phy/LoRaPHY.h b/features/lorawan/lorastack/phy/LoRaPHY.h index d6be36b1dc..f68f646726 100644 --- a/features/lorawan/lorastack/phy/LoRaPHY.h +++ b/features/lorawan/lorastack/phy/LoRaPHY.h @@ -608,7 +608,8 @@ protected: /** * Parses the parameter of an LinkAdrRequest. */ - uint8_t parse_link_ADR_req(const uint8_t *payload, link_adr_params_t *adr_params); + uint8_t parse_link_ADR_req(const uint8_t *payload, uint8_t payload_size, + link_adr_params_t *adr_params); /** * Verifies and updates the datarate, the TX power and the number of repetitions diff --git a/features/lorawan/lorastack/phy/LoRaPHYAU915.cpp b/features/lorawan/lorastack/phy/LoRaPHYAU915.cpp index 46ea344860..b41091e8e8 100644 --- a/features/lorawan/lorastack/phy/LoRaPHYAU915.cpp +++ b/features/lorawan/lorastack/phy/LoRaPHYAU915.cpp @@ -435,12 +435,16 @@ uint8_t LoRaPHYAU915::link_ADR_request(adr_req_params_t *params, // Initialize local copy of channels mask copy_channel_mask(temp_channel_masks, channel_mask, AU915_CHANNEL_MASK_SIZE); - while (bytes_processed < params->payload_size) { + while (bytes_processed < params->payload_size && + params->payload[bytes_processed] == SRV_MAC_LINK_ADR_REQ) { next_index = parse_link_ADR_req(&(params->payload[bytes_processed]), + params->payload_size, &adr_settings); if (next_index == 0) { - break; // break loop, since no more request has been found + bytes_processed = 0; + // break loop, malformed packet + break; } // Update bytes processed @@ -471,6 +475,11 @@ uint8_t LoRaPHYAU915::link_ADR_request(adr_req_params_t *params, } } + if (bytes_processed == 0) { + *nb_bytes_parsed = 0; + return status; + } + // FCC 15.247 paragraph F mandates to hop on at least 2 125 kHz channels if ((adr_settings.datarate < DR_6) && (num_active_channels(temp_channel_masks, 0, 4) < 2)) { diff --git a/features/lorawan/lorastack/phy/LoRaPHYCN470.cpp b/features/lorawan/lorastack/phy/LoRaPHYCN470.cpp index 01614e34f5..da46aea6bf 100644 --- a/features/lorawan/lorastack/phy/LoRaPHYCN470.cpp +++ b/features/lorawan/lorastack/phy/LoRaPHYCN470.cpp @@ -460,13 +460,18 @@ uint8_t LoRaPHYCN470::link_ADR_request(adr_req_params_t *params, // Initialize local copy of channels mask copy_channel_mask(temp_channel_masks, channel_mask, CN470_CHANNEL_MASK_SIZE); - while (bytes_processed < params->payload_size) { + while (bytes_processed < params->payload_size && + params->payload[bytes_processed] == SRV_MAC_LINK_ADR_REQ) { // Get ADR request parameters - next_index = parse_link_ADR_req(&(params->payload[bytes_processed]), &adr_settings); + next_index = parse_link_ADR_req(&(params->payload[bytes_processed]), + params->payload_size, + &adr_settings); if (next_index == 0) { - break; // break loop, since no more request has been found + bytes_processed = 0; + // break loop, malformed packet + break; } // Update bytes processed @@ -501,6 +506,11 @@ uint8_t LoRaPHYCN470::link_ADR_request(adr_req_params_t *params, } } + if (bytes_processed == 0) { + *nb_bytes_parsed = 0; + return status; + } + verify_params.status = status; verify_params.adr_enabled = params->adr_enabled; verify_params.datarate = adr_settings.datarate; diff --git a/features/lorawan/lorastack/phy/LoRaPHYUS915.cpp b/features/lorawan/lorastack/phy/LoRaPHYUS915.cpp index abc496be13..d454fb24e4 100644 --- a/features/lorawan/lorastack/phy/LoRaPHYUS915.cpp +++ b/features/lorawan/lorastack/phy/LoRaPHYUS915.cpp @@ -460,12 +460,16 @@ uint8_t LoRaPHYUS915::link_ADR_request(adr_req_params_t *params, // Initialize local copy of channels mask copy_channel_mask(temp_channel_masks, channel_mask, US915_CHANNEL_MASK_SIZE); - while (bytes_processed < params->payload_size) { + while (bytes_processed < params->payload_size && + params->payload[bytes_processed] == SRV_MAC_LINK_ADR_REQ) { next_idx = parse_link_ADR_req(&(params->payload[bytes_processed]), + params->payload_size - bytes_processed, &adr_settings); if (next_idx == 0) { - break; // break loop, since no more request has been found + bytes_processed = 0; + // break loop, malformed packet + break; } // Update bytes processed @@ -501,6 +505,11 @@ uint8_t LoRaPHYUS915::link_ADR_request(adr_req_params_t *params, } } + if (bytes_processed == 0) { + *nb_bytes_parsed = 0; + return status; + } + // FCC 15.247 paragraph F mandates to hop on at least 2 125 kHz channels if ((adr_settings.datarate < DR_4) && (num_active_channels(temp_channel_masks, 0, 4) < 2)) {