Change in osmo-tetra[master]: More robust bounds checking on llc pdu parsing

laforge has submitted this change. ( https://gerrit.osmocom.org/c/osmo-tetra/+/29447 ) Change subject: More robust bounds checking on llc pdu parsing ...................................................................... More robust bounds checking on llc pdu parsing We now have a list containing the lengths of the different llc pdu type minimum lengths Before parsing the pdu, we validate the l2len is indeed sufficient to contain the pdu This prevents out-of-bounds reads for corrupted packets. Change-Id: I118ba2227a22afd295fffaa51aab3e45e85ff3d7 --- M src/tetra_llc_pdu.c 1 file changed, 33 insertions(+), 24 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/src/tetra_llc_pdu.c b/src/tetra_llc_pdu.c index a4c520d..d4a1eb1 100644 --- a/src/tetra_llc_pdu.c +++ b/src/tetra_llc_pdu.c @@ -82,6 +82,25 @@ return get_value_string(pdut_dec_names, pdut); } +static const uint8_t tetra_llc_pdu_lengths[16] = { + 6, /* TLLC_PDUT_BL_ADATA */ + 5, /* TLLC_PDUT_BL_DATA */ + 4, /* TLLC_PDUT_BL_UDATA */ + 5, /* TLLC_PDUT_BL_ACK */ + 6 + 32, /* TLLC_PDUT_BL_ADATA_FCS */ + 5 + 32, /* TLLC_PDUT_BL_DATA_FCS */ + 4 + 32, /* TLLC_PDUT_BL_UDATA_FCS */ + 5 + 32, /* TLLC_PDUT_BL_ACK_FCS */ + 0, /* Not implemented, TLLC_PDUT_AL_SETUP */ + 13, /* TLLC_PDUT_AL_DATA_FINAL */ + 17, /* TLLC_PDUT_AL_UDATA_UFINAL */ + 1, /* Not implemented, TLLC_PDUT_AL_ACK_RNR */ + 0, /* Not implemented, TLLC_PDUT_AL_RECONNECT */ + 0, /* Not implemented, TLLC_PDUT_SUPPL */ + 0, /* Not implemented, TLLC_PDUT_L2SIG */ + 0, /* Not implemented, TLLD_PDUT_AL_DISC */ +}; + static uint32_t tetra_llc_compute_fcs(const uint8_t *buf, int len) { uint32_t crc = 0xFFFFFFFF; @@ -99,7 +118,7 @@ return ~crc; } -static int tetra_llc_check_fcs(struct tetra_llc_pdu *lpp, uint8_t *buf, int len) +static bool tetra_llc_check_fcs(struct tetra_llc_pdu *lpp, uint8_t *buf, int len) { uint32_t computed_fcs = tetra_llc_compute_fcs(buf, len); return lpp->fcs == computed_fcs; @@ -110,12 +129,19 @@ uint8_t *cur = buf; uint8_t pdu_type; - lpp->have_fcs = 0; + lpp->have_fcs = false; lpp->fcs = 0; - lpp->fcs_invalid = 0; + lpp->fcs_invalid = false; pdu_type = bits_to_uint(cur, 4); cur += 4; + /* Check length to prevent out of bounds reads */ + if (len < tetra_llc_pdu_lengths[pdu_type]) { + printf("WARNING llc pdu too small to parse, needed %d\n", tetra_llc_pdu_lengths[pdu_type]); + lpp->tl_sdu_len = 0; + return len; + } + switch (pdu_type) { case TLLC_PDUT_BL_ADATA: @@ -127,12 +153,8 @@ lpp->pdu_type = TLLC_PDUT_DEC_BL_ADATA; if (pdu_type == TLLC_PDUT_BL_ADATA_FCS) { - if (lpp->tl_sdu_len < 32) { - printf("\nWARNING frame too small for FCS (%d)\n", lpp->tl_sdu_len); - goto out; - } lpp->tl_sdu_len -= 32; - lpp->have_fcs = 1; + lpp->have_fcs = true; lpp->fcs = bits_to_uint(buf + len - 32, 32); lpp->fcs_invalid = !tetra_llc_check_fcs(lpp, cur, lpp->tl_sdu_len); } @@ -146,12 +168,8 @@ lpp->pdu_type = TLLC_PDUT_DEC_BL_DATA; if (pdu_type == TLLC_PDUT_BL_DATA_FCS) { - if (lpp->tl_sdu_len < 32) { - printf("\nWARNING frame too small for FCS (%d)\n", lpp->tl_sdu_len); - goto out; - } lpp->tl_sdu_len -= 32; - lpp->have_fcs = 1; + lpp->have_fcs = true; lpp->fcs = bits_to_uint(buf + len - 32, 32); lpp->fcs_invalid = !tetra_llc_check_fcs(lpp, cur, lpp->tl_sdu_len); } @@ -164,12 +182,8 @@ lpp->pdu_type = TLLC_PDUT_DEC_BL_UDATA; if (pdu_type == TLLC_PDUT_BL_UDATA_FCS) { - if (lpp->tl_sdu_len < 32) { - printf("\nWARNING frame too small for FCS (%d)\n", lpp->tl_sdu_len); - goto out; - } lpp->tl_sdu_len -= 32; - lpp->have_fcs = 1; + lpp->have_fcs = true; lpp->fcs = bits_to_uint(buf + len - 32, 32); lpp->fcs_invalid = !tetra_llc_check_fcs(lpp, cur, lpp->tl_sdu_len); } @@ -183,12 +197,8 @@ lpp->pdu_type = TLLC_PDUT_DEC_BL_ACK; if (pdu_type == TLLC_PDUT_BL_ACK_FCS) { - if (lpp->tl_sdu_len < 32) { - printf("\nWARNING frame too small for FCS (%d)\n", lpp->tl_sdu_len); - goto out; - } lpp->tl_sdu_len -= 32; - lpp->have_fcs = 1; + lpp->have_fcs = true; lpp->fcs = bits_to_uint(buf + len - 32, 32); lpp->fcs_invalid = !tetra_llc_check_fcs(lpp, cur, lpp->tl_sdu_len); } @@ -286,7 +296,6 @@ lpp->tl_sdu_len = 0; } -out: /* Sanity check to prevent (further) out of bounds reads */ if (len < cur - buf) { lpp->tl_sdu_len = 0; -- To view, visit https://gerrit.osmocom.org/c/osmo-tetra/+/29447 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-tetra Gerrit-Branch: master Gerrit-Change-Id: I118ba2227a22afd295fffaa51aab3e45e85ff3d7 Gerrit-Change-Number: 29447 Gerrit-PatchSet: 2 Gerrit-Owner: wbokslag &lt;w.bokslag(a)midnightblue.nl&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-MessageType: merged