Re: [Lwip] [IPsec] The LWIG WG has placed draft-mglt-lwig-minimal-esp in state "Call For Adoption By WG Issued"

[Daniel Migault <[e-mail]>]

Hi Paul, 

Thanks for the review. From your response [1] I understand we can proceed to the adoption of the document if no changes are performed on ESP. This condition is fully stated in the abstract [2] as well as in the charter [3].  As a result, I believe the draft is ready for adoption. Of course we welcome further reviews!

Please find inline my responses to the current review. If the changes address your concern, I am happy to update the draft with the proposed text. I am happy to take more reviews as you volunteered to provided further reviews. 😉

For full transparency, my understanding of the current reviews is that they will result in minor -- thought useful -- changes to the current draft. The concerns you expressed are in my opinion mostly due to a confusion with another draft "ESP Header Compression and Diet-ESP" (draft-mglt-ipsecme-diet-esp). This draft is discussed in ipsecme not in lwig and so are sort of out of scope. All comments are answered below.

Yours, 
Daniel

[1]
"""
If the document is defining a minimum/battery optimized ESP configuartion, I have no problems with it and I will review further text and welcome adoption. If it makes changes to the ESP protocol, then I think there should be more discussion before adoption.
"""

[2]
"""
  This document describes a minimal implementation of the IP
   Encapsulation Security Payload (ESP) defined in RFC 4303.  
[...]
  This document does not update or modify RFC 4303, but provides a
   compact description of how to implement the minimal version of the
   protocol.  If this document and RFC 4303 conflicts then RFC 4303 is
   the authoritative description.
"""

[3]
"""
"LWIG is chartered to provide guidance on lightweight implementations. 
Almost all drafts in this WG are informational in nature and we don't intend 
to change existing standards or define new ones.
"""

[4] https://tools.ietf.org/html/draft-mglt-ipsecme-diet-esp-06

-----Original Message-----
From: Paul Wouters <[e-mail]> 
Sent: Wednesday, February 13, 2019 1:09 PM
To: Daniel Migault <[e-mail]>
Cc: Mohit Sethi M <[e-mail]>; Tero Kivinen <[e-mail]>; Heinrich Singh <[e-mail]>; [e-mail]
Subject: Re: [Lwip] [IPsec] The LWIG WG has placed draft-mglt-lwig-minimal-esp in state "Call For Adoption By WG Issued"

On Tue, 12 Feb 2019, Daniel Migault wrote:

> I am wondering what is currently the status of the draft. Feel free to let me know if I something is expected on my side.

I cannot answer that question, I'll leave that to the chairs, but I do have several strong reservations on the document. Especiallt with the complex framework to reduce a number of bytes.

<mglt>
I understand that the strong reservations concern the modification of ESP as well as the compression framework. Such reservations do not apply here. I believe this statement is clearly mentioned in the abstract [1] as well as in the lwig charter[2].  I think what you're referring to is another draft in ipsecme "ESP Header Compression" (draft-mglt-ipsecme-diet-esp) [3] which does not apply here.  We think the abstract of the document states that clearly, but we're happy to state this point stronger if you feel that's necessary.

To be clear:
* draft-mglt-lwig-minimal-esp does not modify ESP
* draft-mglt-lwig-minimal-esp  does not reduce any bytes of ESP
* draft-mglt-lwig-minimal-esp does not describe any framework.

[1]
"""
  This document describes a minimal implementation of the IP
   Encapsulation Security Payload (ESP) defined in RFC 4303.
[...]
  This document does not update or modify RFC 4303, but provides a
   compact description of how to implement the minimal version of the
   protocol.  If this document and RFC 4303 conflicts then RFC 4303 is
   the authoritative description.
"""

[2]
"""
If the document is defining a minimum/battery optimized ESP configuartion, I have no problems with it and I will review further text and welcome adoption. If it makes changes to the ESP protocol, then I think there should be more discussion before adoption.
"""

[3] https://tools.ietf.org/html/draft-mglt-ipsecme-diet-esp-06
</mglt>

