[Gen-art] Gen-art review of draft-bberry-pppoe-credit-04.txt (updated)

I was selected as General Area Review Team reviewer for this specification
(for background on Gen-ART, please see
http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html).

Document: draft-bberry-pppoe-credit-04.txt
Intended Status: Informational (Individual submission via RFC Editor)
Shepherding AD: Mark Townsley
Review Trigger: IESG Telechat 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.

_______________________________________________
Gen-art mailing list
Gen-art@ietf.org
https://www1.ietf.org/mailman/listinfo/gen-art