P1sec / pycrate

A Python library to ease the development of encoders and decoders for various protocols and file formats; contains ASN.1 and CSN.1 compilers.
GNU Lesser General Public License v2.1
381 stars 132 forks source link

Cannot decode range 256 format BA List #151

Closed jewalt closed 2 years ago

jewalt commented 2 years ago

When attempting to decode a GSM SIB2, the BA list is not decoded with the following error: BCCHFreqList, _from_char: unable to decode IE, NeighbourCellChan

I can step through the code and see that it is failing with:

/home/jewalt/.local/lib/python2.7/site-packages/pycrate_mobile/TS44018_IE.py(197)_from_char()->None -> ccur, wbl = char._cur, self._Layout[i-1] (Pdb) s IndexError: 'tuple index out of range'

What appears to be happening is since there is 1 spare byte at the end of the list of channels, the test: if char.len_bit - ccur >=wbl: is not hit and the next loop causes a failure since now i is out of range for the _Layout array.

A possible solution would be to change the while true: line to while i <= len(self._Layout):

I expect this will fail on a Range128 list as well.

mitshell commented 2 years ago

Thanks for your analysis : much appreciated. I have seen your patch too. Before merging, I was hoping if you can share a SIB2 buffer exhibiting the issue : could you kindly provide this ? So I can figure it out by myself, and integrate it into the unit tests.

jewalt commented 2 years ago

Sorry, I was going to include that and then forgot. Here is the SIB2 that I have been testing with:

59061A9F32600000000000000001C000000000FFB90000

The neighbor list should decode to (from tshark): Neighbour Cell Description - BCCH Frequency List EXT-IND: The information element carries the complete BA (0) BA-IND: 1 Format Identifier: 256 range (0x45) Range 256 format ORIG-ARFCN: 612 W(1): 1 W(2): 70 W(3): 72 W(4): 16 W(5): 63 W(6): 0 W(7): 0 W(8): 0 W(9): 0 W(10): 0 W(11): 0 W(12): 0 W(13): 0 W(14): 0 W(15): 0 W(16): 0 W(17): 0 W(18): 0 W(19): 0 W(20): 0 W(21): 0 List of ARFCNs = 612 613 684 685 762 810

I'm seeing the same, minus the list of ARFCNs with the patched code (I need to figure out if pycrate can do the ARFCN conversion as well). I also have to admit, I did not run the unit tests on my patch. I will do that today to make sure I didn't break anything.

p1-bmu commented 2 years ago

For the unit test, this is just a matter of adding the buffer here: https://github.com/P1sec/pycrate/blob/7685d58dcd353ce0fa79b2d7278a64b722c97d88/test/test_gsmrr.py#L57

p1-bmu commented 2 years ago

Regarding the retrieval of the list of ARFCN, you can do this:

In [4]: m, e = parse_NAS_MT(unhexlify('59061A9F32600000000000000001C000000000FFB90000'), wl2=True)

In [5]: e # error, if 0: no error
Out[5]: 0

In [6]: show(m)
### RRSystemInfo2 ###
 ### L2PseudoLength ###
  <Value : 22>
  <M : 0>
  <EL : 1>
 ### RRHeader ###
  <SkipInd : 0>
  <ProtDisc : 6 (RRM)>
  <Type : 26 (SYSTEM INFORMATION TYPE 2)>
 ### BCCHFreqList ###
  ### NeighbourCellChan ###
   <Fmt : 2>
   <ExtInd : 0 (complete BA)>
   <BAInd : 1 (BA(SACCH))>
   ### Alt : 2 -> CellChanAlt1 ###
    <FmtExt : 1>
    ### Alt : 1 -> CellChanAlt2 ###
     <FmtExt2 : 3 (variable bit map)>
     <OriginARFCN : 612>
     ### Alt : 3 ###
      <CellChanBitmapVar : 0xc000000000000000038000000000>
 ### NCCPermitted ###
  <NCCPermitted : 0xff>
 ### RACHCtrl ###
  ### RACHCtrl ###
   <MaxRetrans : 2 (4 retransmissions max)>
   <TxInt : 14 (32 slots)>
   <CELL_BARR_ACCESS : 0 (cell not barred)>
   <CallReestab : 1 (not allowed)>
   <AccessCtrlClass : 0x0000>

In [7]: m['BCCHFreqList']['NeighbourCellChan'].decode()
Out[7]: [613, 614, 683, 684, 685]

This is however not thoroughly tested, and I can see wireshark indicates something different here... The decoding routine should be this one: https://github.com/P1sec/pycrate/blob/7685d58dcd353ce0fa79b2d7278a64b722c97d88/pycrate_mobile/TS44018_IE.py#L348, but I do not have the time currently to deep dive back in the TS 44.018. Sorry

jewalt commented 2 years ago

No worries. I'll look into it and see why they are different. I'll add the SIB2 string to the rr_pdu_l2_mt unit test in the test and push up the change. Thank you for the pointer to the decoding routine.

jewalt commented 2 years ago

O man, I screwed up and sent a tshark decode to a different SIB2. Here is the one that should match the tshark decode. 59061A9B3200C69087E0000000000000000000FFB90000

jewalt commented 2 years ago

It still decodes incorrectly but it appears that range256 frequency lists have a very convoluted computation. It is listed in 10.5.2.13.5. I will spend a bit of time on it and see if I can implement the calculation.

jewalt commented 2 years ago

