Skip to content

Commit

Permalink
Fix decoding of truncated messages containing variable length arrays …
Browse files Browse the repository at this point in the history 
…[AP-948] (#1381)

# Description

@swift-nav/devinfra

If an SBP message has a variable length array of some type, when
decoding a input wire encoded buffer which has been truncated the
decoder will not produce an error when it should.

 

Imagine a message containing some amount of overhead, then a variable
length array of some type.
```
+---------------------------------------+
| header | elem0 | elem1 | .... | elemN |
+---------------------------------------+
```

In this message the array is not fixed length, rather the number of
elements is variable and is calculated from the size of the input
buffer:
```
num_elems = (sizeof(input_buffer) - sizeof(header)) / sizeof(elem)
```

Decoding of each element is then performed in a loop:
```
for (int i = 0; i < num_elems; i++) {
  result = decode_elem(buffer, &output[i]);
}
```

If the size of the header is 3 bytes and the size of each element is 4
bytes an example representation on the wire could be:
offset
```
      0   1   2   3   4   5   6   7   8   9   A   
      +-----------+---------------+---------------+
      | header    | elem0         | elem1         |
      +-----------+---------------+---------------+
```

And so long as this is what’s fed in to the decoder then `num_elems`
will be correctly calculated as 2 and all input data will be consumed.

But if the input buffer is somehow truncated:
```
offset
      0   1   2   3   4   5   6   7   8   
      +-----------+---------------+-------|
      | header    | elem0         | elem1 |
      +-----------+---------------+-------|
                                          ^
                                          abnormally truncated at this point
```

then due to integer rounding `num_elems` will be calculated as 1. Only
the first element of the array will be decoded (and it will succeed),
but the truncated data at the end of the input buffer will not be
considered at all.

The correct behaviour in this situation should be for the decoder to
notice that the input buffer does not contain an integer number of
elements in the variable length array and return an error

# API compatibility

Does this change introduce a API compatibility risk?

No

## API compatibility plan

If the above is "Yes", please detail the compatibility (or migration)
plan:

N/A

# JIRA Reference

https://swift-nav.atlassian.net/browse/AP-948
  • Loading branch information
@woodfell
woodfell authored Nov 21, 2023
1 parent a39d7b8 commit b2fce6d
Show file tree
Hide file tree
Showing 15 changed files with 179 additions and 0 deletions.
7 changes: 7 additions & 0 deletions c/src/v4/acquisition.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -822,6 +822,9 @@ s8 sbp_msg_acq_sv_profile_encode(uint8_t *buf, uint8_t len, uint8_t *n_written,

bool sbp_msg_acq_sv_profile_decode_internal(sbp_decode_ctx_t *ctx,
sbp_msg_acq_sv_profile_t *msg) {
if (((ctx->buf_len - ctx->offset) % SBP_ACQ_SV_PROFILE_ENCODED_LEN) != 0) {
return false;
}
msg->n_acq_sv_profile =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ACQ_SV_PROFILE_ENCODED_LEN);
for (uint8_t i = 0; i < msg->n_acq_sv_profile; i++) {
Expand Down Expand Up @@ -904,6 +907,10 @@ s8 sbp_msg_acq_sv_profile_dep_encode(uint8_t *buf, uint8_t len,

bool sbp_msg_acq_sv_profile_dep_decode_internal(
sbp_decode_ctx_t *ctx, sbp_msg_acq_sv_profile_dep_t *msg) {
if (((ctx->buf_len - ctx->offset) % SBP_ACQ_SV_PROFILE_DEP_ENCODED_LEN) !=
0) {
return false;
}
msg->n_acq_sv_profile = (uint8_t)((ctx->buf_len - ctx->offset) /
SBP_ACQ_SV_PROFILE_DEP_ENCODED_LEN);
for (uint8_t i = 0; i < msg->n_acq_sv_profile; i++) {
Expand Down
6 changes: 6 additions & 0 deletions c/src/v4/file_io.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,9 @@ bool sbp_msg_fileio_read_resp_decode_internal(sbp_decode_ctx_t *ctx,
if (!sbp_u32_decode(ctx, &msg->sequence)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U8) != 0) {
return false;
}
msg->n_contents =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U8);
for (uint8_t i = 0; i < msg->n_contents; i++) {
Expand Down Expand Up @@ -1009,6 +1012,9 @@ bool sbp_msg_fileio_write_req_decode_internal(sbp_decode_ctx_t *ctx,
&msg->filename, SBP_MSG_FILEIO_WRITE_REQ_FILENAME_MAX, ctx)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U8) != 0) {
return false;
}
msg->n_data = (uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U8);
for (uint8_t i = 0; i < msg->n_data; i++) {
if (!sbp_u8_decode(ctx, &msg->data[i])) {
Expand Down
3 changes: 3 additions & 0 deletions c/src/v4/flash.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ bool sbp_msg_flash_program_decode_internal(sbp_decode_ctx_t *ctx,
if (!sbp_u8_decode(ctx, &msg->addr_len)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U8) != 0) {
return false;
}
msg->addr_len = (uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U8);
for (uint8_t i = 0; i < msg->addr_len; i++) {
if (!sbp_u8_decode(ctx, &msg->data[i])) {
Expand Down
15 changes: 15 additions & 0 deletions c/src/v4/integrity.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,9 @@ bool sbp_msg_ssr_flag_satellites_decode_internal(
if (!sbp_u8_decode(ctx, &msg->n_faulty_sats)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U8) != 0) {
return false;
}
msg->n_faulty_sats =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U8);
for (uint8_t i = 0; i < msg->n_faulty_sats; i++) {
Expand Down Expand Up @@ -564,6 +567,9 @@ bool sbp_msg_ssr_flag_tropo_grid_points_decode_internal(
if (!sbp_u8_decode(ctx, &msg->n_faulty_points)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U16) != 0) {
return false;
}
msg->n_faulty_points =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U16);
for (uint8_t i = 0; i < msg->n_faulty_points; i++) {
Expand Down Expand Up @@ -669,6 +675,9 @@ bool sbp_msg_ssr_flag_iono_grid_points_decode_internal(
if (!sbp_u8_decode(ctx, &msg->n_faulty_points)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U16) != 0) {
return false;
}
msg->n_faulty_points =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U16);
for (uint8_t i = 0; i < msg->n_faulty_points; i++) {
Expand Down Expand Up @@ -774,6 +783,9 @@ bool sbp_msg_ssr_flag_iono_tile_sat_los_decode_internal(
if (!sbp_u8_decode(ctx, &msg->n_faulty_los)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_SV_ID_ENCODED_LEN) != 0) {
return false;
}
msg->n_faulty_los =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_SV_ID_ENCODED_LEN);
for (uint8_t i = 0; i < msg->n_faulty_los; i++) {
Expand Down Expand Up @@ -886,6 +898,9 @@ bool sbp_msg_ssr_flag_iono_grid_point_sat_los_decode_internal(
if (!sbp_u8_decode(ctx, &msg->n_faulty_los)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_SV_ID_ENCODED_LEN) != 0) {
return false;
}
msg->n_faulty_los =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_SV_ID_ENCODED_LEN);
for (uint8_t i = 0; i < msg->n_faulty_los; i++) {
Expand Down
3 changes: 3 additions & 0 deletions c/src/v4/logging.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,9 @@ bool sbp_msg_fwd_decode_internal(sbp_decode_ctx_t *ctx, sbp_msg_fwd_t *msg) {
if (!sbp_u8_decode(ctx, &msg->protocol)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U8) != 0) {
return false;
}
msg->n_fwd_payload =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U8);
for (uint8_t i = 0; i < msg->n_fwd_payload; i++) {
Expand Down
23 changes: 23 additions & 0 deletions c/src/v4/observation.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,10 @@ bool sbp_msg_obs_decode_internal(sbp_decode_ctx_t *ctx, sbp_msg_obs_t *msg) {
if (!sbp_observation_header_decode_internal(ctx, &msg->header)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_PACKED_OBS_CONTENT_ENCODED_LEN) !=
0) {
return false;
}
msg->n_obs = (uint8_t)((ctx->buf_len - ctx->offset) /
SBP_PACKED_OBS_CONTENT_ENCODED_LEN);
for (uint8_t i = 0; i < msg->n_obs; i++) {
Expand Down Expand Up @@ -6486,6 +6490,10 @@ bool sbp_msg_obs_dep_a_decode_internal(sbp_decode_ctx_t *ctx,
if (!sbp_observation_header_dep_decode_internal(ctx, &msg->header)) {
return false;
}
if (((ctx->buf_len - ctx->offset) %
SBP_PACKED_OBS_CONTENT_DEP_A_ENCODED_LEN) != 0) {
return false;
}
msg->n_obs = (uint8_t)((ctx->buf_len - ctx->offset) /
SBP_PACKED_OBS_CONTENT_DEP_A_ENCODED_LEN);
for (uint8_t i = 0; i < msg->n_obs; i++) {
Expand Down Expand Up @@ -6577,6 +6585,10 @@ bool sbp_msg_obs_dep_b_decode_internal(sbp_decode_ctx_t *ctx,
if (!sbp_observation_header_dep_decode_internal(ctx, &msg->header)) {
return false;
}
if (((ctx->buf_len - ctx->offset) %
SBP_PACKED_OBS_CONTENT_DEP_B_ENCODED_LEN) != 0) {
return false;
}
msg->n_obs = (uint8_t)((ctx->buf_len - ctx->offset) /
SBP_PACKED_OBS_CONTENT_DEP_B_ENCODED_LEN);
for (uint8_t i = 0; i < msg->n_obs; i++) {
Expand Down Expand Up @@ -6668,6 +6680,10 @@ bool sbp_msg_obs_dep_c_decode_internal(sbp_decode_ctx_t *ctx,
if (!sbp_observation_header_dep_decode_internal(ctx, &msg->header)) {
return false;
}
if (((ctx->buf_len - ctx->offset) %
SBP_PACKED_OBS_CONTENT_DEP_C_ENCODED_LEN) != 0) {
return false;
}
msg->n_obs = (uint8_t)((ctx->buf_len - ctx->offset) /
SBP_PACKED_OBS_CONTENT_DEP_C_ENCODED_LEN);
for (uint8_t i = 0; i < msg->n_obs; i++) {
Expand Down Expand Up @@ -8726,6 +8742,9 @@ s8 sbp_msg_sv_az_el_encode(uint8_t *buf, uint8_t len, uint8_t *n_written,

bool sbp_msg_sv_az_el_decode_internal(sbp_decode_ctx_t *ctx,
sbp_msg_sv_az_el_t *msg) {
if (((ctx->buf_len - ctx->offset) % SBP_SV_AZ_EL_ENCODED_LEN) != 0) {
return false;
}
msg->n_azel =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_SV_AZ_EL_ENCODED_LEN);
for (uint8_t i = 0; i < msg->n_azel; i++) {
Expand Down Expand Up @@ -8809,6 +8828,10 @@ bool sbp_msg_osr_decode_internal(sbp_decode_ctx_t *ctx, sbp_msg_osr_t *msg) {
if (!sbp_observation_header_decode_internal(ctx, &msg->header)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_PACKED_OSR_CONTENT_ENCODED_LEN) !=
0) {
return false;
}
msg->n_obs = (uint8_t)((ctx->buf_len - ctx->offset) /
SBP_PACKED_OSR_CONTENT_ENCODED_LEN);
for (uint8_t i = 0; i < msg->n_obs; i++) {
Expand Down
12 changes: 12 additions & 0 deletions c/src/v4/piksi.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2323,6 +2323,9 @@ s8 sbp_msg_network_bandwidth_usage_encode(

bool sbp_msg_network_bandwidth_usage_decode_internal(
sbp_decode_ctx_t *ctx, sbp_msg_network_bandwidth_usage_t *msg) {
if (((ctx->buf_len - ctx->offset) % SBP_NETWORK_USAGE_ENCODED_LEN) != 0) {
return false;
}
msg->n_interfaces =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_NETWORK_USAGE_ENCODED_LEN);
for (uint8_t i = 0; i < msg->n_interfaces; i++) {
Expand Down Expand Up @@ -2418,6 +2421,9 @@ bool sbp_msg_cell_modem_status_decode_internal(
if (!sbp_float_decode(ctx, &msg->signal_error_rate)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U8) != 0) {
return false;
}
msg->n_reserved =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U8);
for (uint8_t i = 0; i < msg->n_reserved; i++) {
Expand Down Expand Up @@ -2545,6 +2551,9 @@ bool sbp_msg_specan_dep_decode_internal(sbp_decode_ctx_t *ctx,
if (!sbp_float_decode(ctx, &msg->amplitude_unit)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U8) != 0) {
return false;
}
msg->n_amplitude_value =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U8);
for (uint8_t i = 0; i < msg->n_amplitude_value; i++) {
Expand Down Expand Up @@ -2691,6 +2700,9 @@ bool sbp_msg_specan_decode_internal(sbp_decode_ctx_t *ctx,
if (!sbp_float_decode(ctx, &msg->amplitude_unit)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U8) != 0) {
return false;
}
msg->n_amplitude_value =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U8);
for (uint8_t i = 0; i < msg->n_amplitude_value; i++) {
Expand Down
21 changes: 21 additions & 0 deletions c/src/v4/signing.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,9 @@ bool sbp_msg_ecdsa_certificate_decode_internal(
if (!sbp_u8_decode(ctx, &msg->flags)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U8) != 0) {
return false;
}
msg->n_certificate_bytes =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U8);
for (uint8_t i = 0; i < msg->n_certificate_bytes; i++) {
Expand Down Expand Up @@ -699,6 +702,9 @@ bool sbp_msg_ecdsa_signature_decode_internal(sbp_decode_ctx_t *ctx,
if (!sbp_ecdsa_signature_decode_internal(ctx, &msg->signature)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U8) != 0) {
return false;
}
msg->n_signed_messages =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U8);
for (uint8_t i = 0; i < msg->n_signed_messages; i++) {
Expand Down Expand Up @@ -855,6 +861,9 @@ bool sbp_msg_ecdsa_signature_dep_b_decode_internal(
return false;
}
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U8) != 0) {
return false;
}
msg->n_signed_messages =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U8);
for (uint8_t i = 0; i < msg->n_signed_messages; i++) {
Expand Down Expand Up @@ -1014,6 +1023,9 @@ bool sbp_msg_ecdsa_signature_dep_a_decode_internal(
return false;
}
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U8) != 0) {
return false;
}
msg->n_signed_messages =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U8);
for (uint8_t i = 0; i < msg->n_signed_messages; i++) {
Expand Down Expand Up @@ -1145,6 +1157,9 @@ bool sbp_msg_ed25519_certificate_dep_decode_internal(
return false;
}
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U8) != 0) {
return false;
}
msg->n_certificate_bytes =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U8);
for (uint8_t i = 0; i < msg->n_certificate_bytes; i++) {
Expand Down Expand Up @@ -1262,6 +1277,9 @@ bool sbp_msg_ed25519_signature_dep_a_decode_internal(
return false;
}
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U32) != 0) {
return false;
}
msg->n_signed_messages =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U32);
for (uint8_t i = 0; i < msg->n_signed_messages; i++) {
Expand Down Expand Up @@ -1394,6 +1412,9 @@ bool sbp_msg_ed25519_signature_dep_b_decode_internal(
return false;
}
}
if (((ctx->buf_len - ctx->offset) % SBP_ENCODED_LEN_U32) != 0) {
return false;
}
msg->n_signed_messages =
(uint8_t)((ctx->buf_len - ctx->offset) / SBP_ENCODED_LEN_U32);
for (uint8_t i = 0; i < msg->n_signed_messages; i++) {
Expand Down
8 changes: 8 additions & 0 deletions c/src/v4/solution_meta.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,10 @@ bool sbp_msg_soln_meta_dep_a_decode_internal(sbp_decode_ctx_t *ctx,
if (!sbp_u32_decode(ctx, &msg->last_used_gnss_vel_tow)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_SOLUTION_INPUT_TYPE_ENCODED_LEN) !=
0) {
return false;
}
msg->n_sol_in = (uint8_t)((ctx->buf_len - ctx->offset) /
SBP_SOLUTION_INPUT_TYPE_ENCODED_LEN);
for (uint8_t i = 0; i < msg->n_sol_in; i++) {
Expand Down Expand Up @@ -317,6 +321,10 @@ bool sbp_msg_soln_meta_decode_internal(sbp_decode_ctx_t *ctx,
if (!sbp_u32_decode(ctx, &msg->age_gnss)) {
return false;
}
if (((ctx->buf_len - ctx->offset) % SBP_SOLUTION_INPUT_TYPE_ENCODED_LEN) !=
0) {
return false;
}
msg->n_sol_in = (uint8_t)((ctx->buf_len - ctx->offset) /
SBP_SOLUTION_INPUT_TYPE_ENCODED_LEN);
for (uint8_t i = 0; i < msg->n_sol_in; i++) {
Expand Down
Loading

0 comments on commit b2fce6d

Please sign in to comment.