Attention is currently required from: pespin, Christian Amsüss, dexter.

View Change

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

    This bidict is redundant with ResponseStatus, and not used anywhere. […]

    yes, that can be removed. It goes back to some very old first attempts of implementing this months if not years ago.

  • 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 init […]

    I understand the point. the rationale here was that in 99% of all current use cases we won't be using the counter, as we don't have any central database/backend where we would store the current per-card OTA counter. Also, 99% of the sysmoISIM-SJA2 users (which are probably >95% of our overall user base for this feature) have cards with a MSL not requiring counters, as they are used in a research/development setting and if you have multiple different programs talking OTA over time without any centralized counter storage...

• File pySim/ota.py:

  • Patch Set #5, Line 347: apdu += otak.crypt._get_padding(len_cipher, otak.crypt.blocksize)

    This should also set pad_cnt. […]

    your patch was cherry-picked + squashed, thanks.

  • Patch Set #5, Line 402: def decode_resp(self, otak: OtaKeyset, spi: dict, data: bytes) -> bytes:

    `-> bytes` is currently inaccurate, should be `CompactRemoteResp` (but see below)

    Done

  • Patch Set #5, Line 416: res = self.SmsResponsePacket.parse(remainder)

    Do we trust this parsing step enought, to […]

    I'm not sure I understand you here. Why would it be bad if that call would raise some exception, which in turn propagetes further upt to the caller of decode_resp() ?

  • Patch Set #5, Line 443: cc = otak.auth.check_sig(temp_data, res['cc_rc'])

    Needless assign; check_sig is merely called for the exception it'd raise.

    ack, fixing it in my next version.

  • Patch Set #5, Line 452: return dec

    There will need to be an API change later here, as in cases of `res. […]

    Done

• 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 […]

    updated version pushed.

  • 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 […]

    squashed your related patch into my patch

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