Attention is currently required from: laforge, pespin, dexter. Christian Amsüss has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/29033 ) Change subject: Add new pySim.ota library, implement SIM OTA crypto ...................................................................... Patch Set 5: Code-Review+1 (5 comments) Patchset: PS5: Looks good to me, with some comments. I have yet to run the encryption and integrity protection against my current reference implementation; once that passes, I'll update to +2. File pySim/ota.py: https://gerrit.osmocom.org/c/pysim/+/29033/comment/ddba9d13_d60daa0d PS4, Line 44: S 102 225 Table 5 : ota_status_codes = bidict({ : 0x00: 'PoR OK', : 0x01: 'RC/CC/DS failed', : 0x02: 'CNTR low', : 0x03: 'CNTR high', : 0x04: 'CNTR blocked', : 0x05: 'Ciphering error', : 0x06: 'Unidentified security error', : 0x07: 'Insufficient memory', : 0x08: 'more time', : 0x09: 'TAR unknown', : 0x0a: 'Insufficient security level', : 0x0b: 'Actual Response in SMS-SUBMIT', # 31.115 : 0x0c: 'Actual Response in USSD', # 31.115 : }) This bidict is redundant with ResponseStatus, and not used anywhere. Maybe left over from earlier revisions? https://gerrit.osmocom.org/c/pysim/+/29033/comment/00b05de6_921f8369 PS4, Line 120: algo_auth: str, kid_idx: int, kid: bytes, cntr: int = 0): I'm very suspicious of the counter having zero as a default; the symmetric algorithm is already initialized with a zero IV, the CNTR and some of the other early bytes kind of take its place after the first round of encryption. I'm not an expert there, but unless someone who knows all the involved algorithms well tells me that this kind of nonce reuse is OK, I'd prefer APIs that guide users towards explicit monotonous CNTR values. File pySim/sms.py: https://gerrit.osmocom.org/c/pysim/+/29033/comment/c397e2d1_9844a98e PS5, Line 30: ie_c = Struct('offset'/Tell, 'iei'/Int8ub, 'length'/Int8ub, 'data'/Bytes(this.length)) There is an updated version of this in your patch series, which is easier to understand and removes the FIXME. https://gerrit.osmocom.org/c/pysim/+/29033/comment/b33ba117_4cde9e79 PS5, Line 39: def __str__(self) -> str: I think this would be more useful as `__repr__`, especially as it does produce an expression that'd recreate the object (ref: <https://docs.python.org/3/reference/datamodel.html#object.__repr__>). The %s could become %r but likely makes no difference for the typical list of ies. -- To view, visit https://gerrit.osmocom.org/c/pysim/+/29033 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I193ff4712c8503279c017b4b1324f0c3d38b9f84 Gerrit-Change-Number: 29033 Gerrit-PatchSet: 5 Gerrit-Owner: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Reviewer: Christian Amsüss &lt;chrysn(a)fsfe.org&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter &lt;pmaier(a)sysmocom.de&gt; Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: dexter &lt;pmaier(a)sysmocom.de&gt; Gerrit-Comment-Date: Fri, 19 Aug 2022 15:41:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment