Subject: RE: AD review of draft-zorn-radius-pkmv1-04.txt

From: "Glen Zorn" <[email-masked]>
Date: Thu, 23 Jul 2009 19:37:24 +0700

Alan DeKok [mailto:[email-masked]] writes:

> Romascanu, Dan (Dan) wrote:
> > I have reviewed as shepherding AD draft-zorn-radius-pkmv1-04.txt
> 
>   Following on Dan's review, I've reviewed the document for agreement
> with the RADIUS design guidelines document [1]
> 
> 
>   Both the PKM-SS-Cert and PKM-CA-Cert attributes provide 'ad-hoc'
> extension of the RADIUS attribute size, much like the EAP-Message
> attribute.  It would have been preferable to follow the extended
> attribute format [2].  This provides a standardized way of carrying
> data
> larger than a 253 bytes.
> 
>   However, that document has not yet been published.  My question is
> that if there are no current implementations of the PKM specification,
> it may be preferable to wait until the extended attributes document is
> finished, and then to use that format.

No.  There _are_ implementations as I rather clearly stated at the meeting
in SF, using the Experimental attribute space.

> 
>   Section 3.3 defines the PKM-Config-Settings.  This is a "complex
> attribute" within the meaning of Section 2.1.3 of the design guidelines
> document, which states:
> 
>    As a
>    result, the introduction of new complex data types within RADIUS
>    attribute specifications SHOULD be avoided, except in the case of
>    complex attributes involving authentication or security
>    functionality.

All of the attributes are security related.

> 
>   I do not see any pressing need to make this attribute carry seven
> independent fields.  The RADIUS attribute space has sufficient room to
> allocate 7 attributes.  If the extended attribute format is used, then
> nearly all space limitations are removed.  I would recommend that these
> independent values be split up into independent attributes that follow
> the RADIUS data model.

Given that there are already multiple interoperable implementations and all
of the attributes are security related, I see no pressing need to make
people change running code to satisfy your personal preferences.  Sorry if
these attributes were designed to be easy to implement for people writing
PKM code, rather than RADIUS code.
 
> 
>   In Section 3.4.  PKM-Cryptosuite-List, can the attribute be longer
> than 253 bytes?    

No.  There are exactly 

...

> 
>   Section 3.5.  PKM-SAID, defines an attribute containing 2 octets of
> data.  It would be preferable to use a 4-octet type, and to specify
> that
> the upper 2 octets are zero.  This would allow the attribute to fit
> within the existing RADIUS data model, as discussed in Section 2.1.1 of
> the design guidelines document:
> 
>    It is worth noting that since RADIUS only supports unsigned integers
>    of 32 or 64 bits, attributes using signed integer data types or
>    unsigned integer types of other sizes will require code changes, and
>    SHOULD be avoided.

This would allow a lot more code to be written, too.

> 
>   Section 3.6.  PKM-SA-Descriptor defines another complex attribute as
> discussed above.  It would be good to define this as a 64-bit integer,
> which would fit within the RADIUS data model.

You're absolutely right: why make it easy on engineers when there's the
"RADIUS data model" to defend?

> 
>   Section 3.7.  PKM-AUTH-Key defines a complex attribute for
> authentication or security, which fits within the design guidelines.

All of these attributes are related to security.

> However, the encryption of the attribute is specified by reference to
> [IEEE.802.16-2004].  This encryption is unlike anything previously used
> in RADIUS, but it appears to be mandated by the IEEE specification.

Yes, in an obvious error, the IEEE seems to have completely ignored the
RADIUS data model.  Silly them!

...

>   I also have some comments on the Security Considerations section:
> 
>    If the Access-Accept message is not subject to strong integrity
>    protection, an attacker may be able to modify the contents of the
>    PKM-Auth-Key Attribute.  For example, the Key field could be
> replaced
>    with a key known to the attacker.
> 
>   Would it be sufficient to require that the Access-Accept contains a
> Message-Authenticator for integrity protection?

If you consider MD5 to be "strong integrity protection".

