Re: [OPSAWG] Start of WGLC for TACACS+ document.

[Alan DeKok <[e-mail]>]

  Continuing...

  Summary: the spec is still fairly descriptive / philosophical instead of prescriptive.  Many optional parts of the protocol are described, "A and B are allowed", but there is often no description of what to *do* when either A or B is present.  I've suggested descriptive text, or places where descriptive text can be added.

 The larger worry here is what happens if the descriptive text conflicts with existing implementations?  This is a topic which should be discussed in the "Security Considerations" section.

  i.e. the spec documents the existing protocol, with recommendations for security and inter-operability.  As the protocol has historically been poorly specified, implementations are likely to follow the broad requirements of this specification.  They are also likely to not follow many of the newly-clarified behaviours / requirements.  The hope is that this specification can guide implementors towards a common understanding of the protocol.

  Also, vague requirements and open options in a protocol spec are ripe for implementation bugs and security issues.  That worry drives many of my comments.

-----

4.2 ...

server_msg, server_msg_len

   c A message to be displayed to the user.  This field is optional.  If
   it exists, it is intended to be presented to the user.  US-ASCII
   charset MUST be used.  The server_msg_len MUST indicate the length of
   the server_msg field, in bytes.

* The last sentence is atypical of RFC 2119 language.  Then language is typically about system behaviour or inter-field comparisons.  I'm not sure what it means that a field MUST indicate a length.

* i.e. the "server_msg_len" field is *defined* to encode the length of the "server_msg" field.  No "MUST" is necessary.

* similar comments apply to many of the other "foo_len" fields, elsewhere in the document

 rem_addr, rem_addr_len

   An US-ASCII string this is a "best effort" description of the remote
   location from which the user has connected to the client. 

* perhaps instead of "best effort", just re-use language from the "port" description.  The contents of this field are the clients description of the remote location...

4.4 ...


   When the REPLY status equals TAC_PLUS_AUTHEN_STATUS_GETDATA,
   TAC_PLUS_AUTHEN_STATUS_GETUSER or TAC_PLUS_AUTHEN_STATUS_GETPASS,
   then authentication continues and the SHOULD provide server_msg

* editorial: the WHAT should provide...

   ... quest for more information to flexibly support future
   requirements.  If the TAC_PLUS_REPLY_FLAG_NOECHO flag is set in the
   REPLY, then the user response must not be echoed as it is entered.

* the implicit assumption here is that there is a person who is entering text on a terminal.  It would be good to have a bit more explanation of this expected use-case.

  NOTE: Only those requests which have changed from their minor_version
   0 implementation (i.e.  CHAP, MS-CHAP and PAP authentications) will
   use the new minor_version number of 1.  All other requests (i.e.  all
   authorisation and accounting and ASCII authentication) MUST continue
   to use the same minor_version number of 0.

* that's an odd phrasing of the requirement.  Perhaps something more prescriptive would be useful.  e.g.:

  NOTE: Authentication requests which use ASCII login or chpass MUST use minor version 0.  Authentication requests using PAP, CHAP, or MS-CHAP MUST
  use minor version 1.  All authorization and accounting packets MUST use minor version 0.

4.4.2...

   This section describes common authentication flows.  If the options
   are implemented, they MUST follow the description.  If the server
   does not implement an option, it will respond with
   TAC_PLUS_AUTHEN_STATUS_ERROR.

* It's not typically required to say that implementations MUST follow the spec.

* also, perspective terminology is better than descriptive. Not "will do", but "MUST do".

*  I suggest re-phrasing:

  This section describes common authentication flows.  If the server
   does not implement an option, it MUST respond with
   TAC_PLUS_AUTHEN_STATUS_ERROR.

   This is a standard ASCII authentication.  The START packet may
   contain the username, but need not do so.  

* what happens when the username is / is-not included?  i.e. what do implementations *do*?

* if this is a valid protocol flow, then it should be described.  e.g. "Any username in the START packet is ignored", or "any username in the START packet MUST match the username in subsequent packets"

   ... The data fields in both
   the START and CONTINUE packets are not used for ASCII logins. 

* what happens if there is data in the fields?  Is it ignored?

* RFC 2119 language is recommended here.  "any data (if present) MUST be ignored", or "the data field MUST NOT be present"

* I have no idea how to make these requirements compatible with existing implementations.  Sorry...

  CHAP:

   The length of the challenge value can be determined from the length
   of the data field minus the length of the id (always 1 octet) and the
   length of the response field (always 16 octets).

* it would be good to recommend a minimum length for challenge.  1 octet is clearly insufficient, but is (silently) allowed by the spec.  I suggest at least 8 octets/

* also, the challenge MUST be derived from a cryptographically strong pseudo-random number generator, and MUST change on every CHAP request.  Otherwise, the spec allows (also silently) a fixes challenge.

   To perform the authentication, the server will run MD5 over the id,
   the user's secret and the challenge, as defined in the PPP
   Authentication RFC 1334 and then compare that value
   with the response.

* again "server WILL run".  As opposed to "calculates..."

MSCHAPv1 / MSCHAPv2:

   The length of the challenge value can be determined from the length
   of the data field minus the length of the id (always 1 octet) and the
   length of the response field (always 49 octets).

*  RFC 2433 says that the challenge is 8 octets.  RFC 2759 says that the challenge is 16 octets.  It would be good to re-iterate that here.

* i.e. smaller challenges are insecure, and should be forbidden

   ... To perform the authentication, the server will use a the algorithm
   specified  RFC2759 

* editorial: "specified IN RFC ..."

 ... ASCII change password request

* I suspect that the security area review will recommend that this be removed.  Even RADIUS had "change password" removed at the behest of the security people. 

4.4.3.  Aborting an Authentication Session


   The client may prematurely terminate a session by setting the
   TAC_PLUS_CONTINUE_FLAG_ABORT flag in the CONTINUE message.  If this
   flag is set, the data portion of the message may contain an ASCII
   message explaining the reason for the abort. 

* again, what happens if the field is / is-not present?

* what does the server do with this message?  Should it be logged somewhere?

 ... The session is
   terminated and no REPLY message is sent.

* what happens to the TCP connection?  Does it remain open?  It may be good to say "The session is terminated, and the session Id is no longer valid.  However, other sessions on the same TCP connection may continue..."

   The host is specified as either a fully qualified domain name, or an
   ASCII numeric IPV4 address specified as octets separated by dots
   ('.'), or IPV6 address test representation defined in  RFC 4291.

* editorial ".. text .. "representation, not "test"

   If a key is supplied, the client MAY use the key in order to
   authenticate to that host.  The client may use a preconfigured key
   for the host if it has one. 

* so the server is pushing keys to clients?  The security implications of this should be discussed

* what happens if the client gets pushed a key, and then also has a pre-configured key?  Which one takes precedence?  Why?

 ... that's it for today.  More later.