In section 3 there is a discussion about not using randomness for a SPI.
It argues why this is not needed. My issue is that there I can see three ways of a system needing to generate an IPsec SPI:

1) It has an IKEv2 stack
2) It has nother (eg 3GPP??) protocol that does not provide a SPI, but
    handles all other keying.
3) Manual keying

In the case of 1) the implementation clearly needs to generate randomness anyway, at least for the IKE SPIs which really must be strong random. (It saved us from an IKEv1 attack 2 years ago!)

In the case of 2), why isn't the other protocol dictating the SPI?

As for case 3), well manual keying is strongly discouraged outside of using a keying protocol that provides the same safeguards as IKE.

So I don't know which implementations would actually qualify for using non-random static like SPI's

Perhaps you can clarify this with some use cases where this would apply?

<mglt>
I believe the current text [4] already provides some use cases and obviously we do not expect these device to have IKEv2 or keying negotiation facilities implemented. Again, the use of ESP does not mandate IKEv2 or equivalent even though this is recommended strongly. We believe that very constraint device would benefit from having fixed SPI rather than no security. The current draft documents how such this category of device can secure their communications.

Note also that mandating random SPI would represent a change in the ESP specification which is again not intended by the current document nor the charter. In addition, I believe the text is clear enough that random SPI is recommended whenever it is possible and that non random SPI are reserved for very constraint nodes. If the text does not provide this impression, feel free to let us know how to make it clearer.

[4]
"""
   As the general recommendation is to randomly generate the SPI,
   constraint device that will use a limited number of fix SPI are
   expected to be very constraint devices with very limited
   capabilities, where the use of randomly generated SPI may prevent
   them to implement IPsec.  In this case the ability to provision non
   random SPI enables these devices to secure their communications.
   These devices, due to there limitations, are expected to provide
   limited information and how the use of non random SPI impacts privacy
   requires further analysis.  Typically temperature sensors, wind
   sensors, used outdoor do not leak privacy sensitive information.
   When used indoor, the privacy information is stored in the encrypted
   data and as such does not leak privacy.
"""
</mglt>

Section 4 is about the sequence number. While in principle i see nothing wrong with relaing the requirement for increasing by 1 and using another source for increasing numbers, I do find the reasoning weak. For example, with this counter you would also check the number of bytes encrypted with that key to ensure you are not encrypting more than 2^20 or 2^32 packets. While I guess these devices likely wouldn't ever get there, shouldn't there still be a formal check in the code to prevent this anyway?

<mglt>
The document does not provide any recommendations on how to handle the SN. It just states that generally the counter is incremented by one but other means may be used. 

"""
  Usually, SN is generated by incrementing a counter for each packet
   sent.  A constraint device may avoid maintaining this context and use
   another source that is known to always increase.  
"""

The comment provides an additional example of counter that could be re-used. I propose to add that example at the end of the paragraph with the following sentence.

""" Similarly, the SN make be handled as a counter used for other purposes. For example, an implementation may also use the number of bytes encrypted as the SN in order to enforce the management of the key lifetime of the key. The SN may also be used to generate the IV when AES-CTR [RFC3686] AES-GCM [RFC4106], AES-CCM [RFC4309] or Chacha20-Poly1035 [RFC7634] is used. Note that in this later both SN and IV are sent explicitly in the ESP packet. [draft-ipsecme-implicit-iv] defines transforms where the IV is derived from the SN, which avoids the IV to be sent on the wire.    
"""

You are correct that this parameter is already part of the SA and for device with no key management facilities, we hardly expect them to reach this limit nor to be able to rekey. 
</mglt>

Additionally, if resources matter that much, you are using AES-GCM or CHACHA20POLY1305, meaning you need a counter anyway. So you can just re-use that one anyway. So I'm not convinced this is actually a needed use case.

Section 5, while TFC deployment might be used a lot (yet?), it is part of many stacks, so the claim about not being widely adopted for standard ESP traffic is partially true.

