Re: [secdir] Secdir review of draft-ietf-radext-dtls-06

Brian Weis <bew@cisco.com> Thu, 10 October 2013 16:00 UTC

Return-Path: <bew@cisco.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 E421021E813B; Thu, 10 Oct 2013 09:00:08 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -110.103
X-Spam-Level:
X-Spam-Status: No, score=-110.103 tagged_above=-999 required=5 tests=[AWL=-0.496, BAYES_00=-2.599, DATE_IN_PAST_12_24=0.992, RCVD_IN_DNSWL_HI=-8, USER_IN_WHITELIST=-100]
Received: from mail.ietf.org ([12.22.58.30]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 8r3rQ1Jd7HBk; Thu, 10 Oct 2013 09:00:01 -0700 (PDT)
Received: from mtv-iport-4.cisco.com (mtv-iport-4.cisco.com [173.36.130.15]) by ietfa.amsl.com (Postfix) with ESMTP id AB65121E8137; Thu, 10 Oct 2013 09:00:01 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=9133; q=dns/txt; s=iport; t=1381420801; x=1382630401; h=mime-version:subject:from:in-reply-to:date:cc: content-transfer-encoding:message-id:references:to; bh=jxukZeDOnqdjjuaKJ1Zq2FXh+sYEL6Di6Dg4mPs+hCo=; b=lElvY9ojRe0wH9i4NsmlilmL5kLIhc02ka7zEV+Qdn/pbQVCqoKI/R1S G9+gZs+0B5PXXCo1wPyNJvD+QEwq5TCuzzrxMG1UU1V+6yhAs08+cH0XE WR4sVYbslES7O+MvTm+zfwoUF4yMZAboVW4jAzNyYQvNaTQs3EIFo1wuZ 0=;
X-IronPort-Anti-Spam-Filtered: true
X-IronPort-Anti-Spam-Result: Ai0FAFnOVlKrRDoH/2dsb2JhbABZgwfCM4EjFnSCJQEBAQMBZxIFCwsYLlcGE4gABbopjxQzB4MfgQQDiTyOSYY4i0qDRByBLiQ
X-IronPort-AV: E=Sophos;i="4.90,1072,1371081600"; d="scan'208";a="94346945"
Received: from mtv-core-2.cisco.com ([171.68.58.7]) by mtv-iport-4.cisco.com with ESMTP; 10 Oct 2013 15:59:52 +0000
Received: from [10.154.161.207] ([10.154.161.207]) by mtv-core-2.cisco.com (8.14.5/8.14.5) with ESMTP id r9AFxpbb004972 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO); Thu, 10 Oct 2013 15:59:51 GMT
Content-Type: text/plain; charset="iso-8859-1"
Mime-Version: 1.0 (Mac OS X Mail 6.5 \(1508\))
From: Brian Weis <bew@cisco.com>
In-Reply-To: <5250BE14.2000005@deployingradius.com>
Date: Wed, 09 Oct 2013 20:38:19 -0700
Content-Transfer-Encoding: quoted-printable
Message-Id: <3385D735-1078-4DC4-8F39-91458BFE3907@cisco.com>
References: <43CA5AEB-DCB8-46FE-9176-D62CD511BADC@cisco.com> <5250BE14.2000005@deployingradius.com>
To: Alan DeKok <aland@deployingradius.com>
X-Mailer: Apple Mail (2.1508)
Cc: draft-ietf-radext-dtls.all@tools.ietf.org, The IESG <iesg@ietf.org>, "secdir@ietf.org" <secdir@ietf.org>
Subject: Re: [secdir] Secdir review of draft-ietf-radext-dtls-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: Thu, 10 Oct 2013 16:00:09 -0000

Hi Alan,

The new draft looks good. I just have one comment below.

On Oct 5, 2013, at 6:34 PM, Alan DeKok <aland@deployingradius.com> wrote:

