Bug 2505

IMO this patch is fine as far as it goes.  Stylistically I would prefer

  m_dataSize = optionalPayloadSize;
  m_data = new uint8_t[m_dataSize];

to more closely tie m_dataSize and m_data.  (Although I see it's done as in the patch throughout icmpv4.cc)

As an alternative, it's always struck me as odd that Buffer::Iterator doesn't provide a typical container iterator interface (see http://www.cplusplus.com/reference/stl/)

In this case, adding the obvious end(), and using the existing GetDistanceFrom() would be enough to solve the problem, without adding yet more unusual container API.

Thankfully I'm not the network maintainer, so I don't have to make these tough calls.

FYI, I see no other cleaner way to deal with this issue: the patch looks fine.

To answer Peter about the iterator design, I can't remember what I was thinking 8 or 10 years ago but I can try to guess:

1. a C++-style iterator would have required us to clearly separate the "formatting" (i.e., ReadU16 et al) API from the iteration API. 

2. I suspect that this would have been relatively difficult to do in an efficient way and with decent API

3. I also suspect that it would be probably a bit more work to produce a really stl-compliant iterator

To summarize, the java-style iterator we have here was probably easier to design and implement...

Created attachment 2594 [details]
new patch

It seems that the "problem" is more widespread.

Actually *any* call to GetSize in a Deserialize is at risk.

The problem is that if you deserialize a header (or trailer) normally, all is fine.
If you deserialize the header without removing the previous headers, then the buffer size is too large.
This is triggered by printing the headers/trailers without removing them, as in Packet::Print ().

Strange enough, this behaviour has been introduced recently (no idea when).

I forgot.

There are two remaining cases of GetSize used in a Deserialize, and I don't know if they're safe or not:

LteRlcAmHeader::Deserialize, line 572:
m_lastOffset = m_segmentOffset + start.GetSize () - m_headerLength;

WifiInformationElementVector::Deserialize, line 78:
uint32_t size = start.GetSize ();

In order to test if there's an issue, simply activate an AsciiTrace.

Note the "new patch" has bindings diffs as well, which probably shouldn't be applied.

(In reply to Peter Barnes from comment #5)
> Note the "new patch" has bindings diffs as well, which probably shouldn't be
> applied.

Why not ? There's a new member function. Thus, we need to refresh the bindings.

Yes refresh the bindings, but my preference is not in the same patch.  It clutters up the commit with extraneous junk.  I much prefer a separate "rescan bindings" commit following.

> 
> WifiInformationElementVector::Deserialize, line 78:
> uint32_t size = start.GetSize ();

The problem with this one is that this Header type doesn't know its size at Deserialize time.  It is a vector of various TLV fields, and may be arbitrary length.  In some contexts (Packet::Print), the buffer still has a WifiMacTrailer present.  In other contexts (mesh code performing a RemoveHeader), it no longer has the WifiMacTrailer present, because MacLow has already removed it.

I suspect that this patch's use of GetRemainingSize () in the various Deserialize methods is only working because in the various cases in this patch (e.g. RipHeader) the variable length header is the only header left in the buffer iterator when its Deserialize is called.

Is there any other place in the codebase where this issue has been deftly handled, without something like BufferIterator::GetRemainingSize ()?  The issue being that a Header type is variable length and can't determine its size at the point of deserialization?  It seems like two possible options to fix this are to forbid such Header types (i.e. refactor WifiInformationElementVector), or to provide some overloaded Deserialize() method that takes a length argument, where the header length is stored in the metadata, or is derived during the packet processing.

Sadly you're right.

I triple checked and the only reason it works is because using GetRemainingSize prevents a buffer overflow.
Nevertheless, the header is NOT correctly deserialized, i.e., variable length headers are read beyond their limit.

In the Rip[Ng] case there is a fortunate condition: the header variable part must be a multiple of 20 bytes, and the trailers are (usually) much shorter.
In the ICMP cases, the problem is still there, but it just affects the Packet::Print function.

I think that we have an urgent-to-fix problem, and it's the assert without this patch.
The next problem is to find a way to print correctly the Packet.

One option is to store the header / trailer length in the metadata, and have an overloaded Deserialize to be used in the Print. It would work, but it would also add a level of complexity.

Another option could be to store the *order* in which the header/trailers have been added.
In this way one could print them in the right order, deserializing them for real.
Pros: no need for a GetRemainingSize, probably.

The third option is to reverse the problem. First we should print the trailers (in reverse order, in a temporary buffer), then the other headers.
Without the trailers, the GetRemainingSize should work as intended. Unless, of course, one have TWO unknown length headers in a packet.

The bottom line is: we're classifying as "headers" a lot of stuff, even when they're not headers at all. ICMPs are messages, not headers. The same goes for RIP messages.
Their size is stored in the IP header, and we should use that info properly.

Anyway, my proposal is to push this as a temporary workaround and work on a serious fix after 3.26.


(In reply to Tom Henderson from comment #8)
> > 
> > WifiInformationElementVector::Deserialize, line 78:
> > uint32_t size = start.GetSize ();
> 
> The problem with this one is that this Header type doesn't know its size at
> Deserialize time.  It is a vector of various TLV fields, and may be
> arbitrary length.  In some contexts (Packet::Print), the buffer still has a
> WifiMacTrailer present.  In other contexts (mesh code performing a
> RemoveHeader), it no longer has the WifiMacTrailer present, because MacLow
> has already removed it.
> 
> I suspect that this patch's use of GetRemainingSize () in the various
> Deserialize methods is only working because in the various cases in this
> patch (e.g. RipHeader) the variable length header is the only header left in
> the buffer iterator when its Deserialize is called.
> 
> Is there any other place in the codebase where this issue has been deftly
> handled, without something like BufferIterator::GetRemainingSize ()?  The
> issue being that a Header type is variable length and can't determine its
> size at the point of deserialization?  It seems like two possible options to
> fix this are to forbid such Header types (i.e. refactor
> WifiInformationElementVector), or to provide some overloaded Deserialize()
> method that takes a length argument, where the header length is stored in
> the metadata, or is derived during the packet processing.

> 
> Anyway, my proposal is to push this as a temporary workaround and work on a
> serious fix after 3.26.

I agree with the proposal to patch this temporarily and leave open.  For WifiInformationElementVector, there aren't any example uses of Packet::Print with the mesh code at the moment.  It luckily works out in the RemoveHeader() use case since it is the last one deserialized, so I propose to leave that one as is, and leave the Packet::Print use case broken for now.

Pushed the patch in changeset:   12339:08179db4f1e0.

I classify it as a workaround, and a definitive patch should (and must) be found sooner than later.

Here are a couple of hints about what you could do long term to solve this problem for good:

1. in packet-metadata.cc lines 1112 and 1121, you could specify bounds on the buffer iterator because you _know_ the size of the block you are reading (item.currentSize I believe).

2. in buffer.h/cc, there is no API to create an iterator with specific range but I think it should not be too hard to add a private Buffer::Iterator::Iterator(Buffer const *buffer, uint32_t start, uint32_t end) as well as Buffer::BeginRange() that calls this constructor

3. if you do this, GetRemainingSize will work the same whether you are deserializing during packet pretty-printing or during normal simulation.

4. Note that, in general, I find this reliance on GetRemainingSize pretty yucky. A cleaner solution would probably involve a new Header/Trailer Deserialize method:

uint32_t Deserialize (Buffer::Iterator begin, Buffer::Iterator end)

With all current calls to Deserialize replaced by calls to this new Deserialize method (I did not check that it is actually feasible in all cases where Deserialize is called but I think that it should work). It should be possible to make the new Deserialize forward to the current Deserialize method by default to not force all users to port their code on a single flag day.

*** Bug 2728 has been marked as a duplicate of this bug. ***

Created attachment 2851 [details]
patch 1 (network module)

Created attachment 2852 [details]
patch 2 (mesh module)

Created attachment 2853 [details]
patch 3 (mesh example)

Created attachment 2854 [details]
patch 4 (bug in WifiInformationElement::Deserialize)

These patches are along the lines of Mathieu's earlier suggestion, and I believe are ready to commit (all tests pass).

Patches 1-4 plus documentation pushed on 27-Oct-2017 (changesets 13126-13130)