Sorry one more comment for a bit :-) When I add the SIB2 with range256 frequency list to the unit test, the unit test doesn't outright fail. I get this for the output:

...
[+] test_mobile total time: 6.0980
[+] GSM RR MO decoding and re-encoding
test_gsmrr_mo: 0.1176
[+] GSM RR MT decoding and re-encoding
test_gsmrr_mt: 0.1272
[+] GSM RR L2 MT decoding and re-encoding
BCCHFreqList, _from_char: unable to decode IE, NeighbourCellChan
BCCHFreqList, _from_char: unable to decode IE, NeighbourCellChan
BCCHFreqList, _from_char: unable to decode IE, NeighbourCellChan
BCCHFreqList, _from_char: unable to decode IE, NeighbourCellChan
BCCHFreqList, _from_char: unable to decode IE, NeighbourCellChan
BCCHFreqList, _from_char: unable to decode IE, NeighbourCellChan
BCCHFreqList, _from_char: unable to decode IE, NeighbourCellChan
BCCHFreqList, _from_char: unable to decode IE, NeighbourCellChan
test_gsmrr_l2_mt: 0.1424
[+] test_gsmrr total time: 0.3872
[<<<>>>] test_perf_all total time: 22.4072
[+] total time: 26.672379 sec

I would have expected a failure. Is simply seeing the unable to decode IE sufficient to indicate a failure?

p1-bmu commented 2 years ago

Use python -m unittest test.test_pycrate to launch the unit-test. Here, it seems the object BCCHFreqList has some custom exception handling to not raise even if it fails to parse a buffer.

jewalt commented 2 years ago

Based on the pseudo code in Table 10.5.2.13.5.1 in TS 44.018, the append at the end of https://github.com/P1sec/pycrate/blob/7685d58dcd353ce0fa79b2d7278a64b722c97d88/pycrate_mobile/TS44018_IE.py#L241

should add the original ARFCN (W(0)) mod 1024. (from TS44.018)

J := GREATEST_POWER_OF_2_LESSER_OR_EQUAL_TO(INDEX);
 N := W(INDEX);
 while INDEX>1 loop
 if 2*INDEX < 3*J then -- left child
 INDEX := INDEX - J/2;
 N := (N + W(INDEX) + 256/J - 2) mod (512/J - 1) + 1;
 else -- right child
 INDEX := INDEX - J;
 N := (N + W(INDEX) + 512/J - 2) mod (512/J - 1) + 1;
 end if;
 J := J/2;
 end loop;
 F(K) := (W(0) + N) mod 1024;

The original ARFCN is not part of the CellChanRange256. Can you suggest how we could access the original ARFCN? It could be a W_0 entry in CellChanRange256 but that could break other uses of CellChanRange256.

jewalt commented 2 years ago

Ok, I found what I think is a bug.
https://github.com/P1sec/pycrate/blob/7685d58dcd353ce0fa79b2d7278a64b722c97d88/pycrate_mobile/TS44018_IE.py#L290

I believe this should be a test for 1 or 3. 3 is variable and 1 is range256, both require the addition piece. In my previous comment I missed the addition here.

jewalt commented 2 years ago

Also found a bug in the frequency calculation function. The index should be updated before the calculation, not after.

p1-bmu commented 2 years ago

I just tested your changes. Now if I call .decode() on the NeighbourCellChan element, I get the following list of ARFCN from your test buffer: [613, 684, 685, 762, 810]. Does it mean wireshark is wrong in its decoding ? Or that we are still missing something ?

jewalt commented 2 years ago

No, I am pretty sure it matches Wireshark if you add in the original ARFCN of 612. The original ARFCN is not part of the neighbour list, but wireshark seems to add it.

p1-bmu commented 2 years ago

From TS 44.018, section 10.5.2.1b, for range 128, 256 and 512, it says:

ORIG-ARFCN, origin ARFCN (octet 2, 3 and 4)
This field encodes the ARFCN of one frequency belonging to the set. This value is also used to decode the rest of the element.

So I suppose the OriginARFCN value should be part of the list returned by the decode() method. Don't you think ?

jewalt commented 2 years ago

Yes, I think you are correct. I am on holiday for a few days so I can work on adding that when I get back. If you have a solution, please don't wait on me. I thought about this yesterday but since the originalARCN was not part of the frequency list as W0 it was a bit more than I wanted to modify as my first commit to this repo.

p1-bmu commented 2 years ago

OK, I fixed this last stuff myself and merged together with your PR into master. Thanks for your work.

jewalt commented 2 years ago

I know this issue is closed, but I wanted to ask why the special case to not include the original ARFCN in the frequency list for a variable list type was made? It isn't clear from the spec as to if it should be included or not.

p1-bmu commented 2 years ago

Do you mean here: https://github.com/P1sec/pycrate/blob/11c791572c6c2bfdc06f0b77c36f601a118268f0/pycrate_mobile/TS44018_IE.py#L310 ?

jewalt commented 2 years ago

Yes. Looking at the decode from tshark, the original ARFCN is included in the frequency list

p1-bmu commented 2 years ago

Yes, reading 44.018, section 10.5.2.13.7, it says "This field [ORIG-ARFCN] encodes the ARFCN of one frequency belonging to the set." So I suppose we should have it part of the list, too. I'll patch this soon. Good catch.

jewalt commented 2 years ago

Thank you for being so responsive in this!

jewalt commented 2 years ago

One final question... Do you have a 0.5.4 release planned anytime soon?

p1-bmu commented 2 years ago

I guess I'll tag one before the end of the year.