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

View Change

4 comments:

• 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
    })
    

    yes, that can be removed. […]

    Ack

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

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

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

To view, visit change 29033. To unsubscribe, or for help writing mail filters, visit settings.