wbokslag has uploaded this change for review. (
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(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-tetra refs/changes/47/29447/1
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: 1
Gerrit-Owner: wbokslag <[email protected]>
Gerrit-MessageType: newchange
