RE: Issue 101: WG Last Call Review

[<Pasi.Eronen@nokia.com>]

paul.congdon@hp.com wrote:

> [pc] Here are some proposed resolutions to Pasi's Issue 101 WG 
> review comments:

I'm happy with most of these proposals; see comments below
for the rest...

> 6) Section 3.3: Using "START OF HEADING" (0x01) and "START OF
> TEXT" (0x02) control characters to indicate tagged/untagged
> might make it unnecessarily difficult to enter this attribute
> in a management interface... maybe something like 0x74/0x75
> ("t"/"u") or 0x31/0x32 ("1"/"2") would be easier?
> 
> [pc] We were trying to get Egress-VLANID and (now called)
> Egress-VLAN-Name to have as similar a format as possible.
> While Egress-VLAN-Name could be made to be an all text type of
> management interface, Egress-VLANID can not.  The VLANID is a
> simple 12-bit number and won't map well to the keyboard input
> optimization no matter what we do, so some form of special
> formatting for the management interface will be required to
> support these two attributes.  Also, tagged and untagged are
> the only flags we need today, but perhaps this field would
> need to be extended someday (e.g. 802.1ad support), and using
> more bits to represent the two values needed today would seems
> to be wasteful.  I propose no change here.

Egress-VLANID doesn't necessarily need any special code in the
management interface if it supports entering hexadecimal numbers
(e.g. tagged VID 6 could be entered as 0x01000006).

And VLAN-Name attribute already uses 8 bits to represent these
two values; changing that from 0x01/0x02 to, say, 0x31/0x32
wouldn't use anything more... (and would not prevent us from
defining value values later any more than 0x01/0x02 does)

> 9) A comment about the bit figures in Section 3 and 4.
> Currently, all the figures are basically identical: they
> contain the RADIUS Type and Length fields, plus one other
> field (variably called either "Integer", "String" or "Text").
> The contents of this field are then described in English.
> 
> I think it would be much more understandable if the figure also showed
> the structure. For instance, instead of this (Section 3.1),
> 
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 
> |     Type      |    Length     |            Integer          
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 
> |        Integer(cont.)         |  
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 
> 
> this would IMHO be more readable:
> 
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 
> |     Type      |    Length     |   Tag Option  |     (zero)   
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 
>  (zero) |   VLANID (12 bits)    |  
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 
>
> (Note that I'm not proposing changing the attribute formats at
> all -- so no changes to any software --, just how they are
> presented in the document. And we can keep all the zero
> padding there if we want...)
>
> [pc] To make this clean and follow your recommendation I
> believe it has significant structural changes to the document.
> Most of the attributes do not have diagrams at all.  There
> would need to be a lot of new ASCII art added and it would be
> difficult for string based fields like NAS-Filter-Rule to
> accommodate this.  If we put all the internal field names in
> the base attribute diagram along with Type and Length, it
> would seem that we would need to describe each of the other
> fields individually rather than indicating how to format the
> Integer, Text or String portion.  I'd rather keep the bulk of
> the attribute sections intact and only provide a diagram
> describing how to format the Integer, Text or String porition
> - as is done in 3.1.  We could certainly make 3.2, 3.3 and 3.4
> more consistent with 3.1 and add a diagram on how to format
> the Integer, Text and/or String component.  Would this work?

I don't think significant changes are needed; basically, only
a couple of small changes would clarify the document significantly.
The most important ones would IMHO be:

- combining the two figures in 3.1

- fixing the figure in 4.1 (explicitly show that the attribute
  contains both a 64-bit counter and a string according to 
  NAS-Filter-Rule; calling the counter a part of the string
  is misleading, since it doesn't even consist of printable
  characters...)

- Add figure to section 3.3 (explicitly showing that the 
  attribute contains two different items)

> 11) Section 7: It looks like this security considerations
> section was just copied from somewhere without much thought
> for how well it actually applies to this document. Most of the
> text looks redundant to me; we don't need to describe how to
> use IPsec with RADIUS in every document that defines a new
> RADIUS attribute.
> 
> On the other hand, there's exactly zero discussion about new
> threats caused by the attributes defined in _this_ document.
> And to me it looks like there are new attacks that e.g. a
> malicious or compromised RADIUS server or proxy could do with
> these attributes (compared to the situation where NAS does not
> support these attributes).  At least Egress-VLANID,
> Ingress-Filters and NAS-Filter-Rule have some pretty obvious
> possibilities for abuse... (and maybe some non-obvious as
> well)
> 
> [pc] Yes, much of this text was copied from 3580, but short of
> trying to imagine all the things that could happen if a RADIUS
> server is comprimized, I'm not sure what else to put here.  We
> could puts some words here about how modified settings of
> things like Ingress-Filters or Filter-Rules could change the
> expected behavior, but the discussion would not be exhaustive.
> I thought the point of this section was to document what the
> vulnerabilities are, not how they might be used.  For example,
> the scheme is vulnerable to packet modification attacks, but
> we can't document what might happen with every possible
> modification of a packet.  Please provide some more
> explanation or examples of what you are looking for here.

There's no need to imagine all things that could happen if a
RADIUS server is compromised, but it is IMHO necessary to at
least briefly discuss how implementing _this_ document changes
the existing situation.

Maybe something like this:
 
  This documents specifies new attributes that can be included
  in existing RADIUS messages. These messages are protected
  using existing security mechanisms; see [RFC2865] and
  [RFC3576] for a more detailed description and related security
  considerations.

  The security mechanisms in [RFC2865] and [RFC3576] are
  primarily concerned with an outside attacker who modifies
  messages in transit or inserts new messages. They do not
  prevent an authorized RADIUS server or proxy from inserting
  attributes with a malicious purpose in messagse it sends.

  An attacker who compromises an authorized RADIUS server or
  proxy can use the attributes defined in this document to
  influence the behavior of the NAS in ways that previously may
  not have been possible. For example, modifications to the
  VLAN-related attributes may cause the NAS to permit access to
  other VLANs that were intended. To give another example,
  inserting suitable redirection rules to the NAS-Filter-Rule
  attribute may allow the attacker to eavesdrop or modify
  packets sent by a legitimate client.

  In general, the NAS cannot know whether the attribute values
  it receives from an authenticated and authorized server are 
  well-intentioned or malicious, and thus it is not possible to 
  completely protect against attacks by compromised nodes. In 
  some cases, the extent of the possible attacks can be limited 
  by performing more fine-grained authorization checks at the NAS. 
  For instance, a NAS could be configured to accept only certain 
  VLAN IDs from a certain RADIUS server/proxy, or not to accept
  any redirection rules if it is known they are not used in 
  this environment.

Best regards,
Pasi

--
to unsubscribe send a message to radiusext-request@ops.ietf.org with
the word 'unsubscribe' in a single line as the message text body.
archive: <http://psg.com/lists/radiusext/>