> Brian Weis wrote:
>> I have the following suggestions to tighten up the document.
>> 
>> 1. Section 2.1, bottom of page 6. It is surprising that RADIUS/DTLS would cause a change to the RADIUS data packet handling. If item (1) is referring to the checks described in the RADIUS Length description (Section 3 of RFC 2865), I don't understand why the RADIUS description would change. RFC 2865 seems to clearly say that the RADIUS length check only covers bytes within the RADIUS data packet, and so the length carried in the UDP packet would not be relevant to RADIUS.
> 
>  DTLS is replacing UDP as a transport later for RADIUS.  So any text
> referring to UDP decapsulation should be replaced with DTLS decapsulation.
> 
>  RFC 2865 Section 3, top of page 15 says:
> 
>   ...  Octets outside the range of the Length field
>      MUST be treated as padding and ignored on reception.
> 
>  i.e. a UDP packet of 8K may only carry 32 octets of valid RADIUS data.
> The extra 8K-32 octets are discarded by the RADIUS layer.
> 
>> If a RADIUS packet is decapsulated from DTLS I would expect exactly the same situation. Is the use of a UDP packet length actually an implementation issue, or is there another RADIUS length check that I'm missing? (Note that if a change is made to this text, it might also need to be reflected in Section 2.2.1.)
> 
>  The text guides implementors to not be lazy.  The length of a RADIUS
> packet is determined by the RADIUS header, and *not* by the amount of
> data read from it's encapsulating protocol.

Thanks for the explanation.  Would it also be accurate to say this?

     (1) The Length checks defined in [RFC2865] Section 3 MUST treat the
      decrypted DTLS data octets outside the range of the Length field
      as padding and ignore it on reception.

I think something like that would be a lot clearer.

Thanks,
Brian