> 
> 
>    Although it is necessary for a plaintext copy of the Key field in
> the
>    PKM-AUTH-Key Attribute to be transmitted in the Access-Accept
>    message, this document does not define a method for doing so
>    securely.
> 
>   How is inter-operability to be achieved if there is no specification
> for how to carry a necessary field?
> 
>   I suggest changing the suggestion to use MS-MPPE-Send-Key into a
> mandatory requirement, and making it part of the specification.

MPPE-Send-Key is broken; I don't see the value in requiring the use of
broken technology.  There are much better ways to encrypt attributes for
transmission, in which (once again) the radext WG has shown near zero
interest.  Maybe it should be a cisco VSA?

...

---

From: Alan DeKok <[email-masked]>
Date: Thu, 23 Jul 2009 11:43:05 +0200

Both the PKM-SS-Cert and PKM-CA-Cert attributes provide 'ad-hoc'
extension of the RADIUS attribute size, much like the EAP-Message
attribute.  It would have been preferable to follow the extended
attribute format [2].  This provides a standardized way of carrying data
larger than a 253 bytes.

   As a

  In Section 3.4.  PKM-Cryptosuite-List, can the attribute be longer
than 253 bytes?    If so, do the same ad-hoc rules as above apply?  The
IEEE specification seems to permit attributes up to 32768 octets in length.

  It would also be good to reference a specific page or section in the
IEEE spec, rather than just referring to the document as a whole.

data.  It would be preferable to use a 4-octet type, and to specify that

in RADIUS, but it appears to be mandated by the IEEE specificiation.

  It would also be good to reference a specific section of
[IEEE.802.16-2004] for this definition, rather than referencing the
document as a whole.

   If the Access-Accept message is not subject to strong integrity
   protection, an attacker may be able to modify the contents of the
   PKM-Auth-Key Attribute.  For example, the Key field could be replaced
   with a key known to the attacker.

   Although it is necessary for a plaintext copy of the Key field in the
   PKM-AUTH-Key Attribute to be transmitted in the Access-Accept
   message, this document does not define a method for doing so
   securely.

  Alan DeKok.

[1] http://tools.ietf.org/html/draft-ietf-radext-design-07
[2] http://tools.ietf.org/html/draft-ietf-radext-extended-attributes-08

---

From: Alan DeKok <[email-masked]>
Date: Thu, 23 Jul 2009 17:01:52 +0200

> in SF, using the Experimental attribute space.

  So the implementations have to be updated to use the IANA code points,
independent of them possibly using the extended attributes format.

> people change running code to satisfy your personal preferences.

 The contents of the design guidelines document reflect the WG consensus
after years of effort and review.  It by *no* means reflects solely my
personal preference, as seems to be implied here.

>  Sorry if

  There's no need to follow the RADIUS design guidelines, because the
attributes refer to data that interacts with non-RADIUS systems...

  What value, then, is in the design guidelines, WG consensus, or IETF
review?  Can we just over-ride them willy-nilly because a vendor has an
implementation of a spec?

>>   In Section 3.4.  PKM-Cryptosuite-List, can the attribute be longer

  This response seems to have been cut off.

> If you consider MD5 to be "strong integrity protection".

  It appears to be good enough for the rest of RADIUS, at least until
the TLS-based methods are standardized.

> interest.  Maybe it should be a cisco VSA?

  If it's documented, sure.

  Alan DeKok.

---

From: [email-masked]
Date: Thu, 23 Jul 2009 20:21:29 +0000 (UTC)

Glen Zorn writes... 

> > As a  result, the introduction of new complex data types within 

> > RADIUS attribute specifications SHOULD be avoided, except 

> > in the case of complex attributes involving authentication or 

> > security functionality. 

Yeah.  I've always been a bit uncomfortable with the "security functionality" escape clause in the RADIUS Design Guidelines draft.  Lots of things can reasonably be claimed to be "security related".  I would have preferred the exception to be crafted a bit narrower, just for this reason.  But, unless wording of Design Guidelines is changed, you have a legitimate argument. 

