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

[Alan DeKok <[e-mail]>]

  Some comments.  Mostly little nits and clarifications.

  The major points for me are related to security.  The "Security Considerations" section is almost empty, and will certainly not pass a review from the security area.  They will in all likelihood block publication until substantive comments have been added.

 ------ 

3.3.  Single Connect Mode


   The packet header (see below) contains a flag to allow sessions to be
   multiplexed on a connection.

* it would be good to name the flag here.

   If the server sets this flag in the first reply packet in response to
   the first packet from a client, this indicates its willingness to
   support single-connection over the current connection.  The server
   may set this flag even if the client does not set it, but the client
   is under no obligation to honor it.

* what happens if the client does not honour the flag?

   The flag is only relevant for the first two packets on a connection,
   to allow the client and server to establish single connection mode.
   The flag MUST be ignored after these two packets since the single-
   connect status of a connection, once established, must not be
   changed.  The connection must instead be closed and a new connection
   opened, if required.

  Why would it be required to change the mode?  It would be good to explain the use-cases, and why (or not) they make sense.

   TAC_PLUS_UNENCRYPTED_FLAG := 0x01

   If this flag is set, then body encryption is not used.  If this flag
   is cleared, the packet is encrypted.  Unencrypted packets are
   intended for testing, and are not recommended for normal use.

* it would be good to have a pointer to the security considerations, with a discussion of the implications.

*  Also, it's odd that a completely insecure mode is just "not recommended for normal use".  I would suggest more panic-inducing phrasing.  e.g. ."Unencrypted packets offer a complete compromise of all security related to the protocol and MUST NOT be used outside of a testing environment"

   TAC_PLUS_SINGLE_CONNECT_FLAG := 0x04

   This flag is ussequernt, ed to allow a client and server to agree whether
   multiple sessions may be multiplexed onto a single connection.

* I find the "agree" phrasing odd.  I've mentioned before that the tone of the document is often philosophical.  Perhaps saying instead "this flag signals whether the client and server are capable of performing session multiplexing"

* i.e. the flag is a signal, not a capability for the systems to agree.

   session_id

   The Id for this TACACS+ session.  The session id is to be selected
   randomly.  This field does not change for the duration of the TACACS+
   session.  (If this value is not a cryptographically strong random
   number, it will compromise the protocol's security, see  RFC 1750

* it would be good to update the paragraph to be more prescriptive.  i.e.

   The Id for this TACACS+ session.  The session id MUST be taken
   fro a cryptographically strong random number generation.  If not,
   the protocol's security will be compromised.  See  RFC 1750
   for further information

...

   length

   The total length of the packet body (not including the header).  This
   value is in network byte order.  Packets are never padded beyond this
   length.

* Why is the text on "padding" there?  The TACACS+ packets are sent over TCP, not UDP.  So any "padding" would be outside of the scope of the current packet, and be seen by the other end as a subsequent, but invalid, TACACS+ packet.

3.5 ...

      - Unused fixed length fields SHOULD have values of zero.

* what happens when they don't?  I suggest adding text saying that the fields are ignored.

     - There will be no padding in any of the fields or at the end of a
      packet.

* I wonder again what "padding" means here.

3.6.  Encryption


   The body of packets may be encrypted.   ...

* Which is not a standard encryption method.  Security people will complain here.  I suggest (as before) using the term "obfuscated".  With a note that for historical reasons, the flags / fields / etc. refer to TACACS+ "encryption".

 ...  This document does not discuss the management and storage of
   those keys.  ...

* there should be some discussion in the Security Considerations section about this.  Otherwise, this subject will be brought up in the security review.

  ...  It is an implementation detail of the server and client,
   as to whether they will maintain only one key, or a different key for
   each client or server with which they communicate.  For security
   reasons, the latter options MUST be available, but it is a site
   dependent decision as to whether the use of separate keys is
   appropriate.

* those sentences seem to disagree with each other.  It's an "implementation detail", but it MUST be supported.  I suggest requiring the capability of per-server and per-client keys.

  ...   The session id is used in network byte order.

* nit: if the session ID is a 32-bit opaque token, it's not in any byte order.  Maybe just "as taken from the packet header" ?

   When a server detects that the secret(s) it has configured for the
   device mismatch, it MUST return ERROR.  The handling of the TCP
   connection by the server is implementation independent.

* I repeat my objection here.  There is just no reason for the TCP connection to remain open after the connection has been determined to be fraudulent.  This subject also came up in the RADIUS over TCP drafts.  The consensus there was the same: if it's not a real / known / good client, there is every reason to close the connection, and zero reasons to keep it open.

   After a packet body is decrypted, the lengths of the component values
   in the packet are summed.  If the sum is not identical to the
   cleartext datalength value from the header, the packet MUST be
   discarded, and an error signalled.  The underlying TCP connection MAY
   also be closed, if it is not being used for other sessions in single-
   connect mode.

* This last sentence is confusing.  Are there different secrets for different packets in one TCP stream?  If so, nothing in the document says so.  If not, then there is every reason to believe that the secret is mismatched. And, the *next* packet will decrypt incorrectly.  Which means (again) you might as well just close the TCP connection.

   If an error must be declared but the type of the incoming packet
   cannot be determined, a packet with the identical cleartext header
   but with a sequence number incremented by one and the length set to
   zero MUST be returned to indicate an error.

* this mix of clear-text and encrypted signalling opens up the protocol to an attacker.  Since this "NAK'" is not protected, an attacker can forge the NAK at any time, and force the connection to be closed.

* this attack (along with all others) should be noted in the Security Considerations section.

...
   Packet fields are as follows:

   action

   This describes the authentication action to be performed.  Legal
   values are:

* what happens if a client/server receives values outside of the legal range?  I suggest sending ERROR, and closing the TCP connection.

* similar comments apply for other fields with "legal values".  While it's good to define what happens when things go as planned, it's also good to define what happens when things *don't* go according to plan.

* review of 4.2 and subsequent sections will follow in a later message.