>> 2. Section 2.2.1, top of page 8. Is the statement "Client identities can be determined from TLS parameters" strong enough? Using the TLS parameters seems to be a much stronger statement, especially in the wildcarding case mentioned in the Security Considerations. Perhaps s/can be/SHOULD be/. 
> 
>  Good point.  I'll make it SHOULD.
> 
>> 3. Section 2.2.1. There's no comment on whether RFC 6614 Section 3.4 item (2) applies to RADIUS/DTLS. It would be good to be consistent and add something here.
> 
>  It does apply.  I'll add text to that effect.
> 
>> 4. Section 3.2. The second paragraph says:
>> 
>>  "Servers MUST NOT accept DTLS packets on the old RADIUS/UDP ports.
>>   Early drafts of this specification permitted this behavior.  It is
>>   forbidden here, as it depended on behavior in DTLS which may change
>>   without notice."
>> 
>> This is in line with WG consensus, so is correct. But there are some other sections that seem to conflict with this and probably need to be updated as well:
>> 
>> - Section 2.2.1 has three instances of "When DTLS is used over the existing RADIUS/UDP ports ...", which seems to be forbidden now. Should these all just say "Section 3.4 item (N) applies to RADIUS/DTLS"?
> 
>  Yes.  I'll fix it.
> 
>> - Section 5.1.1 says "The requirement to accept RADIUS/UDP and RADIUS/DTLS on the same port makes this recommendation difficult to implement in practice." This seems out of date.
> 
>  I've deleted the old text, and fixed the surrounding text.
> 
>> - Section 10 says "
>>  "The main portion of the specification that could have security
>>   implications is a servers ability to accept both RADIUS and DTLS
>>   packets on the same port.  The filter that disambiguates the two
>>   protocols is simple, and is just a check for the value of one octet.
>>   We do not expect this check to have any security issues."
>>  I assume this should be removed since accepting both on one port is forbidden. 
> 
>  Yes.
> 
>> 5. Section 5. I understand that RADIUS/DTLS creates connections where RADIUS/UDP had none, so there are some new considerations around keeping track of connections. But the section begins by saying that implementations are free to choose their method. Since the implementation details of this section aren't vital to understanding RADIUS/DTLS, and are frankly quite complex, it might be better organization to put the text in Section 6 ("Implementation Guidelines"). This would make it easier to understand the protocol details.
> 
>  Arguably yes.  I'm inclined to leave it as-is.  The existing text
> motivates and frames the discussion of connection management.
> 
>> 6. Section 5.1. Why is this a MUST? Is it required for interoperability? If not, I think it should be a recommendation.
>> 
>>  "A RADIUS/DTLS server MUST track ongoing DTLS client connections based
>>   on a key composed of the following 4-tuple...."
> 
>  I'm willing to make that change.  I'm curious as to how else the
> connections would be tracked.  Would a single DTLS session send packets
> using multiple source IPs / ports?  If so, how would the peer correlated
> a random packet to a particular session?
> 
>> 7. Section 5.1.1, bottom of page 11. What does the term "failed security" mean? Is there a more precise way to say this? 
> 
>  Perhaps "failing security requirements".  When a connection doesn't
> meet the security configurations set by the host, the connection should
> not be accepted.
> 
>> 8. Section 5.2.2, top of page 12. I assume the errors mentioned here are errors in the decrypted RADIUS data packet. Why is it necessary to mention these? Aren't they also necessary checks for RADIUS/UDP?
> 
>  When a RADIUS/UDP packet is malformed, it just means that the packet
> gets discarded.
> 
>  When you've set up a DTLS connection with someone, and he then sends
> you a malformed RADIUS packet, it means the entire DTLS session should
> be discarded.
> 
>  There was some discussion in the WG about this.  In the end, I can't
> see any reason to maintain a DTLS session when the other end isn't
> sending RADIUS packets inside of it.
> 
>> 9. Section 6. Can you cite a rationale for choosing these PSK key sizes? Do they represent a particular strength?
> 
>  They come from the WG discussion, if I recall.
> 
>> 10. Section 10. Having a fixed key always comes with threats. It would be helpful to mention that if the RADIUS/DTLS session becomes a RADIUS/UDP session due to configuration or implementation error, that the fixed RADIUS MD5 key would provide no security since the key was trivially known.
> 
>  I'll add some text.
> 
>> 11. Section 10.1. It would be fair to say that using either RADIUS/DTLS or RADIUS/TLS is recommended. I suggest adding "or RFC 6614" to the end of the 2nd sentence.
> 
>  OK.
> 
>> 12. Section 10.4. I don't think this document can make normative requirements on previous RFCs so "NOT RECOMMENDED" should probably be in lower case. I would also reword the second paragraph something like this to get the point across better: "The use of RADIUS/DTLS can allow for the safe usage of wildcards. When RADIUS/DTLS is used with wildcards clients MUST be uniquely identified using TLS parameters, and any certificate or PSK used MUST be unique to each client".
> 
>  OK.
> 
>> 13. Section 10.5. The first paragraph says "This requirement is due to security considerations.". Could you state them please?
> 
>  The next paragraph explains.
> 
> 
>> 14. Section 10.5. The second paragraph says:
>> 
>>  "Similarly, any RADIUS traffic failing validation
>>   means that two parties do not share the same security parameters, and
>>   the session is therefore a security risk."
>> 
>>  Aren't the security parameters all in DTLS? This seems like a security consideration of the normal operation of DTLS. DTLS is going to detect if there's a mismatch between the two endpoints and so there won't be a security risk. I think this sentence should be omitted.
> 
> Someone may allow administrator configuration of the shared secret
> inside of DTLS, instead of fixing it as "radius/dtls".
> 
> The two paragraphs should say:
> 
> 
> Section 5.1.1, above, requires that DTLS sessions be closed when the
> transported RADIUS packets are malformed, or fail the authenticator
> checks.  The reason is that the connection is expected to be used for
> transport of RADIUS packets only.
> 
> Any non-RADIUS traffic on that connection means the other party is
> misbehaving, and a potential security risk.  Similarly, any RADIUS
> traffic failing validation means that two parties do not have a common
> same shared secret, and the session is therefore unauthenticated.
> 
> 
>  I'll issue a new draft ASAP.