Bugzilla – Bug 1318
Ipv6L3Protocol::LocalDeliver can get stuck in an infinite loop trying to decode a malformed packet
Last modified: 2013-03-14 16:22:16 UTC
Created attachment 1284 [details] test program The attached test program (provided by Eric) shows the bug. The packet sent is zero-filled, and it's a Ipv6RawSocket, so basically a clean, bare-bone IPv6 packet with a zero-filled payload. Nothing bad with it. Right? Wrong. Ipv6L3Protocol::LocalDeliver goes nuts. 1) it doesn't find any L4 protocol (ofc). 2) it tries to decode a ghost IPV6_EXT_HOP_BY_HOP, as nextHeader is zero (there's NO next header at all!) Right now using ns3::Ipv6RawSocket from an application is equivalent to shooting in a barrel full of bugs: you'll hit one. Problem: Ipv6L3Protocol::LocalDeliver is doing **exactly** what is supposed to do (minus one thing, see later on). THE problem is: IPv6 is not keen on having no L4 information in the IPv6 header, and Ipv6RawSocket does not set anything. So the packet is not well-formed. The correct procedure is: if you wanna use Ipv6RawSocket from anything but low (L4) layers, you should: 1) set an L4 protocol number, even a temporary one, not conflicting with the other ones already defined, 2) register your protocol number in the L4demux (by deriving it from Ipv4L4Protocol) and so on. 3) etc. Basically you have to define a L4 protocol, or things go bad. Very bad. Now, where is the bug? The bug is that Ipv6RawSocket does exist but it shouldn't be used at all. By anyone. Unless they do know what they're doing AND they're setting up a new L4 protocol. So where's the bug? In the documentation. A bug is a feature once it's documented. T. Oh, forgot the "minus one thing". This is a real one. We do expect that the extension we're reading is the right length. No check is done, not even in debug mode, that the buffer have enough bytes to hold the option we're reading. If, like in this case, the packet is not well formed, ns-3 will happily keep decoding the packet. Forever... And this is the real bug. But I don't know how to fix it.
More data on this bug. http://sock-raw.org/papers/sock_raw and http://www.iana.org/assignments/protocol-numbers/protocol-numbers.xml#Internet_Assigned_Numbers_Authority are relevant to this. It seems that Linux is using 255 as IPPROTO_RAW value, as is as a "dummy" protocol number. On the other hand it's an IANA-reserved number. Using 255 as proto number should fix the issues I found in raw IPv6 packets (and maybe should check also raw IPv4 sockets). On the other hand, we must take care not to break the actual stuff that does use raw sockets, so I'll not push for this change 'til we're totally sure of its effects. Moreover, it seems that we're missing an API from the Raw socket interface, as is the one to set up the L4 protocol number. At least linux and FreeBSD do allow to set it, see: socket(AF_INET, SOCK_STREAM, xxx); where xxx can be 0, IPPROTO_RAW, or any other value. In case of a 0, however, IP protocol should be still set to anything but 0 (as I said, 0 is HOPOPT, see the IANA numbers). If anyone cares to do a little wireshark + Linux testing a confirmation would be nice. Moreover, a little more documentation would be good in order to clarify the correct raw socket use case. Tommaso
For the first part of the issue, my proposal is to add this to BOTH the ns3::Ipv6RawSocketFactory and ns3::Ipv6RawSocketImpl class description: A RAW Socket typically is used to access specific IP layers not usually available through L4 sockets, e.g., ICMP. The implementer should take particular care to define the Ipv6RawSocketImpl Attributes, and in particular the Protocol attribute. Not settin it will result in a zero protocol at IP level (corresponding to the HopByHop Extension Header) when sending data through the socket, which is probably not the intended behaviour. A correct example is (from src/applications/model/radvd.cc): if (!m_socket) { TypeId tid = TypeId::LookupByName ("ns3::Ipv6RawSocketFactory"); m_socket = Socket::CreateSocket (GetNode (), tid); NS_ASSERT (m_socket); m_socket->SetAttribute ("Protocol", UintegerValue (Ipv6Header::IPV6_ICMPV6)); m_socket->SetRecvCallback (MakeCallback (&Radvd::HandleRead, this)); }
(In reply to comment #2) > For the first part of the issue, my proposal is to add this to BOTH the > ns3::Ipv6RawSocketFactory and ns3::Ipv6RawSocketImpl class description: > > > A RAW Socket typically is used to access specific IP layers not usually > available through L4 > sockets, e.g., ICMP. The implementer should take particular care to define the > Ipv6RawSocketImpl Attributes, and in particular the Protocol attribute. > > Not settin it will result in a zero protocol at IP level (corresponding to the > HopByHop Extension > Header) when sending data through the socket, which is probably not the > intended behaviour. > > A correct example is (from src/applications/model/radvd.cc): > if (!m_socket) > { > TypeId tid = TypeId::LookupByName ("ns3::Ipv6RawSocketFactory"); > m_socket = Socket::CreateSocket (GetNode (), tid); > > NS_ASSERT (m_socket); > > m_socket->SetAttribute ("Protocol", UintegerValue > (Ipv6Header::IPV6_ICMPV6)); > m_socket->SetRecvCallback (MakeCallback (&Radvd::HandleRead, this)); > } I agree with the documentation, perhaps with the clarification that HopByHop pertains to the IPv6 HopByHop option. Also, suggest to say "and in particular, the Protocol attribute, which also controls the Next Header field in IPv6". I also agree that we need to sanity check the option before parsing on the receive side.
Changeset 7650:46bfff3026b7 Added the proposed documentation enhancements for Ipv6RawSockets. Leaving the bug open and changing its description to reflect the remaining bug. T.
Created attachment 1285 [details] Checks to avoid infinite loops
I added some checks to halt ns-3 in case there is a zero-length option header (must never happen). This should definitely fix the infinite loop that was, ultimately, caused by ipv6Extension->Process() not advancing in the buffer. T.
Created attachment 1286 [details] Checks to avoid infinite loops (fixed)
Back from the holidays, back at work, back at bugs. Can I have a quick review on this little set of asserts so to close this bug for good ? Cheers, T.
(In reply to comment #8) > Back from the holidays, back at work, back at bugs. > > Can I have a quick review on this little set of asserts so to close this bug > for good ? > > Cheers, > > T. Yes, looks good to me (add space after MSG and before parentheses)
Fixed with changeset 7680:722145d57c8a
(In reply to comment #7) > Created attachment 1286 [details] > Checks to avoid infinite loops (fixed) (sorry to mention to old post) This additional assert gives another effect: # more specifically, the following assert. http://code.nsnam.org/ns-3-dev/file/tip/src/internet/model/ipv6-l3-protocol.cc#l1004 IPv6 Router Alert Option (RFC2711), used by MLD (or RSVP), generates zero-length field. This is possible by definition (RFC2460, Section 4.3), and Linux (2.6.36) is implemented this. RFC2460, Section 4.3. Hdr Ext Len 8-bit unsigned integer. Length of the Hop-by- Hop Options header in 8-octet units, not including the first 8 octets. Attached pcap is generated by dce-umip example at the CN node. http://code.nsnam.org/thehajime/ns-3-dce-umip/file/b5447406cb67/example/dce-umip-cmip6.cc When an attached link is up, linux generate MLD packet with Hop-by-hop header including length 0. I believe the following assert is enough to check malformed packet in this case, since "nextHeaderStep" will not return 0 even if hop-by-hop header has zero-length field. http://code.nsnam.org/ns-3-dev/file/tip/src/internet/model/ipv6-l3-protocol.cc#l1024
Created attachment 1530 [details] packet trace includes IPv6 Hop-by-Hop header with length field zero.
Created attachment 1533 [details] Bugfix The patch removes the check about the HopByHop extension length (it was wrong anyway) and it clarifies what's the check is for
(In reply to comment #13) > The patch removes the check about the HopByHop extension length (it was wrong > anyway) and it clarifies what's the check is for Thanks. MLD packet is also able to receive with this change. +1 for push.
fixed in changeset 9255:e407ed747d1c