Attention is currently required from: laforge, pespin, dexter.

Patch set 5:Code-Review +1

View Change

5 comments:

• Patchset:

  • Patch Set #5:

    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:

  • Patch Set #4, 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?

  • Patch Set #4, 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:

  • Patch Set #5, 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.

  • Patch Set #5, 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 change 29033. To unsubscribe, or for help writing mail filters, visit settings.