<p>laforge has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/22113">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">tlv_parser: Fix various out-of-bounds accesses<br><br>The libosmocore TLV parser had a number of insufficient bounds checks<br>leading to reads beyond the end of the respective input buffer.<br><br>This patch<br>* adds proper out-of-bounds checks to all TLV types<br>* simplifies some of the existing checks<br>* introduces test cases to test all the corner cases<br> where either TAG, or length, or value are not fully contained<br> in the input buffer.<br><br>Thanks to Ilja Van Sprundel for reporting these problems.<br><br>Change-Id: I98b02c914c9e3ecf56050af846292aa6979d7508<br>---<br>M src/gsm/tlv_parser.c<br>M tests/tlv/tlv_test.c<br>M tests/tlv/tlv_test.ok<br>3 files changed, 114 insertions(+), 15 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/13/22113/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/gsm/tlv_parser.c b/src/gsm/tlv_parser.c</span><br><span>index 7930d64..4da7e08 100644</span><br><span>--- a/src/gsm/tlv_parser.c</span><br><span>+++ b/src/gsm/tlv_parser.c</span><br><span>@@ -232,7 +232,7 @@</span><br><span> const uint8_t *buf, int buf_len)</span><br><span> {</span><br><span> uint8_t tag;</span><br><span style="color: hsl(0, 100%, 40%);">- int len;</span><br><span style="color: hsl(120, 100%, 40%);">+ int len; /* number of bytes consumed by TLV entry */</span><br><span> </span><br><span> tag = *buf;</span><br><span> *o_tag = tag;</span><br><span>@@ -254,67 +254,67 @@</span><br><span> len = 1;</span><br><span> break;</span><br><span> case TLV_TYPE_TV:</span><br><span style="color: hsl(120, 100%, 40%);">+ if (buf_len < 1)</span><br><span style="color: hsl(120, 100%, 40%);">+ return OSMO_TLVP_ERR_OFS_BEYOND_BUFFER;</span><br><span> *o_val = buf+1;</span><br><span> *o_len = 1;</span><br><span> len = 2;</span><br><span> break;</span><br><span> case TLV_TYPE_FIXED:</span><br><span style="color: hsl(120, 100%, 40%);">+ if (buf_len < 1)</span><br><span style="color: hsl(120, 100%, 40%);">+ return OSMO_TLVP_ERR_OFS_BEYOND_BUFFER;</span><br><span> *o_val = buf+1;</span><br><span> *o_len = def->def[tag].fixed_len;</span><br><span> len = def->def[tag].fixed_len + 1;</span><br><span> break;</span><br><span> case TLV_TYPE_TLV:</span><br><span> tlv: /* GSM TS 04.07 11.2.4: Type 4 TLV */</span><br><span style="color: hsl(0, 100%, 40%);">- if (buf + 1 > buf + buf_len)</span><br><span style="color: hsl(120, 100%, 40%);">+ if (buf_len < 2)</span><br><span> return OSMO_TLVP_ERR_OFS_BEYOND_BUFFER;</span><br><span> *o_val = buf+2;</span><br><span> *o_len = *(buf+1);</span><br><span> len = *o_len + 2;</span><br><span style="color: hsl(0, 100%, 40%);">- if (len > buf_len)</span><br><span style="color: hsl(0, 100%, 40%);">- return OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER;</span><br><span> break;</span><br><span> case TLV_TYPE_vTvLV_GAN: /* 44.318 / 11.1.4 */</span><br><span> /* FIXME: variable-length TAG! */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (buf_len < 2)</span><br><span style="color: hsl(120, 100%, 40%);">+ return OSMO_TLVP_ERR_OFS_BEYOND_BUFFER;</span><br><span> if (*(buf+1) & 0x80) {</span><br><span style="color: hsl(0, 100%, 40%);">- /* like TL16Vbut without highest bit of len */</span><br><span style="color: hsl(0, 100%, 40%);">- if (2 > buf_len)</span><br><span style="color: hsl(120, 100%, 40%);">+ if (buf_len < 3)</span><br><span> return OSMO_TLVP_ERR_OFS_BEYOND_BUFFER;</span><br><span style="color: hsl(120, 100%, 40%);">+ /* like TL16Vbut without highest bit of len */</span><br><span> *o_val = buf+3;</span><br><span> *o_len = (*(buf+1) & 0x7F) << 8 | *(buf+2);</span><br><span> len = *o_len + 3;</span><br><span style="color: hsl(0, 100%, 40%);">- if (len > buf_len)</span><br><span style="color: hsl(0, 100%, 40%);">- return OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER;</span><br><span> } else {</span><br><span> /* like TLV */</span><br><span> goto tlv;</span><br><span> }</span><br><span> break;</span><br><span> case TLV_TYPE_TvLV:</span><br><span style="color: hsl(120, 100%, 40%);">+ if (buf_len < 2)</span><br><span style="color: hsl(120, 100%, 40%);">+ return OSMO_TLVP_ERR_OFS_BEYOND_BUFFER;</span><br><span> if (*(buf+1) & 0x80) {</span><br><span> /* like TLV, but without highest bit of len */</span><br><span style="color: hsl(0, 100%, 40%);">- if (buf + 1 > buf + buf_len)</span><br><span style="color: hsl(0, 100%, 40%);">- return OSMO_TLVP_ERR_OFS_BEYOND_BUFFER;</span><br><span> *o_val = buf+2;</span><br><span> *o_len = *(buf+1) & 0x7f;</span><br><span> len = *o_len + 2;</span><br><span style="color: hsl(0, 100%, 40%);">- if (len > buf_len)</span><br><span style="color: hsl(0, 100%, 40%);">- return OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER;</span><br><span> break;</span><br><span> }</span><br><span> /* like TL16V, fallthrough */</span><br><span> case TLV_TYPE_TL16V:</span><br><span style="color: hsl(0, 100%, 40%);">- if (2 > buf_len)</span><br><span style="color: hsl(120, 100%, 40%);">+ if (buf_len < 3)</span><br><span> return OSMO_TLVP_ERR_OFS_BEYOND_BUFFER;</span><br><span> *o_val = buf+3;</span><br><span> *o_len = *(buf+1) << 8 | *(buf+2);</span><br><span> len = *o_len + 3;</span><br><span style="color: hsl(0, 100%, 40%);">- if (len > buf_len)</span><br><span style="color: hsl(0, 100%, 40%);">- return OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER;</span><br><span> break;</span><br><span> default:</span><br><span> return OSMO_TLVP_ERR_UNKNOWN_TLV_TYPE;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ if (buf_len < len)</span><br><span style="color: hsl(120, 100%, 40%);">+ return OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER;</span><br><span> return len;</span><br><span> }</span><br><span> </span><br><span>diff --git a/tests/tlv/tlv_test.c b/tests/tlv/tlv_test.c</span><br><span>index 925d762..3b4f441 100644</span><br><span>--- a/tests/tlv/tlv_test.c</span><br><span>+++ b/tests/tlv/tlv_test.c</span><br><span>@@ -332,6 +332,97 @@</span><br><span> msgb_free(msg);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+static void test_tlv_parser_bounds()</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ struct tlv_definition tdef;</span><br><span style="color: hsl(120, 100%, 40%);">+ struct tlv_parsed dec;</span><br><span style="color: hsl(120, 100%, 40%);">+ uint8_t buf[32];</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ memset(&tdef, 0, sizeof(tdef));</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("Testing TLV_TYPE_T decoder for out-of-bounds\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ tdef.def[0x23].type = TLV_TYPE_T;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[0] = 0x23;</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 1, 0, 0) == 1);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 0, 0, 0) == 0);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("Testing TLV_TYPE_TV decoder for out-of-bounds\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ tdef.def[0x23].type = TLV_TYPE_TV;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[0] = 0x23;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[1] = 0x42;</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 2, 0, 0) == 1);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(*TLVP_VAL(&dec, 0x23) == buf[1]);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 1, 0, 0) == OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("Testing TLV_TYPE_FIXED decoder for out-of-bounds\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ tdef.def[0x23].type = TLV_TYPE_FIXED;</span><br><span style="color: hsl(120, 100%, 40%);">+ tdef.def[0x23].fixed_len = 2;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[0] = 0x23;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[1] = 0x42;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[2] = 0x55;</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 3, 0, 0) == 1);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 2, 0, 0) == OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("Testing TLV_TYPE_TLV decoder for out-of-bounds\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ tdef.def[0x23].type = TLV_TYPE_TLV;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[0] = 0x23;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[1] = 0x02;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[2] = 0x55;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[3] = 0xAA;</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 4, 0, 0) == 1);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == &buf[2]);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 3, 0, 0) == OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 2, 0, 0) == OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 1, 0, 0) == OSMO_TLVP_ERR_OFS_BEYOND_BUFFER);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("Testing TLV_TYPE_vTvLV_GAN decoder for out-of-bounds\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ tdef.def[0x23].type = TLV_TYPE_vTvLV_GAN;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[0] = 0x23;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[1] = 0x80;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[2] = 0x01;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[3] = 0xAA;</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 4, 0, 0) == 1);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == &buf[3]);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 3, 0, 0) == OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 2, 0, 0) == OSMO_TLVP_ERR_OFS_BEYOND_BUFFER);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 1, 0, 0) == OSMO_TLVP_ERR_OFS_BEYOND_BUFFER);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("Testing TLV_TYPE_TvLV decoder for out-of-bounds\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ tdef.def[0x23].type = TLV_TYPE_TvLV;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[0] = 0x23;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[1] = 0x81;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[2] = 0xAA;</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 3, 0, 0) == 1);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == &buf[2]);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 2, 0, 0) == OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 1, 0, 0) == OSMO_TLVP_ERR_OFS_BEYOND_BUFFER);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("Testing TLV_TYPE_TL16V decoder for out-of-bounds\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ tdef.def[0x23].type = TLV_TYPE_TL16V;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[0] = 0x23;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[1] = 0x00;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[2] = 0x01;</span><br><span style="color: hsl(120, 100%, 40%);">+ buf[3] = 0xAA;</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 4, 0, 0) == 1);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == &buf[3]);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 3, 0, 0) == OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 2, 0, 0) == OSMO_TLVP_ERR_OFS_BEYOND_BUFFER);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 1, 0, 0) == OSMO_TLVP_ERR_OFS_BEYOND_BUFFER);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> int main(int argc, char **argv)</span><br><span> {</span><br><span> //osmo_init_logging2(ctx, &info);</span><br><span>@@ -339,6 +430,7 @@</span><br><span> test_tlv_shift_functions();</span><br><span> test_tlv_repeated_ie();</span><br><span> test_tlv_encoder();</span><br><span style="color: hsl(120, 100%, 40%);">+ test_tlv_parser_bounds();</span><br><span> </span><br><span> printf("Done.\n");</span><br><span> return EXIT_SUCCESS;</span><br><span>diff --git a/tests/tlv/tlv_test.ok b/tests/tlv/tlv_test.ok</span><br><span>index f3f0fd4..e24b889 100644</span><br><span>--- a/tests/tlv/tlv_test.ok</span><br><span>+++ b/tests/tlv/tlv_test.ok</span><br><span>@@ -1,4 +1,11 @@</span><br><span> Test shift functions</span><br><span> Testing TLV encoder by decoding + re-encoding binary</span><br><span> Testing TLV encoder with IE ordering</span><br><span style="color: hsl(120, 100%, 40%);">+Testing TLV_TYPE_T decoder for out-of-bounds</span><br><span style="color: hsl(120, 100%, 40%);">+Testing TLV_TYPE_TV decoder for out-of-bounds</span><br><span style="color: hsl(120, 100%, 40%);">+Testing TLV_TYPE_FIXED decoder for out-of-bounds</span><br><span style="color: hsl(120, 100%, 40%);">+Testing TLV_TYPE_TLV decoder for out-of-bounds</span><br><span style="color: hsl(120, 100%, 40%);">+Testing TLV_TYPE_vTvLV_GAN decoder for out-of-bounds</span><br><span style="color: hsl(120, 100%, 40%);">+Testing TLV_TYPE_TvLV decoder for out-of-bounds</span><br><span style="color: hsl(120, 100%, 40%);">+Testing TLV_TYPE_TL16V decoder for out-of-bounds</span><br><span> Done.</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/22113">change 22113</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/libosmocore/+/22113"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I98b02c914c9e3ecf56050af846292aa6979d7508 </div>
<div style="display:none"> Gerrit-Change-Number: 22113 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>