> PKM code, rather than RADIUS code. 

No one has ever claimed that following the RADIUS Design Guidelines is the only way to interoperable implementations.  Lots of designs, good, bad and indifferent, can be made to interoperate . 

If we are to dismiss the Design Guidelines as a "personal preference" how are they to be taken seriously?  I understand that following someone else's design guidance is not always popular with developers, but following regular design patterns is a well recognized practice in software engineering, that is generally attributed with increasing product quality and maintainability.  I think we simply need to "get with the program" here.

---

From: Bernard Aboba <[email-masked]>
Date: Sun, 26 Jul 2009 16:06:59 -0700

>   I believe the intent was "related to RADIUS security".  The guidelines
> document could be updated to address this.

The rationale behind the original exemption was that security required
changes on both the RADIUS client and server.  Therefore the RADIUS
server would need a code change anyway, regardless of whether the
attributes were complex or simple.   That rationale applies not just
to "RADIUS security" but also to authentication mechanisms, many of
which were previously implemented with complex attributes (e.g. CHAP). 
So I'm not clear that we're talking about a "loophole" here. 

> encapsulation using RFC 2548 MPPE-Key attributes...

I was unclear about how this is supposed to work.  Is the idea to apply
the MPPE-Key encryption mechanism to the attribute specified
in the draft, or is the idea to actually use the MPPE-Key attributes 
themselves?  If the former, more detail should be provided.  If the latter,
is it necessary to define two attribute formats or would it be simpler to
go with one?  If the RFC 2548 MPPE-Key attributes are used, is the format
the same as that defined in RFC 2548 (just a wrapped key) or is the wrapping
applied to a complex attribute?  

> a four octet Integer should be used instead of a two octet data type
> (which doesn't exist in RADIUS)

As I recall, the security exemption didn't apply to creation of new RADIUS
data types, correct?

---

From: Michael Richardson <[email-masked]>
Date: Thu, 23 Jul 2009 15:10:10 -0400

>>>>> "Alan" == Alan DeKok <[email-masked]> writes:
    Alan>   Both the PKM-SS-Cert and PKM-CA-Cert attributes provide
    Alan> 'ad-hoc' extension of the RADIUS attribute size, much like the
    Alan> EAP-Message attribute.  It would have been preferable to

  Back in the time of EAP-SIM, I complained about the rather
inconsistent attribute encoding in it, and why didn't they use the
radius encoding, or at least some consistent mechanism.
  (Sizes are both in "words" and "bytes" in EAP-SIM)

  While doing interop, I found at least two implementations that got
things wrong (and therefore their corresponding clients must not checked
at all!) and would have resulted in buffer overflows, and possible
exploits. 

  I want to emphasis what Alan says in the next message:

    Alan>  What value, then, is in the design guidelines, WG consensus,
    Alan>  or IETF  review?  Can we just over-ride them willy-nilly
    Alan>  because a vendor has an implementation of a spec?

- -- 
]     Y'avait une poule de jammé dans l'muffler!!!!!!!!!        |  firewalls  [
]   Michael Richardson, Sandelman Software Works, Ottawa, ON    |net architect[
] [email-masked] http://www.sandelman.ottawa.on.ca/ |device driver[
]    h("Just another Debian GNU/Linux using, kernel hacking,    ruby  guy");  [

---

From: Alan DeKok <[email-masked]>
Date: Fri, 24 Jul 2009 17:06:49 +0200

[email-masked] wrote:
> Yeah.  I've always been a bit uncomfortable with the "security
> functionality" escape clause in the RADIUS Design Guidelines draft. 
> Lots of things can reasonably be claimed to be "security related".  I
> would have preferred the exception to be crafted a bit narrower, just
> for this reason.  But, unless wording of Design Guidelines is changed,
> you have a legitimate argument.

  RADIUS could transport parameters for *another* protocol.  Those
attributes are not security related.  They either follow the RADIUS data
model (int, IP address, etc.), or they are "opaque data" that RADIUS is
simply transporting on the behalf of the other protocol.

  Alan DeKok.