Attention is currently required from: Hoernchen.
dexter has posted comments on this change by Hoernchen. ( https://gerrit.osmocom.org/c/pysim/+/43546?usp=email )
Change subject: ota: indefinite length en/decoding support ......................................................................
Patch Set 2: Code-Review+1
(13 comments)
Patchset:
PS2: I have now carefully looked through and checked against the spec as much as possible. I think the logic and everything is fine. Most of the comments are of cosmetic nature. With one thing I am not sure: The while loop in the verifier of the indefinite responses may have problems when the end marker is damaged.
File pySim/ota.py:
https://gerrit.osmocom.org/c/pysim/+/43546/comment/4bb7e1ce_a47e4d96?usp=ema... : PS2, Line 134: 'commands'/Prefixed(BerTlvLen, ExpandedCmdItems)) We don't need such a definition for the indefinite length coding as well?
A direct spec ref for each of those construct definitions would be helpful. For this one: ETSI TS 102 226, table 5.2
https://gerrit.osmocom.org/c/pysim/+/43546/comment/07ce3f45_9a3839b9?usp=ema... : PS2, Line 169: # - starts with the "Number of executed command TLV objects" (table 5.10/5.13/5.15) I would recommend to put a spec ref on top of this:
``` # TS 102 226 5.2.2
```
https://gerrit.osmocom.org/c/pysim/+/43546/comment/2d0f8214_cc11bfa2?usp=ema... : PS2, Line 172: ExpandedRemoteResp = Struct('_tag'/Const(b'\xab'), 0xAB is the tag for definite length coding. For indefinite length coding the tag is 0xAF. Don't we need a second construct definition for the indefinite variant as well?
https://gerrit.osmocom.org/c/pysim/+/43546/comment/ed653762_1acafccd?usp=ema... : PS2, Line 185: """Parse the definite BER-TLV len, return (length_value, number_of_length_octets) """ maybe you can use bertlv_parse_len() from pyosmocom here? However, it returns the length value and the remainder. That is not exactly what you need, but maybe you can so some len() math to deduct the number of bytes that the length fields had.
https://gerrit.osmocom.org/c/pysim/+/43546/comment/59b17c30_2890d7a2?usp=ema... : PS2, Line 196: "<tag> 80 <content> 00 00" """ maybe add a hint like "(works for commands and response)" It was first wondering a bit why you put the tag as a parameter. But I guess you want to keep this code path flexible for future use.
https://gerrit.osmocom.org/c/pysim/+/43546/comment/8f3c2e7d_82fa1448?usp=ema... : PS2, Line 198: raise ValueError('expected indefinite-length template tag 0x%02x' % tag) I would strongly recommend to split this into two distinct checks. If you get a short template, but with a correct tag, this check would also fire.
https://gerrit.osmocom.org/c/pysim/+/43546/comment/ca80393a_2cf972ea?usp=ema... : PS2, Line 204: if data[off:off+2] != b'\x00\x00': Is this safe? What if you get a string where everything is valid but the last byte is chopped off for some reason.
https://gerrit.osmocom.org/c/pysim/+/43546/comment/1eaf1c47_0e0d54f8?usp=ema... : PS2, Line 207: length, len_octets = _read_ber_tlv_len(data, off + 1) As it seems you are not only interested in the octets, you are also putting much effort into verifying that the data structure is correct. Maybe then also check if the C/R-APDU tag is correct. But this then likely requires another parameter or a str parameter that distinguishes between command/response.
C-APDU tag: (0x22, ETSI TS 101 220 table 7.19) R-APDU tag: (0x23, ETSI TS 101 220 table 7.20)
https://gerrit.osmocom.org/c/pysim/+/43546/comment/831bed04_5956132b?usp=ema... : PS2, Line 232: return b'\xae\x80' + ExpandedCmdItems.build(commands) + b'\x00\x00' Ah, now I see why there is no explicit struct for the indefinite command scripting template. Is it really impossible to writ this as a construct struct? If not I think defining a construct struct would be much cleaner.
File tests/unittests/test_ota.py:
https://gerrit.osmocom.org/c/pysim/+/43546/comment/239accac_ba4c7c7f?usp=ema... : PS2, Line 470: 'ae80220700a4000c023f00220700a4000c022fe2220500b000000a0000') maybe add a testcase like this one, but with one byte chopped off at the end? (See my other comment)
https://gerrit.osmocom.org/c/pysim/+/43546/comment/9f6e6821_39dc37cb?usp=ema... : PS2, Line 486: 'af8023029000230290002' '30c988812010000408608149000' '0000')) Looks like you have chopped the string wrongly. The tag of the second response (0x23) is cut in the middle. (has no effect on the test result)
Maybe chop like this: 'af80' '23029000' '23029000' '230c988812010000408608149000' '0000' ?
https://gerrit.osmocom.org/c/pysim/+/43546/comment/6104c430_103543ea?usp=ema... : PS2, Line 498: 'c00401408510c00403018810c00402018810c004010188109000' '0000') maybe separate the 'af80'? You also have the end marker separate.