Attention is currently required from: osmith, daniel.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/erlang/osmo_dia2gsup/+/29163 )
Change subject: Release new version: 0.1.2
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo_dia2gsup/+/29163
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: erlang/osmo_dia2gsup
Gerrit-Branch: master
Gerrit-Change-Id: I2b71212009149ea46effecb08d96620cc67c944b
Gerrit-Change-Number: 29163
Gerrit-PatchSet: 1
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 19 Aug 2022 22:25:02 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
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:
(3 comments)
Patchset:
PS5:
I've verified this module's encryption and decryption against what SJA2 and one other SIM implementation.
Most comments are on an "it's not broken" level and could just as well be addressed later, but the PCNT should not get overlooked, so I'm leaving this at +1 until that is fixed to avoid accidental merging (if a +2 from me even carried the weight to make things mergable).
File pySim/ota.py:
https://gerrit.osmocom.org/c/pysim/+/29033/comment/2a3b12c8_25dcdd2e
PS5, Line 402: def decode_resp(self, otak: OtaKeyset, spi: dict, data: bytes) -> bytes:
`-> bytes` is currently inaccurate, should be `CompactRemoteResp` (but see below)
https://gerrit.osmocom.org/c/pysim/+/29033/comment/ce9cbba8_295545de
PS5, Line 452: return dec
There will need to be an API change later here, as in cases of `res.response_status == 'actual_response_sms_submit'`, we'll rather need the res (and the CompactRemoteResp parsing fails in these cases, currently) rather than the dec.
A concrete suggestion for a change is in I1c488a19ad75114255c8ff1c59d903388451b37a in the chrysn/for-29033 branch.
--
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 <laforge(a)osmocom.org>
Gerrit-Reviewer: Christian Amsüss <chrysn(a)fsfe.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 19 Aug 2022 20:50:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
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:
(3 comments)
File pySim/ota.py:
https://gerrit.osmocom.org/c/pysim/+/29033/comment/6ebf4152_00f338c4
PS5, Line 347: apdu += otak.crypt._get_padding(len_cipher, otak.crypt.blocksize)
This should also set pad_cnt. Setting PCNT=0 often works in practice because the last intentional command produces output so the padding zeros aren't even read, but still that's probably not intended.
I'm not sure what the right behavior is with Gerrit here -- I've prepared a fix in the branch chrysn/for-29033 (but didn't push it to refs/change/29033 as that might create a new patchset rather than a proposed patch set if that's even a thing here).
https://gerrit.osmocom.org/c/pysim/+/29033/comment/b3a60db4_7c7cfb9c
PS5, Line 416: res = self.SmsResponsePacket.parse(remainder)
Do we trust this parsing step enought, to
* not raise anything even when run on the encrypted data in the por_shall_be_cipherd case? (If not, it could go into an `else` branch of the next line.)
* to be run before the CC is evaluated? (It's not like we're doing *much* processing yet, but I have a weak personal preference to look at as little data from networks as possible before I've verified it's from a known somewhat-trusted peer.)
(If both are "yes", please just mark as resolved).
https://gerrit.osmocom.org/c/pysim/+/29033/comment/46ee5d21_fe577b61
PS5, 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.
--
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 <laforge(a)osmocom.org>
Gerrit-Reviewer: Christian Amsüss <chrysn(a)fsfe.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 19 Aug 2022 19:09:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
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 <laforge(a)osmocom.org>
Gerrit-Reviewer: Christian Amsüss <chrysn(a)fsfe.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 19 Aug 2022 15:41:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
fixeria has abandoned this change. ( https://gerrit.osmocom.org/c/erlang/osmo_dia2gsup/+/29160 )
Change subject: Fix handling of Re-Synchronization-Info AVP in AIR
......................................................................
Abandoned
You're welcome!
--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo_dia2gsup/+/29160
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: erlang/osmo_dia2gsup
Gerrit-Branch: master
Gerrit-Change-Id: Ie5eded2f5fb2de01f69d2a9c0e5d70283bf5cbf5
Gerrit-Change-Number: 29160
Gerrit-PatchSet: 3
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-MessageType: abandon
Attention is currently required from: laforge, fixeria.
daniel has posted comments on this change. ( https://gerrit.osmocom.org/c/erlang/osmo_dia2gsup/+/29160 )
Change subject: Fix handling of Re-Synchronization-Info AVP in AIR
......................................................................
Patch Set 3: Code-Review-1
(1 comment)
Patchset:
PS3:
I tried, but something was still off. https://gerrit.osmocom.org/c/erlang/osmo_dia2gsup/+/20021 did work though, so I merged that instead. In any case thanks for the help!
--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo_dia2gsup/+/29160
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: erlang/osmo_dia2gsup
Gerrit-Branch: master
Gerrit-Change-Id: Ie5eded2f5fb2de01f69d2a9c0e5d70283bf5cbf5
Gerrit-Change-Number: 29160
Gerrit-PatchSet: 3
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 19 Aug 2022 15:35:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment