Subject: [IPsec] AD review comments for draft-ietf-ipsecme-traffic-visibility

From: <[email-masked]>
Date: Thu, 17 Sep 2009 15:05:04 +0200

I've now done my AD review for draft-ietf-ipsecme-traffic-visibility-08.  
I have two substantive comments, and a bunch of minor clarifications/nits.  
The substantive comments first:

- A question: did the WG discuss the pros and cons of integrity
protecting the WESP header? (This does make WESP more complex to
implement, and currently the WESP header does not contain any data
that would benefit from integrity protection in any way.)

- IPv6 requires extension headers to be aligned on 8-octet boundaries,
and I believe this requirement applies to ESP, too (see e.g. RFC 4303
Section 2.3, 2nd paragraph). All current ESP specs (all encryption
algorithms, UDP encapsulation, etc.) meet the 8-octet alignment
requirement -- but adding a new four-octet header there obviously
breaks it.
 
Some minor clarifications/editorial nits:

- The text currently uses "using ESP-NULL [RFC2410]" and "unencrypted"
as synonyms. This was accurate before RFC4543, but is not any more.
This needs some clarifying text somewhere (perhaps Section 1).

- Section 1 needs a sentence or two motivating the existence of the
"E" bit -- currently it comes as a surprise to the reader later.

- Section 2/2.1: In Figures 1, 2, and 3, the bit numbers should be 
shifted one character to the right.

- Section 2: In some parts of the text, the last 8 bits of the WESP
header are called "Flags"; in other parts, the last 5 bits are called
"Flags". I'd suggest changing e.g. Figure 2 so that last 5 bits are
called "Rsvd" or something.

- Section 2: "The bits are defined LSB first, so bit 0 would be the
least significant bit of the flags octet." This doesn't match the bit
numbering in Figure 2 (where bit 0 is the most significant bit).
However, the bit numbers are not really used anywhere, so I would just
suggest deleting this sentence.

- Section 2: It would be helpful if the text explicitly said that
HdrLen values less than 12 are invalid (and probably HdrLen values
that are not multiple of 4 are invalid, and multiple of 8 for IPv6
case).

- Section 2: the text about TrailerLen is a bit unclearly written --
the offset from the end of the packet to the last byte of the payload
would be a negative number. I'd suggest phrasing this something like
the "The number of bytes following the payload data in the packet, in
octets: i.e. the total length of TFC Padding (normally not present for
unencrypted packets), ESP Trailer (Padding, Pad Length, Next Header),
and ESP ICV."

- Section 2: "the packet must be dropped" -> "the packet MUST be dropped"

- The figures in 2.2.1 and 2.2.2 are very confusing, since they
suggest WESP could be applied as a separate step after ESP processing
(that was possible in some earlier versions of the draft, but not any
more since ICV covers the WESP header). Since they don't really
present much new compared to the figures in RFC 4303 Section
3.1.1/3.1.2, perhaps they could be omitted? Or if we want to keep
them, they probably should show the packet before applying ESP?

- Section 3: s/IPSec/IPsec/

- Section 4: this section is missing the allocation of SPI value 2 
to indicate WESP from the "SPI Values" registry.

- Section 4 should say that for the WESP Version Number, the
unassigned values are 1, 2, and 3.

- Section 6: [RFC4306], [RFC3948], and [RFC5226] should be normative
references, not informative.

Best regards,
Pasi

---

From: Tero Kivinen <[email-masked]>
Date: Fri, 18 Sep 2009 11:45:14 +0300

> that would benefit from integrity protection in any way.)

Thats is actually good question, as all of the data in the WESP header
is verified by comparing it to data elsewhere (either in packet (Next
Header) or data associated to SA (HdrLen, TrailerLen, flags)) already
I do not think if protecting WESP header adds anything.

I think that if we don't add it to ICV that could simplify
implementations as the ICV calculations would be exactly same as they
were before, so converting ESP -> WESP would be simple, by just
insertting new header and changing protocol number.

> suggest deleting this sentence.

I earlier proposed change that would add those bit numbers to the
text, so bit positions would be also given in other place than in the
picture. See my earlier message
http://www.ietf.org/mail-archive/web/ipsec/current/msg04761.html

> unencrypted packets), ESP Trailer (Padding, Pad Length, Next Header),

TFC Padding cannot be included in the TrailerLen as it can be too
large to expressed in 8 bits. Actually reading the description it
would indicate that the Padding, Pad Length and Next Header at the end
of packet would be included in the TrailerLen, which would mean that
even that might not be expressed in 8 bits in octects.

I originally only assumed it includes the ICV field length, nothing
else. That is the only thing needed by the intermediate device, as it
can see pad length and padding from the packet and it can also skip
the TFC padding in the same way recipient will do.

With the current wording the TrailerLen would be different for each
packet depending how many bytes of padding there is, which will mean
that final recipient needs to do some extra calculations while
verifying its length is correct (i.e it cannot simply compare the
TrailerLen to fixed value based on the SA (12 for HMAC-SHA1 etc), but
it needs to calculate Pad Length + ICV length + 2 + TFC Padding length
and see that it matches the TralerLen. 
-- 
[email-masked]

---

From: "Grewal, Ken" <[email-masked]>
Date: Fri, 18 Sep 2009 17:06:17 -0600

Hi Pasi, 
Many thanks for the great feedback. I will incorporate all these items as part of the WESP update during the next virtual interim meeting on Sept 22. Furthermore, I have opened multiple tickets to ensure these are tracked and resolved. 

Some comments inline...and others will result from the discussion during the interim meeting.

Thanks, 
- Ken

>-----Original Message-----
>From: [email-masked] [mailto:[email-masked]] On Behalf
>Of [email-masked]
>Sent: Thursday, September 17, 2009 6:05 AM
>To: [email-masked]
>Subject: [IPsec] AD review comments for draft-ietf-ipsecme-traffic-
>visibility
>
[Ken] This change was the result of a discussion on threats posed by 'malware', which could modify the WESP headers to obfuscate the payload from inspection by intermediate nodes such as IDS/IPS systems. 
The issue (ticket #104) was raised and closed some time back after lengthy discussions on the topic. 
http://trac.tools.ietf.org/wg/ipsecme/trac/ticket/104

>requirement -- but adding a new four-octet header there obviously

[Ken] Great point! Yes, this will need to be addressed and we do need to provide an extension of the header for alignment purposes in IPv6 usages. 
I have opened a new ticket (#109) to track this. 

>"E" bit -- currently it comes as a surprise to the reader later.

[Ken] I have reopened the related ticket #84 and we will generate / vet additional text to elaborate on the motivation.

[Ken] I have captured the items below in a single new ticket (#110) as most are minor editorial changes. 

>shifted one character to the right.
[Ken] OK

>called "Rsvd" or something.

[Ken] Agreed, using Rsvd is better than Flags. 
>suggest deleting this sentence.
[Ken] Agree. We already have new text in the next rev of the draft using MSB, as Tero had separately raised this point. 

>that are not multiple of 4 are invalid, and multiple of 8 for IPv6
[Ken] OK. 

>unencrypted packets), ESP Trailer (Padding, Pad Length, Next Header),
[Ken] Will discuss the scope of this, based on separate comment from Tero. 

>them, they probably should show the packet before applying ESP?

[Ken] Someone (do not recall who) had asked for these figures. But, I agree that it is misleading. So, propose we either remove these figures or change the 'before' diagrams to raw, instead of ESP.

>references, not informative.

[Ken] OK.
>
>Best regards,
>Pasi
>_______________________________________________
>IPsec mailing list
>[email-masked]
>https://www.ietf.org/mailman/listinfo/ipsec

---

From: Tero Kivinen <[email-masked]>
Date: Mon, 21 Sep 2009 14:13:45 +0300

Grewal, Ken writes:
> [Ken] This change was the result of a discussion on threats posed by
> 'malware', which could modify the WESP headers to obfuscate the
> payload from inspection by intermediate nodes such as IDS/IPS
> systems.  

As everything in the WESP header is something that can be verified by
the recipient node why is the integrity protection needed?

I think it would make implementation WESP much easier if it can be
done as post processing step after ESP has been applied, in a similar
way UDP encapsulation can be done to the ESP packet. 
-- 
[email-masked]

---

From: Yaron Sheffer <[email-masked]>
Date: Mon, 21 Sep 2009 15:40:19 +0300

Hi Tero,

Given that the existing ESP header is integrity-protected, I don't see the downside to adding the same protection for the new header. On the other hand, this would eliminate a whole class of vulnerabilities. We still have a few reserved bits in the WESP header, and you don't want to find out years down the road that they cannot be used because they're not protected in transit.

Thanks,
	Yaron

> From: [email-masked] [mailto:[email-masked]] On Behalf Of
> Tero Kivinen
> Sent: Monday, September 21, 2009 14:14
> To: Grewal, Ken
> Cc: [email-masked]; [email-masked]
> Subject: Re: [IPsec] AD review comments for draft-ietf-ipsecme-traffic-
> visibility
> 
> Grewal, Ken writes:
> Scanned by Check Point Total Security Gateway.

Email secured by Check Point

Email secured by Check Point