[MERGED] libosmocore[master]: TLVP_PRESENT() should not return TRUE after tlv_parse() fails.

[Harald Welte]

Harald Welte has submitted this change and it was merged.

Change subject: TLVP_PRESENT() should not return TRUE after tlv_parse() fails.
......................................................................


TLVP_PRESENT() should not return TRUE after tlv_parse() fails.

If the length provided in the patcket exceeds the buffer length,
tlv_parse() returns -2 but leaves tlv.val and tlv.len initializd.

Many callers of tlv_parse() do not check its return value, but
rely on TLVP_PRESENT() to see if a particular TLV was parsed
successfully. By clearing tlv.val and tlv.len we make it less
likely that those callers will use an overlong TLV length value.

Change-Id: I4dda6938e1650b4bcaac45809a4763f86f5a9794
---
M src/gsm/tlv_parser.c
1 file changed, 10 insertions(+), 4 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/gsm/tlv_parser.c b/src/gsm/tlv_parser.c
index ead856c..9b1fb17 100644
--- a/src/gsm/tlv_parser.c
+++ b/src/gsm/tlv_parser.c
@@ -127,7 +127,7 @@
  *  \param[in] def structure defining the valid TLV tags / configurations
  *  \param[in] buf the input data buffer to be parsed
  *  \param[in] buf_len length of the input data buffer
- *  \returns number of bytes consumed by the TLV entry / IE parsed
+ *  \returns number of bytes consumed by the TLV entry / IE parsed; negative in case of error
  */
 int tlv_parse_one(uint8_t *o_tag, uint16_t *o_len, const uint8_t **o_val,
 		  const struct tlv_definition *def,
@@ -227,7 +227,7 @@
  *  \param[in] buf_len length of the input data buffer
  *  \param[in] lv_tag an initial LV tag at the start of the buffer
  *  \param[in] lv_tag2 a second initial LV tag following the \a lv_tag
- *  \returns number of bytes consumed by the TLV entry / IE parsed
+ *  \returns number of bytes consumed by the TLV entry / IE parsed; negative in case of error
  */
 int tlv_parse(struct tlv_parsed *dec, const struct tlv_definition *def,
 	      const uint8_t *buf, int buf_len, uint8_t lv_tag,
@@ -244,8 +244,11 @@
 		dec->lv[lv_tag].val = &buf[ofs+1];
 		dec->lv[lv_tag].len = buf[ofs];
 		len = dec->lv[lv_tag].len + 1;
-		if (ofs + len > buf_len)
+		if (ofs + len > buf_len) {
+			dec->lv[lv_tag].val = NULL;
+			dec->lv[lv_tag].len = 0;
 			return -2;
+		}
 		num_parsed++;
 		ofs += len;
 	}
@@ -255,8 +258,11 @@
 		dec->lv[lv_tag2].val = &buf[ofs+1];
 		dec->lv[lv_tag2].len = buf[ofs];
 		len = dec->lv[lv_tag2].len + 1;
-		if (ofs + len > buf_len)
+		if (ofs + len > buf_len) {
+			dec->lv[lv_tag2].val = NULL;
+			dec->lv[lv_tag2].len = 0;
 			return -2;
+		}
 		num_parsed++;
 		ofs += len;
 	}

-- 
To view, visit https://gerrit.osmocom.org/5689
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I4dda6938e1650b4bcaac45809a4763f86f5a9794
Gerrit-PatchSet: 6
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Stefan Sperling <ssperling at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Stefan Sperling <ssperling at sysmocom.de>