<p>laforge <strong>submitted</strong> this change.</p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/22121">View Change</a></p><div style="white-space:pre-wrap">Approvals:
Jenkins Builder: Verified
pespin: Looks good to me, approved
</div><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, 115 insertions(+), 15 deletions(-)<br><br></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 159b42b..5640abe 100644</span><br><span>--- a/src/gsm/tlv_parser.c</span><br><span>+++ b/src/gsm/tlv_parser.c</span><br><span>@@ -231,7 +231,10 @@</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 style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (buf_len < 1)</span><br><span style="color: hsl(120, 100%, 40%);">+ return -1;</span><br><span> </span><br><span> tag = *buf;</span><br><span> *o_tag = tag;</span><br><span>@@ -264,56 +267,54 @@</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 -1;</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 -2;</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 -1;</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 -1;</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 -2;</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 -1;</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 -1;</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 -2;</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 -1;</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 -2;</span><br><span> break;</span><br><span> default:</span><br><span> return -3;</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%);">+ *o_val = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+ return -2;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</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..148a7c2 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) == -2);</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) == -2);</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) == -2);</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) == -2);</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) == -1);</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) == -2);</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) == -1);</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) == -1);</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) == -2);</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) == -1);</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) == -2);</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) == -1);</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) == -1);</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/+/22121">change 22121</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/+/22121"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: for-1.4 </div>
<div style="display:none"> Gerrit-Change-Id: I98b02c914c9e3ecf56050af846292aa6979d7508 </div>
<div style="display:none"> Gerrit-Change-Number: 22121 </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-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>