Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12227 )
Change subject: ACL: integrate sanitize check into sgsn_acl_* functions ...................................................................... Patch Set 5: Code-Review-1 (3 comments) as in last patch, enlighten us why we bother with ACLs? https://gerrit.osmocom.org/#/c/12227/5/src/gprs/sgsn_auth.c File src/gprs/sgsn_auth.c: https://gerrit.osmocom.org/#/c/12227/5/src/gprs/sgsn_auth.c@49 PS5, Line 49: * \returns true if IMSI would require truncation, false otherwise what do you mean, truncation -- if it is too long, then it is invalid. Return false if it is invalid, true if it is valid. Pau did mention the weird return value before. https://gerrit.osmocom.org/#/c/12227/5/src/gprs/sgsn_auth.c@60 PS5, Line 60: osmo_strlcpy(dst + GSM23003_IMSI_MAX_DIGITS - len, imsi, dst_len - (GSM23003_IMSI_MAX_DIGITS - len)); this won't work if dst_len is too small, because the osmo_strlcpy() siz arg is a size_t, which is unsigned. You need to check dst_len bounds avoiding negative number space. https://gerrit.osmocom.org/#/c/12227/5/src/gprs/sgsn_vty.c File src/gprs/sgsn_vty.c: https://gerrit.osmocom.org/#/c/12227/5/src/gprs/sgsn_vty.c@a651 PS5, Line 651: (ok, this was technically also wrong, but ensured the size because the buf was declared just above it.) -- To view, visit https://gerrit.osmocom.org/12227 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic3dff108148683b107e9edac430a0475283580e9 Gerrit-Change-Number: 12227 Gerrit-PatchSet: 5 Gerrit-Owner: Max <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <[email protected]> Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-CC: Stefan Sperling <[email protected]> Gerrit-Comment-Date: Tue, 11 Dec 2018 12:50:06 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
