TCP: parse generic stable irregular: wrong discriminator handling

[Klaus Warnke]

Hello,

in d_tcp_parse_generic_irreg() the discriminator for
generic_stable_irregular is 0xff, not 0x01:

  // An item that can change, but currently is unchanged
  COMPRESSED generic_stable_irregular {
    discriminator =:= '11111111' [ 8 ];
    ENFORCE(option_static.UVALUE == 0);
  }

Additionally I propose the following implementation for
the handling:

@@ -1315,11 +1323,13 @@ static int d_tcp_parse_generic_irreg(const struct rohc_decomp_ctxt *const contex
   discriminator = data[0];
   read++;

- if(discriminator == 0x01)
+ if(discriminator == 0xff)
   {
- /* TODO: handle generic_stable_irregular() */
- rohc_decomp_warn(context, "unsupported generic_stable_irregular() encoding");
- goto error;
+ const size_t opt_load_len = persist->data.generic.load_len;
+
+ opt_ctxt->data.generic.load_len = opt_load_len;
+ memcpy(opt_ctxt->data.generic.load, persist->data.generic.load, opt_load_len);
+ rohc_decomp_debug(context, "TCP generic option payload = %zu bytes", opt_load_len);
   }
   else if(discriminator == 0x00)
   {
@@ -1339,6 +1349,14 @@ static int d_tcp_parse_generic_irreg(const struct rohc_decomp_ctxt *const contex
    read += opt_load_len;
    rohc_decomp_debug(context, "TCP generic option payload = %zu bytes", opt_load_len);
   }
+ else
+ {
+ rohc_decomp_warn(context, "malformed TCP irregular part: malformed "
+ "TCP option items: TCP generic irregular option "
+ "discriminator should be either 0x00 or 0xff, "
+ "but is %u", discriminator);
+ goto error;
+ }
  }

  return read;

These are the four RoHC packets to test. I'm using the pcap from the ipv4_tcp_afl11 test.

[DEBUG] [rohc_traces_internal.c:100 rohc_dump_buf()] compressed data, max 100 bytes (59 bytes):
[DEBUG] [rohc_traces_internal.c:109 rohc_dump_buf()] fd 06 c0 00 06 c0 a8 02 02 5b 79 32 85 be 63 00
[DEBUG] [rohc_traces_internal.c:109 rohc_dump_buf()] 50 06 00 40 45 35 30 02 00 01 b7 00 cb 35 39 08
[DEBUG] [rohc_traces_internal.c:109 rohc_dump_buf()] 50 d7 15 87 88 89 80 8a 7f 84 05 b4 25 82 14 8a
[DEBUG] [rohc_traces_internal.c:129 rohc_dump_buf()] 41 d6 8e 00 00 00 00 00 f0 83 07

[DEBUG] [rohc_traces_internal.c:100 rohc_dump_buf()] compressed data, max 100 bytes (32 bytes):
[DEBUG] [rohc_traces_internal.c:109 rohc_dump_buf()] fa 22 00 0c 92 15 87 88 89 00 81 80 04 05 b4 84
[DEBUG] [rohc_traces_internal.c:129 rohc_dump_buf()] 02 b1 0a 00 00 00 00 ff 7f 00 df 10 45 35 50 d7

[DEBUG] [rohc_traces_internal.c:100 rohc_dump_buf()] compressed data, max 100 bytes (32 bytes):
[DEBUG] [rohc_traces_internal.c:109 rohc_dump_buf()] fa 23 00 0c c7 15 87 88 89 00 83 ff 04 05 b4 25
[DEBUG] [rohc_traces_internal.c:129 rohc_dump_buf()] 02 14 0a 41 d6 8e 00 00 00 00 00 07 45 35 50 d7

[DEBUG] [rohc_traces_internal.c:100 rohc_dump_buf()] compressed data, max 100 bytes (31 bytes):
[DEBUG] [rohc_traces_internal.c:109 rohc_dump_buf()] fa 24 00 0c ef 15 07 88 89 00 8a 84 02 b1 0a 41
[DEBUG] [rohc_traces_internal.c:129 rohc_dump_buf()] d6 8e 00 00 00 00 00 0d 03 07 45 35 50 d7 ff

I attached a corrected version of the pcap, because the forth packet contains a wrong IP length (wrong 72 instead of correct 60).
With the wrong length, the crc checksum test fails, because the decompressor computes the correct length, but
the compressor uses the wrong to calculate, but everything was correct.

Best,
Klaus Warnke

[Didier Barvaux (didier-barvaux)]

Hello,

You're right. The discriminator value is wrong. I didn't detect it because the compressor doesn't support that encoding variant yet. Thank you for the complement of implementation :)

For the PCAP, I'm not sure to understand why you modified it. The packet was generated by a fuzzer (AFL), hence the incorrect IP checksum. The compressor shall therefore handle it with the Uncompressed profile. Using any other profile will result in CRC failures as you said.

Regards,
Didier

[Klaus Warnke (k-warnke)]

The problem is the value of the total length field in the IPv4 header of the 4th packet.
In the source.pcap from git it has the value 72, which is wrong, because
the packet is 60 bytes in size. The (my) compressor don't check this, but
it computes the crc3/7 header checksum over it.
The total length is not transmitted, because the decompressor
computes it. And here is the problem. The decompressor
computes a different value, because it uses the correct value 60 for the total length
field in the IPv4 header. And this results into a different crc3/crc7 value
for the header checksum. Then the decompressor detects and notifies a
decompression error, because the crc value does not match.
After I corrected it, everything works fine, because the IP checksum is
also correct now, for what reason ever.

Therefore it is important, that the header values, which are not
explicit transmitted, but computed by the decompressor, have a
correct (the same value as computed by the decompressor) to
avoid crc failures for non IR or IR-DYN packets. In IR/IR-DYN packets
the crc checksum is not computed over the uncompressed IP packet, but over
the IR/IR-DYN packet itself.

Best,
Klaus Warnke

[Didier Barvaux (didier-barvaux)]

Klaus,

OK, I see your point. However, in real-world networks, (un)intentional malformed packets may be encountered and the compressor shall cope with them. I believe that the compression/decompression process shall not alter the packet (even the malformed parts), ie. the compression/decompression process shall be fully transparent. The compressor shall therefore handle malformed packets by using the Uncompressed profile.

An additional note about the PCAP file: the AFL fuzzer generates many of malformed packets, the PCAP capture (and its well-formed or malformed packets) caused something to go wrong with either the compressor or the decompressor, so I saved it to avoid the problem to come back later.

Regards,
Didier

[Klaus Warnke (k-warnke)]

Didier,

yes this is correct.
The RFC says for the checksum, similar to length

6.4.1. inferred_ip_v4_header_checksum

   This encoding method implicitly assumes that the compressor will not
   process a corrupted header; otherwise, it cannot guarantee that the
   checksum as recomputed by the decompressor will be bitwise identical
   to its original value before compression.

I think, the compressor should drop such packets, because they
can't processed at all. Sending with uncompressed profile
makes no sense, because at the receiving site the IP
stack will drop that frame (I guess).

Best,
Klaus Warnke

[Didier Barvaux (didier-barvaux)]

Klaus,

I agree. The IP hop following the ROHC decompressor will probably drop the packets. So, using the Uncompressed profile or dropping the packet directly should give the same result at the end. However, it is easier to handle all unsupported packets (fragmented IP, malformed packets...) the same way, ie. the Uncompressed profile. That's why the ROHC library handle them that way.

Regards,
Didier

[Didier Barvaux (didier-barvaux)]

Fix pushed on master.