Attention is currently required from: laforge, pespin. dexter has posted comments on this change by dexter. ( https://gerrit.osmocom.org/c/osmo-msc/+/43229?usp=email )
Change subject: sgs_iface: guard against over-long MME name IEs. ...................................................................... Patch Set 2: (5 comments) Patchset: PS2: @pespin: The things you have pointed out are not relevant for the security problem this patch addresses, so I have addressed those points in a follow up-patch after this one. (Except the open question with the length, but lets discuss this one also in a follow up patch then.) File src/libmsc/sgs_iface.c: https://gerrit.osmocom.org/c/osmo-msc/+/43229/comment/cef9f704_4eaf20bf?usp=email : PS2, Line 165: const uint8_t *mme_name_enc = TLVP_VAL_MINLEN(tp, SGSAP_IE_MME_NAME, SGS_MME_NAME_LEN); > so is the TLV value actually expected to be at least SGS_MME_NAME_LEN? Is > that correct? In recent versions 3GPP TS 29.118, section 9.4.13 only says that it is an FQDN with no further info. So the length could be the maximum length of an FQDN. However in the V13.5.0 version of that spec they explicitly state that the FQDN has an exact length of 55 bytes. Then the line would be correct, but some doubt on my side remains. https://gerrit.osmocom.org/c/osmo-msc/+/43229/comment/9056c000_6100e837?usp=email : PS2, Line 173: if (TLVP_LEN(tp, SGSAP_IE_MME_NAME) > mme_name_len - 1) > "if (TLVP_LEN(tp, SGSAP_IE_MME_NAME) >= mme_name_len)" looks a lot easier to > read to me but fine :) I like the "- 1" notation more since it tells me that the comparison is related to the handling of the \0 char. https://gerrit.osmocom.org/c/osmo-msc/+/43229/comment/4fb6ba3d_5428ed32?usp=email : PS2, Line 179: return 0; > I'd store TLVP_LEN(tp, SGSAP_IE_MME_NAME) in some len and add an extra > mme_name[len] = '\0' here so […] Done https://gerrit.osmocom.org/c/osmo-msc/+/43229/comment/b43ddcfe_9dc95932?usp=email : PS2, Line 183: osmo_apn_to_str(mme_name, TLVP_VAL(tp, SGSAP_IE_MME_NAME), TLVP_LEN(tp, SGSAP_IE_MME_NAME)); > isn't "TLVP_VAL(tp, SGSAP_IE_MME_NAME)" here just "mme_name_enc" ? I think you are right. -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/43229?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: I9e845ad1568cb3a88c90f81655c30cac862f83ec Gerrit-Change-Number: 43229 Gerrit-PatchSet: 2 Gerrit-Owner: dexter <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Comment-Date: Thu, 13 Aug 2026 17:24:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <[email protected]>
