Document: draft-bberry-pppoe-credit-04.txt Reviewer: Elwyn Davies [elwynd@dial.pipex.com] Review Date: Tuesday 1/3/2006 12:35 PM CST Telechat Date: Thursday, 5 January 2006 Review also sent to the RFC Editor due to the submission route. In the course of discussions regarding other points, I realised that there is an additional security issue regarding the extended discovery phase packets (see below). Summary: In my opinion this proposal is technically unsound and should not be published in its current form. At the very least Section 5 has to be formulated differently (it is a serious abuse of PPP), the credit mechanism would need to be much better specified to demonstrate that it is robust and that interoperable implementations could be made, and, in my opinion, the value of the complex link quality mechanisms in likely deployment scenarios is questionable. I note that the PPPEXT wg review has also noted some serious concerns about the general principles of this proposal. I haven't repeated their views about layer violations and misunderstanding about the purpose of PPPoE - there are quite enough other problems. Even were it to be published I am unclear why this document should be Informational. It has been pointed out that PPPoE is an informational document rather than a PS so that if this were a pure extension of PPPoE then it might be also Informational. However the modifications to PPP in Section 5 also need to be taken into account. It therefore appears to be additionally documenting a proposed extension to a standard and as such ought to be aiming for PS. Assuming this document is approved, it needs an IANA considerations section. I think that it effectively creates two registries for PPPoE discovery message id codes and discovery option tags which (AFAICS) weren't setup by RFC2516 although the TAG registry is implicit in the wording of RFC2516. [I have been informed after the review was submitted that one of these registries is being set up by draft-arberg-pppoe-iana-00 but that document is not referenced and it desn't cover the message id code registry.] Review: -------- System overview diagram/description: PPPoE is supposed to connect a host to an Access Concentrator, not to link two Access Concentrators. The term Host Radio is probably misleading. I am not clear if this is all just an oversize typo, a fundamental misunderstanding or an intentionally different scenario. The scenario in which PPPoE is typically used normally envisages hosts tied to an access network via a single link. It is not clear that the routing protocol would have any alternatives other than the PPPoE link in most circumstances, so that the only link indication needed is link up/link down. The proposed metric system therefore seems to be overkill in most conventional PPPoE situations. If this is not the expected deployment scenario it really ought to be made clearer. Also it is not made clear how the radio related information is transferred to the PPPoE endpoints or whether the 'host radio' nodes are intended to intercept the relevant packets and fill in the required info (and then fix up the checksum - hmm!). If it were used, the interactions between the rate of change of radio bandwidth (especially in cellular cases) and the time constants in routing protocols need to be considered. The authors should take a look at draft-iab-link-indications-04.txt if they haven't already done so. ss4.3-4.5: If these packets are needed, I think it would make more sense to send them as a new PPP sub-protocol on the established session rather than overloading the discovery packets. The addressing of these packets needs to be made explicit if not. ss4.3-4.5 and s8: These new packets are transmitted outside any security envelope of PPP. There appears to be scope for DoS attacks using any or all of these packets which could either throttle a specific link, cause it to overload a radio link or cause the routing protocol to thrash. Sequence numbers are used on the credit control messages but their use as a security measure is not discussed (or indeed behaviour of any kind relating to missed sequence numbers or duplicate received sequence numbers). The link quality reporting messages are not protected in any way. There is therefore a vulnerability to spoofed packets of these types. s4.5: PPP already has a link quality report sub-protocol (0xc025) - do we need another one? Note if the radio is 'receive only' the link is no good for IP! Section 5: There is a fundamental misunderstanding here. The example packet given in RFC2516 seems to have been taken as meaning that *all* session stage packets will be using the LCP PPP protocol type (0xC021). This is clearly not the case - for example, plain vanilla user IPv4 packets will use protocol 0x0021 and there could be any number of different protcols going across the link. The proposal to piggyback credit tags onto these packets in the manner proposed is therefore broken - and a serious abuse of the PPP structure. A fix defining a new PPP protocol number would be entirely possible, but that is another matter. Section 6/7: I haven't analysed the credit flow mechanism in s6 in detail but it doesn't seem to take into account that Ethernet is not a reliable medium. Section 7 *does* seem to acknowledge this but the interaction between the inband and out-of-band credits during a session is far from clear - I don't think I could build interoperable implementations as it stands. Also, some idea of default timeout values needs to be specified. The document has no IANA considerations section. It assigns three new (pseudo-)discovery message types and three new 'TAG' numbers that fit into the space used for tags defined in RFC2516. These message ids and tags are currently internal to RFC2516 and so have no registries. Since RFC2516 allows new tags to be defined there presumably ought to be an IANA registry for these tags but it wasn't setup at the time. RFC2516 did not originally envision new discovery messages. I assume that there ought be to be expert review of the assignment by analogy with the other PPP registries. Editorial: The boilerplate is out of date (see idnits output). The figures need titles and bit position indicators. Also the rules say that figures are optional add-ons to words - s9 needs to specify the contents of the tags in words as well. The document needs a spell check. Running idnits: Checking nits according to http://www.ietf.org/ID-Checklist.html: * The document seems to lack an IANA Considerations section. Checking conformance with RFC 3978/3979 boilerplate... * Found RFC 3978 Section 5.5 boilerplate (on line 30), which is fine, but *also* found RFC 2026 Section 10.4C paragraph 4 boilerplate on line 701. It should be removed. * The document claims conformance with section 10 of RFC 2026, but uses some RFC 3978/3979 boilerplate. As RFC 3978/3979 replaces section 10 of RFC 2026, you should not claim conformance with it if you have changed to using RFC 3978/3979 boilerplate. * The document seems to lack an RFC 3978 Section 5.1 IPR Disclosure Acknowledgement -- however, there's a paragraph with a matching beginning. Boilerplate error? * The document seems to lack an RFC 3978 Section 5.4 Copyright Line -- however, there's a paragraph with a matching beginning. Boilerplate error? * The document seems to lack an RFC 3978 Section 5.4 Reference to BCP 78. * The document seems to lack an RFC 3979 Section 5, para 1 IPR Disclosure Acknowledgement -- however, there's a paragraph with a matching beginning. Boilerplate error? ( - It does however have an RFC 2026 Section 10.4(A) Disclaimer.) * The document seems to lack an RFC 3979 Section 5, para 2 IPR Disclosure Acknowledgement. * The document seems to lack an RFC 3979 Section 5, para 3 IPR Disclosure Invitation -- however, there's a paragraph with a matching beginning. Boilerplate error? ( - It does however have an RFC 2026 Section 10.4(B) IPR Disclosure Invitation.) * The document uses RFC 3667 boilerplate or RFC 3978-like boilerplate instead of verbatim RFC 3978 boilerplate. After 6 May 2005, submission of drafts without verbatim RFC 3978 boilerplate is not accepted. The following non-3978 patterns matched text found in the document. That text should be removed or replaced: "By submitting this Internet-Draft, I certify that any applicable patent or other IPR claims of which I am aware have been disclosed, or will be disclosed, and any of which I become aware will be disclosed, in accordance with RFC 3668." * There are 243 instances of too long lines in the document, the longest one being 6 characters in excess of 72. * There is 1 instance of lines with non-ascii characters in the document. Checking nits according to http://www.ietf.org/ietf/1id-guidelines.txt: - It seems as if not all pages are separated by form feeds - found 0 form feeds but 20 pages Miscellaneous warnings: None.