I am not sure what you are trying to say with this paragraph:

    As a result, TFC cannot not be enabled with minimal, and
    communication protection that were relying on TFC will be more
    sensitive to traffic shaping.  This could expose the application as
    well as the devices used to a passive monitoring attacker.  Such
    information could be used by the attacker in case a vulnerability is
    disclosed on the specific device.  In addition, some application use
    - such as health applications - may also reveal important privacy
    oriented informations.

Are you suggesting to obsolete TFC ?

<mglt>
No. we are saying that not enabling TFC with minimal implementation is correct. TFC is hard to manage and devices that are looking at minimizing the number of bytes sent over the air will not enabled it.  The sentence has a nit and should be:

OLD:
    As a result, TFC cannot not be enabled with minimal, and

NEW:
    As a result, TFC is not enabled with minimal ESP, and

</mglt>

Section 5 does not actually propose a change that I can see. It implies padding support and any and all kind of padding should be able to be omited?

<mglt>
To remain compliant padding is performed in a given way and other paddings are accepted. This is to remain compliant with ESP and avoid minimal ESP implementation to perform padding in alternative ways as defined by ESP. 
</mglt>

Similarly, section 6 does not seem to propose a change to strip out the Next Header, though it is suggesting it. I think BEET is not a real consideration, as I dont think many implementations support it and I don't know anyone using it? But I'm not convinced this 1 byte is really a goal we should consider.

<mglt>
Stripping the next header would change ESP. This is out of scope of the draft and have no intention to suggest this. I do not find any such suggestion in the text. Please let us know what make you think so, so that we clarify this.
</mglt>

This sentence is confusing:

 	ESP can be used to authenticate only or to encrypt the communication.

Since IPsec-v2 allowed ESP without authentication, and IPsec-v3 only has authenticated ESP. It's better to say ESP allows null-encryption and not mention authentication (which always happens)

<mglt>
I would argue the current text is clearer than the proposed text, but I am happy to change it. I believe this is clearer to say that we are willing to authenticate the communications than to say we are encrypting it with the NULL encryption. Null encryption is the way to performed authentication only - not the intent. The following sentence insists also on providing authentication all the time. Again English is not my natural language and I am happy to be told otherwise. 

"""
      ESP can
       be used to authenticate only or to encrypt the communication.  In
       the later case, authenticated encryption must always be
       considered [RFC8221].
"""
</mglt>

It talks about "Cipher Suites" which is really a TLS term.

<mglt>
We can change "Cipher Suites" to a more appropriated term. Would you suggest instead the use of Encryption Algorithm Transform ? 
</mglt>

        For example if the device
        benefits from AES hardware modules and uses AES-CTR, it may
        prefer AUTH_AES-XCBC for its authentication.

Note that AES-XCBC is not a FIPS approves integrity algorithm :) But more importantly, you do not want a non-AEAD at all if battery usage is so important. use AES-CCM or AES-GCM or when not having AES HW support, chacha20-poly1305.

<mglt>
The idea was to replace the usage of hashing functions by a function using HW support. However, the draft should not provide specific algorithms and instead rely on specific recommendations (RFC 8221) that will be updated over time. If following the up-to-date cryptographic recommendations is not clear, please feel free to let us know how to clarify this.
</mglt>



All in all, I think the document should more clearly seperate the issues of a minimal ESP implementation, and any proposed modifications to ESP.
And if that is done, the protocol shouldn't be ESP but something new, unless it is completely backwards compatible (like IPsec-v2 to->
IPsec-v3 was)
<mglt>
We are proposing NO CHANGES to ESP. We are then completely backwards compatible as well. I believe this has been clearly stated in the abstract. 
</mglt>

If the document is defining a minimum/battery optimized ESP configuartion, I have no problems with it and I will review further text and welcome adoption. If it makes changes to the ESP protocol, then I think there should be more discussion before adoption.

<mglt>
There is no changes to ESP. Any change to ESP would have been discussed in ipsecme. We more welcome your feed back to improve the text.. 
</mglt>

Paul