Hello,
The next installment in the evaluation series. The full raw texts are
available at:
http://www.cs.helsinki.fi/u/amlaukka/ipv6/rfc2460-01.eval
http://www.cs.helsinki.fi/u/amlaukka/ipv6/draft-icmp-v3-00.eval
This is the shortened version. There are some minor points but
nothing really major, except for the lack of anycast support?
---
INTERNET-DRAFT A. Conta, Lucent
IPNG Working Group S. Deering, Cisco Systems
Internet Control Message Protocol (ICMPv6)
for the Internet Protocol Version 6 (IPv6)
Specification
<draft-ietf-ipngwg-icmp-v3-00.txt>
This document specifies a set of Internet Control Message Protocol
(ICMP) messages for use with version 6 of the Internet Protocol
(IPv6).
+++
This is a raw "code-review" type evaluation of the Linux IPv6 stack
implementation according to the requirements specified in the above
document.
Disclaimer:
o Passing this review does not mean the code is not buggy. Only that
it appears to pass the requirements in the respective RFCs. I've tried
to look for bugs in the process but of course can't guarantee anything.
o Also I can not guarantee that I misintepreted some requirement or
the code that was supposed to implement it.
o RFCs are not a substitute for design document - meaning they assume
a level that is above the implementation. Implementation can make
wrong decisions which are not simply covered in the RFCs.
Definition of markups:
Q: Question
A: Answer - to questions
E: Error - not compliant
N: Note or todo
O: Okay - compliant
+++
(a) If the message is a response to a message sent to one of the
node's unicast addresses, the Source Address of the reply must
be that same address.
---
O: if (ipv6_chk_addr(&hdr->daddr, skb->dev))
O: saddr = &hdr->daddr;
O:
N: ipv6_chk_addr() checks for the validity of the address (i.e. not tentative
N: and of correct scope). However it does not check for unicast address. Meaning
N: ipv6_add_addr() MUST never be given anycast or multicast addresses.
Q: Can this happen?
Q: I can't see much support for anycast addresses, is this correct?
+++
(b) If the message is a response to a message sent to a multicast or
anycast group in which the node is a member, the Source Address
of the reply must be a unicast address belonging to the
interface on which the multicast or anycast packet was received.
---
E: if ((addr_type & IPV6_ADDR_MULTICAST || skb->pkt_type != PACKET_HOST)) {
E: There seems to be no check for the anycast case.
Q: saddr = NULL;
Q: Later on saddr is written to fl.nl_u.ip6_u.saddr. Pointer to the flowi
Q: is given as a parameter to ip6_build_xmit(). Now that the the sending
Q: interface to be the same, the following conditions must be fullfilled:
if (!fl->oif && ipv6_addr_is_multicast(fl->nl_u.ip6_u.daddr))
fl->oif = np->mcast_oif;
Q: o address is not a link-local address and np->mcast_oif must point to the
Q: same interface than where the packet came from. Now this is per sock
Q: variable and there is only a global icmp socket for flow control
Q: purposes so I don't see this happening.
dst = __sk_dst_check(sk, np->dst_cookie);
if (dst) {
|| (fl->oif && fl->oif != dst->dev->ifindex)) {
dst = NULL;
Q: o if there is a destination cache entry for the socket which I don't
Q: understand in the context of icmp btw. and it is a link local
Q: address, ip6_route_output() is called. It is also called if there
Q: is no destination cache entry. Anyway, function checks route
Q: with rt6_device_match()
if ((rt->rt6i_flags & RTF_CACHE)) {
if (ip6_rt_policy == 0) {
rt = rt6_device_match(rt, fl->oif, strict);
Q: if these conditions hold true. Let us be reminded that fl->oif is != 0
Q: if addr_type & IPV6_ADDR_LINKLOCAL. If not RTF_CACHE then we get a lot
Q: of code I didn't get through.
Q: For the sending address to be a "unicast address beloging to the interface"
Q: then it must be that ipv6_get_saddr() given chosen route and especially
Q: its device must be same. Oh, and there must of course valid unicast addresses
Q: belonging to that interface.
Q: My hunch is that the sending address is most probably belonging to the same
Q: interface only if oif is set. Then it would be:
E: It is not guaranteed that sending unicast address belongs to the same
E: interface.
+++
(c) If the message is a response to a message sent to an address
that does not belong to the node, the Source Address should be
that unicast address belonging to the node that will be most
helpful in diagnosing the error. For example, if the message is
a response to a packet forwarding action that cannot complete
successfully, the Source Address should be a unicast address
belonging to the interface on which the packet forwarding
failed.
---
N: "Most helpful" is not very clearly defined. Neither do I understand
N: "on which the packet forwarding" failed. There are of course two interfaces
N: involved in packet forwarding. ICMP packets are sent to the sender so
N: this could logically refer to the ingress interface.
Q: /* Force OUTPUT device used as source address */
Q: skb->dev = dst->dev;
Q: But in this case the code seems to do the opposite. What's the logic of this?
Q: The text above could be intepreted to propose just that but is it the
Q: sensible thing to do?
+++
2.3 Message Checksum Calculation
---
O: icmpv6_getfrag() calculates the checksum. Pseudo-header checksum
O: calculated by csum_ipv6_magic() using IPPROTO_ICMPV6 as the proto.
Q: Wouldn't the checksum need be converted to network byte order? How
Q: is this done? I can't see the code in asm-i386/checksum.h.
+++
(b) If an ICMPv6 informational message of unknown type is received,
it MUST be silently discarded.
---
O: there is an explicit check for type & 0x80.
N: Change to use the constant.
+++
(d) In those cases where the internet-layer protocol is required to
pass an ICMPv6 error message to the upper-layer process, the
upper-layer protocol type is extracted from the original packet
(contained in the body of the ICMPv6 error message) and used to
select the appropriate upper-layer process to handle the error.
---
O: icmpv6_notify() skips extension headers by calling ipv6_skip_exthdr()
O: though it only understands a limited set of extension headers.
N:
N: /* BUGGG_FUTURE: we should try to parse exthdrs in this packet.
N: Without this we will not able f.e. to make source routed
N: pmtu discovery.
N: Corresponding argument (opt) to notifiers is already added.
N: --ANK (980726)
N: */
+++
(e.4) a packet sent as a link-layer multicast, (the exception
from e.2 applies to this case too), or
---
Q: That must refer to e.3?
+++
(f.1) Timer-based - for example, limiting the rate of
transmission of error messages to a given source, or to
any source, to at most once every T milliseconds.
---
O: This is implemented -- sysctl_icmpv6_time = 1*HZ.
N: With the following exceptions:
N: /* Informational messages are not limited. */
N: if (type & 0x80)
N: return 1;
N:
N: /* Do not limit pmtu discovery, it would break it. */
N: if (type == ICMPV6_PKT_TOOBIG)
N: return 1;
Q: Doesn't this poke holes if intentionally exploited?
+++
(f.2) Bandwidth-based - for example, limiting the rate at which
error messages are sent from a particular interface to
some fraction F of the attached link's bandwidth.
---
N: Not implemented.
+++
NOTE: THE RESTRICTIONS UNDER (e) AND (f) ABOVE TAKE PRECEDENCE OVER
ANY REQUIREMENT ELSEWHERE IN THIS DOCUMENT FOR SENDING ICMP ERROR
MESSAGES.
---
O: They do.
N: Except for echo reply?
Q: Why does this bypass the checks in icmpv6_send()?
+++
A Destination Unreachable message SHOULD be generated by a router, or
by the IPv6 layer in the originating node, in response to a packet
that cannot be delivered to its destination address for reasons other
than congestion. (An ICMPv6 message MUST NOT be generated if a
packet is dropped due to congestion.)
---
N: NOROUTE messages seem not to be generated.
N: ADM_PROHIBITED not sent by IPv6 code itself.
N: NOT_NEIGHBOUR messages are not either.
N: ADDR_UNREACH is generated. Maybe sometimes NOROUTE could be used instead.
N: PORT_UNREACH generated by udp, sit. Tcp probably sends RST instead?
+++
If the reason for the failure to deliver is lack of a matching entry
in the forwarding node's routing table, the Code field is set to 0
(NOTE: this error can occur only in nodes that do not hold a "default
route" in their routing tables).
---
E: see above.
+++
If the reason for the failure to deliver is administrative
prohibition, e.g., a "firewall filter", the Code field is set to 1.
---
N: Unimplemented. REJECT target is not currently supported by the IPv6 code.
+++
If the reason for the failure to deliver is that the destination is
beyond the scope of the source address, the Code field is set to 2.
This condition can occur only when the scope of the source address is
smaller than the scope of the destination address (e.g., when a
packet has a site-local source address and a global-scope destination
address) and the packet cannot be delivered to the destination
without leaving the scope of the source address (e.g., without
leaving the source's site, in the case of a site-local source
address).
---
N: Unimplemented. SITELOCAL is not checked anywhere in the code.
+++
Upper layer notification
An incoming Packet Too Big message MUST be passed to the upper-layer
process.
---
O: See icmpv6_rcv():
N: /* BUGGG_FUTURE: if packet contains rthdr, we cannot update
N: standard destination cache. Seems, only "advanced"
N: destination cache will allow to solve this problem
N: --ANK (980726)
N: */
Q: What does this mean exactly?
+++
Data Zero or more octets of arbitrary data.
---
Q: Can echo reply packets be fragmented? It seems so.
+++
An Echo Reply SHOULD be sent in response to an Echo Request message
sent to an IPv6 multicast address. The source address of the reply
MUST be a unicast address belonging to the interface on which the
multicast Echo Request message was received.
---
O: if (ipv6_addr_type(saddr) & IPV6_ADDR_MULTICAST)
O: saddr = NULL;
N: Same concerns apply than with icmpv6_send().
+++
5. Security Considerations
5.1 Authentication and Encryption of ICMP messages
ICMP protocol packet exchanges can be authenticated using the IP
Authentication Header [IPv6-AUTH]. A node SHOULD include an
Authentication Header when sending ICMP messages if a security
association for use with the IP Authentication Header exists for the
destination address. The security associations may have been created
through manual configuration or through the operation of some key
management protocol.
---
N: Not implemented.
+++
5.2 ICMP Attacks
4. ICMP messages may be used as attempts to perform denial of service
attacks by sending back to back erroneous IP packets. An
implementation that correctly followed section 2.4, paragraph (f)
of this specifications, would be protected by the ICMP error rate
limiting mechanism.
---
N: Echo replies are not rate limited.
--
D.
|