Max has posted comments on this change. ( https://gerrit.osmocom.org/12227 )
Change subject: ACL: integrate sanitize check into sgsn_acl_* functions ...................................................................... Patch Set 5: > Patch Set 5: > I did, maybe it was not clear enough: "I think sanitizing the imsi should be > done by caller of sgsn_acl_* based on where the information come from (from > the wire or from known sanitized source)." > Hmm, I was sure I've addressed it already. Anyway, the "where the information come from" is not applicable because it only comes from a single source which is not trusted so we always sanitize it. > If you know your data is sane there's no need to re-sanitize it. That's not our case. > You should expect the caller of a data struct to provide sane data instead of > internally sanitizing it and storing different data from what was provided. Sorry, you've lost me with "caller of a data struct" - what do you mean by that? > It's responsibility of the caller (vty code for instance) to make sure parse > of human input is correctly parsed and sanitized. I disagree, and the code I've looked over seems to disagree as well. For example, in osmo_bsc_vty.c: * osmo_mcc_from_str() sanitize data internally * gsm_parse_reg() regexp compiled and result checked outside of vty In general, I don't see any non-trivial checks done inside vty which I think is the right thing. What would be the advantage of having this check in separate file instead of static function in the same file? We can also move it to libosmocore but I don't see any benefits from keeping it in vty. Do you? > You can do checks inside the data structure if you want (I wouldn't), but I'd > avoid changing content of the data being handled in there. You mean inside function? The rest would be addressed in a next revision. -- 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 <msur...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <msur...@sysmocom.de> Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Pau Espin Pedrol <pes...@sysmocom.de> Gerrit-CC: Stefan Sperling <s...@stsp.name> Gerrit-Comment-Date: Thu, 13 Dec 2018 10:49:18 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No
