[secdir] SecDir and AppsDir review of draft-ietf-storm-iscsi-cons-06

Alexey Melnikov <alexey.melnikov@isode.com> Wed, 03 October 2012 13:54 UTC

Return-Path: <alexey.melnikov@isode.com>
X-Original-To: secdir@ietfa.amsl.com
Delivered-To: secdir@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 95D7321F84F6; Wed, 3 Oct 2012 06:54:55 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -101.375
X-Spam-Level:
X-Spam-Status: No, score=-101.375 tagged_above=-999 required=5 tests=[AWL=-0.576, BAYES_50=0.001, GB_I_LETTER=-2, J_CHICKENPOX_102=0.6, J_CHICKENPOX_35=0.6, USER_IN_WHITELIST=-100]
Received: from mail.ietf.org ([64.170.98.30]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id dbbtMHStjRUb; Wed, 3 Oct 2012 06:54:54 -0700 (PDT)
Received: from statler.isode.com (statler.isode.com [62.3.217.254]) by ietfa.amsl.com (Postfix) with ESMTP id 4F29B21F8501; Wed, 3 Oct 2012 06:54:49 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; t=1349272488; d=isode.com; s=selector; i=@isode.com; bh=Gg/hGxyRnEsZC/tYRpCG/F+K514/Qma/0Yst1rgDtm8=; h=From:Sender:Reply-To:Subject:Date:Message-ID:To:Cc:MIME-Version: In-Reply-To:References:Content-Type:Content-Transfer-Encoding: Content-ID:Content-Description; b=goJC1IAEAMJTd7DIcL/KfH2WYmWlMA5vjHOW+1zdu9rGAB4ae6aq6OB0YxMZzpqKypvm2D irccqoPbHviFsMs6BUEk2PsjLtqObGmCVgYNN1dEwhK2etXV1fTEhGDhrQ0fUYXcnKi/qy qNtxsCIVVvPfd6BdJ2EVyipFdOqFasc=;
Received: from [172.16.1.29] (shiny.isode.com [62.3.217.250]) by statler.isode.com (submission channel) via TCP with ESMTPA id <UGxDpwA1oqIy@statler.isode.com>; Wed, 3 Oct 2012 14:54:48 +0100
Message-ID: <506C43AA.9010206@isode.com>
Date: Wed, 03 Oct 2012 14:54:50 +0100
From: Alexey Melnikov <alexey.melnikov@isode.com>
User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20120614 Thunderbird/13.0.1
To: secdir@ietf.org, draft-ietf-storm-iscsi-cons.all@tools.ietf.org, iesg@ietf.org
References: <4FBFAE5F.8010305@gmail.com>
In-Reply-To: <4FBFAE5F.8010305@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="ISO-8859-1"; format="flowed"
Content-Transfer-Encoding: 7bit
Subject: [secdir] SecDir and AppsDir review of draft-ietf-storm-iscsi-cons-06
X-BeenThere: secdir@ietf.org
X-Mailman-Version: 2.1.12
Precedence: list
List-Id: Security Area Directorate <secdir.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/secdir>, <mailto:secdir-request@ietf.org?subject=unsubscribe>
List-Archive: <http://www.ietf.org/mail-archive/web/secdir>
List-Post: <mailto:secdir@ietf.org>
List-Help: <mailto:secdir-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/secdir>, <mailto:secdir-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 03 Oct 2012 13:54:55 -0000

I have reviewed this document as part of the security directorate's
ongoing effort to review all IETF documents being processed by the IESG.
These comments were written primarily for the benefit of the security
area directors.  Document editors and WG chairs should treat these
comments just like any other last call comments.

This document describes a transport protocol for SCSI that works
on top of TCP.


Summary

Overall this is a very well written document. I also trust that all 
options were implemented by at least 1 implementation, so the document 
reflects implementation experience.

The Security Considerations section is thorough. iSCSI security 
partially relies on security provided by IPSec. Unfortunately I do not 
have the IPSec expertise to determine if all relevant cases were 
covered, although the text looked plausible.

Security considerations for CHAP and SRP authentication mechanisms are 
discussed in details and seem to be adequate. I've not seen any 
discussion of KRB5 (mentioned in 12.1) related issues though.

I also have other more detailed comments (some of them are security 
related). They range between minor and nits, although some of them can 
be reclassified as a bit more important:


In Section 2.2:

 >- SCSI Port Name: A name made up as UTF-8 characters and includes
 >  the iSCSI Name + 'i' or 't' + ISID or Portal Group Tag.

The first mention of UTF-8 needs a reference.

"UTF-8 characters" should be something like "UTF-8 [UTF-8] encoding of 
Unicode characters.
(There is no such thing as "UTF-8 character")


In Section 4.2, 2nd paragraph:

 >  For the remainder of this document, the terms "initiator" and
 >  "target" refer to "iSCSI initiator node" and "iSCSI target node",
 >  respectively (see iSCS) unless otherwise qualified.

I didn't find iSCS in the list of acronyms.

In 4.2.2.3.3:

   Whenever the TaskReporting key (Section 13.23"Task Reporting") is
   negotiated to ResponseFence or FastAbort for an iSCSI session and
   the Response Fence behavior is required for a SCSI response
   message,

The last part: how can this be known?

   the target iSCSI layer MUST perform the actions described
   in this Section for that session.

In 4.2.2.3.4:

  Note:
     - Due to the absence of ACA-related fencing requirements in
       [RFC3720], initiator implementations SHOULD NOT use ACA on
        multi-connection iSCSI sessions with targets complying only
        with [RFC3720].


How can the initiator know whether the target only complies with RFC 
3720 or not?

4.2.3.2. Notion of Affected Tasks

   LOGICAL UNIT RESET: All outstanding tasks from all initiators for
   the LU identified by the LUN field in the LOGICAL UNIT RESET
   Request PDU.

Are there any access control facilities for this operation?

4.2.3.4. FastAbort Multi-task Abort Semantics

   Protocol behavior defined in this Section SHOULD be implemented by

Why is this a SHOULD (and not a MUST)? What are possible alternatives?

   all iSCSI implementations complying with this document. Protocol
   behavior defined in this Section MUST be exhibited by iSCSI
   implementations on an iSCSI session when they negotiate the
   TaskReporting (Section 13.23) key to "FastAbort" on that session.
   The execution of ABORT TASK SET, CLEAR TASK SET, LOGICAL UNIT
   RESET, TARGET WARM RESET, and TARGET COLD RESET TMF Requests
   consists of the following sequence of actions in the specified
   order on the specified party.

In Section 4.2.7.1

      iSCSI names are composed only of displayable characters.

What does "displayable" means here? "ASCII printable"? "Unicode printable"?

         iSCSI
         names allow the use of international character sets but are
         not case sensitive.

What does this mean exactly? Do you mean ASCII case sensitivity? Unicode 
case sensitivity?

         No whitespace characters are used in
         iSCSI names.

What is "whitespace". U+0020? (ASCII space) Something else?

4.2.7.2. iSCSI Name Encoding

   The stringprep process is described in [RFC3454]; iSCSI's use of
   the stringprep process is described in [RFC3722]. Stringprep is a
   method designed by the Internationalized Domain Name (IDN) working
   group to translate human-typed strings into a format that can be
   compared as opaque strings. Strings MUST NOT include punctuation,
   spacing, diacritical marks, or other characters that could get in
   the way of readability.

This MUST NOT is not well defined. You need to define specific Unicode 
codepoints or character classes.

   The stringprep process also converts
   strings into equivalent strings of lower-case characters.


   The stringprep process does not need to be implemented if the
   names are only generated using numeric and lower-case (any
   character set) alphabetic characters.

Lower-case is not well defined, unless you say you mean ASCII or 
something else.
Also, "any character set"? This really doesn't look correct.

   Once iSCSI names encoded in UTF-8 are "normalized" they may be
   safely compared byte-for-byte.

Nit: I suggest adding "as described in this section" after "normalized" 
to distinguish this from various forms of Unicode Normalization, etc.


In Section 4.2.7.4:

   To generate names of this type, the person or organization
   generating the name must own a registered domain name. This domain
   name does not have to be active,

IMHO, the meaning of "active" is not clear here.

   and does not have to resolve to
   an address; it just needs to be reserved to prevent others from
   generating iSCSI names using the same domain name.



Every time the document is referring to "hexadecimal" - is the case 
important or not?

In 4.2.9:

   In situations where IP packets are delivered in order from the
   network, iSCSI message framing is not an issue and messages are
   processed one after the other. In the presence of IP packet
   reordering (i.e., frames being dropped), legacy TCP

Nit: What is so "legacy" about these implementations? Please explain or 
drop "legacy" here.

   implementations store the "out of order" TCP segments in temporary
   buffers until the missing TCP segments arrive, upon which the data
   must be copied to the application buffers. In iSCSI, it is
   desirable to steer the SCSI data within these out of order TCP
   segments into the pre-allocated SCSI buffers rather than store
   them in temporary buffers. This decreases the need for dedicated
   reassembly buffers as well as the latency and bandwidth related to
   extra copies.

4.3. iSCSI Session Types

   The session type is defined during login with key=value parameter
   in the login command.

Using which key?

In 4.6.1.5:

   To enable a SCSI command to be processed while involving a minimum
   number of messages, the last SCSI Data-in PDU passed for a command
   may also contain the status if the status indicates termination
   with no exceptions (no sense or response involved).

A forward reference to "sense" would be good here.

In 6.1:

  base64-constant: base64 constant encoded as a string that
        starts with "0b" or "0B" followed by 1 or more digits or
        letters or plus or slash or equal.

I think you need to include ASCII codes for the above, just to be clear.

Also, this is not quite correct, as leading "=" is not valid in base64 
encoding, etc.

        The encoding is done
        according to [RFC2045]

Please reference "Section 4 of [RFC4648]" instead of RFC 2045. RFC 4648 
is the most recent
RFC for base64 definition.

6.2.1. List negotiations

   In list negotiation, the originator sends a list of values (which
   may include "None") in its order of preference.

Is "None" a special value that designates an empty list? Or am I 
misintepreting what it is used for?

   If an acceptor does not understand any particular value in a list,
   it MUST ignore it. If an acceptor does not support, does not
   understand, or is not allowed to use any of the proposed options
   with a specific originator, it may use the constant "Reject" or
   terminate the negotiation. The selection of a value not proposed
   MUST be handled as a protocol error.

I suggest inserting "by the initiator" before "as a protocol error".


6.2.2. Simple-value Negotiations

   Specifically, the two cases in which answers are OPTIONAL are:

      - The Boolean function is "AND" and the value "No" is
        received. The outcome of the negotiation is "No".

      - The Boolean function is "OR" and the value "Yes" is
        received. The outcome of the negotiation is "Yes".

You lost me here, can you provide an example or two?

In 6.3:

   The Login Phase MAY include a SecurityNegotiation stage and a
   LoginOperationalNegotiation stage and MUST include at least one of
   them, but the included stage MAY be empty except for the mandatory
   names.

Again, an example (or pointer) here would be useful, as I am not 
entirely clear what this mean.


   Security MUST be completely negotiated within the Login Phase. The

What is the meaning of the word "Security" in this case?

   use of underlying IPsec security is specified in Chapter 8 and in
   [RFC3723]. iSCSI support for security within the protocol only
   consists of authentication in the Login Phase.


   In some environments, a target or an initiator is not interested
   in authenticating its counterpart. It is possible to bypass

How?

   authentication through the Login request and response.

In 6.3.1:

       -Login Response with Login reject. This is an immediate
        rejection from the target that causes the connection to
        terminate and the session to terminate if this is the first
        (or only) connection of a new session. The T bit and the CSG
        and NSG fields are reserved.

What does "reserved" mean in this case?

In 6.3.2:

   It should be noted that the negotiation might also be directed by
   the target if the initiator does support security, but is not
   ready to direct the negotiation (propose options).

How can this be done?

In 6.3.5:

   Session timeout is an event defined to occur when the last
   connection state timeout expires and no tasks are waiting for
   reassignment. This takes the session to the FREE state (N6
   transition in the session state diagram).

Is the timeout implied or negotiated?

In 7.1.4:

   In the detailed description of the recover classes the
   mandating terms (MUST, SHOULD, MAY, etc.) indicate normative
   actions to be executed if the recovery class is supported and
   used.

How is such support shown/negotiated?


7.1.4.1. Recovery Within-command

      Header digest error, which manifests as a sequence reception
         timeout or a sequence error - dealt with as specified in
         Section 7.9, using the option of a recovery R2T.

Did you mean 7.9 or 7.8?
(Also check elsewhere in the document.)


7.1.4.2. Recovery Within-connection

   At the initiator, the following cases lend themselves to within-
   connection recovery:

      - Requests not acknowledged for a long time. Requests are
        acknowledged explicitly through ExpCmdSN or implicitly by
        receiving data and/or status. The initiator MAY retry non-
        acknowledged commands as specified in Retry an.

Sorry, I didn't get what "Retry an" is.

In 7.2.2:

   In reassigning connection allegiance for a command, the targets
   SHOULD continue the command from its current state. For example,
   when reassigning read commands, the target SHOULD take advantage
   of the ExpDataSN field provided by the Task Management function
   request (which must be set to zero if there was no data transfer)
   and bring the read command to completion by sending the remaining
   data and sending (or resending) the status. ExpDataSN
   acknowledges all data sent up to, but not including, the Data-In
   PDU and or R2T with DataSN (or R2TSN) equal to ExpDataSN. However,
   targets may choose to send/receive all unacknowledged data or all
   of the data on a reassignment of connection allegiance if unable
   to recover or maintain accurate an state.

Nit: Should "an" be dropped?

7.14. Connection Failures

   iSCSI can keep a session in operation if it is able to
   keep/establish at least one TCP connection between the initiator
   and the target in a timely fashion. Targets and/or initiators may
   recognize a failing connection by either transport level means
   (TCP), a gap in the command sequence number, a response stream
   that is not filled for a long time, or by a failing iSCSI NOP
   (acting as a ping). The latter MAY be used periodically to
   increase the speed and likelihood of detecting connection
   failures. Initiators and targets MAY also use the keep-alive
   option on the TCP connection

I think TCP keep-alive option needs a reference here.

   to enable early link failure
   detection on otherwise idle links.

In 9.2.1:

The first mentioning of RADIUS needs an Informative reference.


In 9.3.1:

- HMAC-SHA1 MUST be implemented [RFC2404].

RFC 2404 seems to define HMAC-SHA-1-96, not HMAC-SHA1.

9.3.2. Confidentiality

   The NULL encryption algorithm MUST also be implemented.

I find it odd that the section talks about how weak DES is and then 
requires NULL encryption to be supported. What is the reason for this?

9.3.3. Policy, Security Associations, and Cryptographic Key
         Management

      - When digital signatures are used to achieve authentication,
        an IKE negotiator SHOULD use IKE Certificate Request
        Payload(s) to specify the certificate authority. IKE
        negotiators SHOULD check the pertinent Certificate
        Revocation List (CRL) before accepting a PKI certificate for
        use in IKE authentication procedures.

What are the reasons for these requirements being SHOULD level (as 
opposed to MUST level)?

   - The following identification type requirements apply to IKEv1.
     ID_IPV4_ADDR, ID_IPV6_ADDR (if the protocol stack supports
     IPv6) and ID_FQDN Identification Types MUST be supported;
     ID_USER_FQDN SHOULD be supported. The IP Subnet, IP Address
     Range, ID_DER_ASN1_DN, and ID_DER_ASN1_GN Identification Types
     SHOULD NOT be used. The ID_KEY_ID Identification Type MUST NOT
     be used.

It would be good to know the reason for the last SHOULD NOT and the last 
MUST NOT.


10.5. Synch and Steering Layer and Performance

   While a synch and steering layer is optional,

Is this section defining "synch and steering layer"? I found the use of 
this term confusing.

   an initiator/target
   that does not have it working against a target/initiator that
   demands synch and steering may experience performance degradation
   caused by packet reordering and loss. Providing a synch and
   steering mechanism is recommended for all high-speed
   implementations.

10.6.1. Determining the Proper ErrorRecoveryLevel

      Probability of transport layer "checksum escape".

Is this a well known term and if it is, what does it mean?


11.3.1. Flags and Task Attributes (byte 1)

      bit 3-4 Reserved.

Here and everywhere else in the document: what does it mean "reserved"?
Do you mean "MUST be set to zero when sent, MUST be ignored when received"?

11.4.4. SNACK Tag

    For a detailed discussion on R-Data SNACK see SNACK.

Is the last "SNACK" supposed to be a proper section reference?

11.4.5.3. SCSI REPORT LUNS and Residual Overflow

   If the Expected Data Transfer Length (EDTL) in the iSCSI header of
   the SCSI Command PDU for a REPORT LUNS command is set to at least
   as large as that ALLOCATION LENGTH, the SCSI layer truncation
   prevents an iSCSI Residual Overflow from occurring. A SCSI
   initiator can detect that such truncation has occurred via other
   information at theS CSI layer. The rest of the Section elaborates

s/theS CSI/the SCSI

   this required behavior.

11.9.2. AsyncVCode

    AsyncVCode is a vendor specific detail code that is only valid if
    the AsyncEvent field indicates a vendor specific event. Otherwise,
    it is reserved.

Is there an IANA registry for these?


11.13.8. Login Parameters

   Keys in Section 12, only need to be
   supported when the function to which they refer is mandatory to
   implement.

How is this decided?


12.1. AuthMethod

    Use: During Login - Security Negotiation
    Senders: Initiator and Target
    Scope: connection

    AuthMethod = <list-of-values>

    The main item of security negotiation is the authentication method
    (AuthMethod).

   The authentication methods that can be used (appear in the list-
   of-values) are either those listed in the following table or are
   vendor-unique methods:

Is there an IANA registry for this?


12.1.2. Secure Remote Password (SRP)

   For SRP [RFC2945], the initiator MUST use:

       SRP_U=<U> TargetAuth=Yes     /* or TargetAuth=No */

   The target MUST answer with a Login reject with the "Authorization
   Failure" status or reply with:

   SRP_GROUP=<G1,G2...> SRP_s=<s>

How are elements delemited on the wire?

12.1.3. Challenge Handshake Authentication Protocol (CHAP)

   For CHAP [RFC1994], the initiator MUST use:

      CHAP_A=<A1,A2...>

As above: How are elements delemited on the wire?

   To guarantee interoperability, initiators MUST always offer it as
   one of the proposed algorithms.

"Offer" or "support"? Most Apps protocols describe support, not 
necessarily requiring that
the mandatory-to-implement is enabled in a particular server.