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 7:

(4 comments)

File pySim/ota.py:

https://gerrit.osmocom.org/c/pysim/+/29033/comment/f6c2c89c_c57bb312 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 : })

yes, that can be removed. […]

Ack

https://gerrit.osmocom.org/c/pysim/+/29033/comment/397c5293_41284fbb PS4, Line 120: algo_auth: str, kid_idx: int, kid: bytes, cntr: int = 0): I still think we'd have a more secure-in-use API if we removed the default cntr and added a note in the docstring along the lines of:

Setting the cntr to a constant value (e.g. cntr=0) is only recommended in experimentation, testing (especially to obtain reproducible ciphertexts) and research. Whenever commands need to be secured against adversaries that could intercept the messages, the caller needs to ensure that the same counter value is not used twice for any key.

Thus, users in testing / research actively have to acknowledge that they do something that'd be dangerous in production, and users who ship this have an easier time finding the right place to adjust the demo code they find.

File pySim/ota.py:

https://gerrit.osmocom.org/c/pysim/+/29033/comment/66ee6149_01287e6b PS5, Line 416: res = self.SmsResponsePacket.parse(remainder)

I'm not sure I understand you here. […]

The bad part about an exception is that aborting would be unwarranted. If `spi['por_shall_be_ciphered']`, then decoding the remainder would yield garbage (only what's in the if-clause below, decoding the deciphered remainder yields good data).

Decoding is currently obviously rather lax, in that it even accepts the "garbage" data it currently decodes, but if that decode step got stricter, it'd be a toss-up whether the ciphertext happens to be a valid SmsResponsePacket (in which case the function can continue, and the garbage in `res` is never accessed) or it happens to be invalid (which would be no surprise), and an exception is raised about data that is neither expected to be usable nor actually used (as `res` would be overwritten with the correct result in the if-clause, if only execution got there).

File pySim/sms.py:

https://gerrit.osmocom.org/c/pysim/+/29033/comment/627e5a1d_91beacc0 PS5, Line 30: ie_c = Struct('offset'/Tell, 'iei'/Int8ub, 'length'/Int8ub, 'data'/Bytes(this.length))

updated version pushed.

I don't see that in change set 7 yet -- I was referring to the changes in I0d95e62c1e7183a7851d1fe38df0f5133830cb1f which look like:

``` class UserDataHeader: # a single IE in the user data header - ie_c = Struct('offset'/Tell, 'iei'/Int8ub, 'length'/Int8ub, 'data'/Bytes(this.length)) + ie_c = Struct('iei'/Int8ub, 'length'/Int8ub, 'value'/Bytes(this.length)) # parser for the full UDH: Length octet followed by sequence of IEs - _construct = Struct('udhl'/Int8ub, - # FIXME: somehow the below lambda is not working, we always only get the first IE? - 'ies'/RepeatUntil(lambda obj,lst,ctx: ctx._io.tell() > 1+this.udhl, ie_c)) + _construct = Struct('ies'/Prefixed(Int8ub, GreedyRange(ie_c)), + 'data'/GreedyBytes) ```