Subject: [netconf] Secdir last call review of draft-ietf-netconf-crypto-types-20

From: Valery Smyslov via Datatracker <[email-masked]>
Date: Tue, 24 Aug 2021 11:29:43 -0700

Reviewer: Valery Smyslov
Review result: Has Issues

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.

When I was re-assigned to review this draft the indicated deadline was already
missed by 8 months, so I don't know how relevant the review is. I reviewed
the latest -20 version of the draft instead of -18, that was requested.

The draft defines common YANG data types and groupings useful for cryptography.
I din't try to check the YANG module itself. 

Issues:

Shouldn't a Privacy Considerations section be added to the draft?
The draft defines quite a lot of privacy-sensitive information (like certificates)
with no restriction on read access (as far as I understand).

Section 3.5.
While I understand and support the idea, expressed in this section, I think that
the way it is expressed makes it difficult to follow in practice. In general, it's
not always obvious how to estimate the "strength" of the underlying secure transport.
For this reason it's not clear for me how it is supposed to "compare" the 
"strength" of the transport with the "strength" of the keys being transported.

In addition, the requirement, that "Implementations SHOULD fail the write-request if ever
the strength of the private key is greater then the strength of the
underlying transport" looks wrong to me. You don't need to have
1024 bits transport protocol strength to transfer 1024 bit key, since
even for say 256 bits it's infeasible to break.

I think that the better approach would be to advise using strong
ciphersuites for transport protocols defined in corresponding RFCs.
For example, for TLS 1.3 there are ciphersuites marked as "recommended",
that were evaluated by IETF crypto community.

Section 3.6:
   For instance, AES
   using "EBC" SHOULD NOT be used to encrypt passwords, whereas "CBC"
   mode is okay since it a unique initialization vector (IV) should be
   used for each run.

Not only IV for CBC should be unique, it should be unpredictable (random or pseudo-random).
I also think that lowercase "should" here must be uppercase, or even MUST.

Typos, nits:

Section 3.6: 
   In order to thwart rainbow attacks, algorithms that result in a
   unique output for the same input SHOULD be used. 

s/SHOULD/SHOULD NOT

   For instance, AES
   using "EBC" SHOULD NOT be used to encrypt passwords, whereas "CBC"
   mode is okay since it a unique initialization vector (IV) should be
   used for each run.

s/EBC/ECB
s/it//
I believe "okay' is a bit of slang, isn't it?

Section 3.8:
   Since the module in this document only define groupings, these
   considerations are primarily for the designers of other modules that
   use these groupings.

s/define/defines

---

From: Kent Watsen <[email-masked]>
Date: Fri, 27 Aug 2021 14:53:14 +0000

[removing “last-call” alias]

Hello Valery.  

Thank you for your SecDir review, it is very much appreciated and never too late!

More comments below.

Thanks,
Kent

> On Aug 24, 2021, at 2:29 PM, Valery Smyslov via Datatracker <[email-masked]> wrote:
> 
> Reviewer: Valery Smyslov

Perfect.

> with no restriction on read access (as far as I understand).

Both the "trust-anchor-cert-grouping” and “end-entity-cert-grouping” groupings have "nacm:default-deny-write” that, to your point, does not restrict reads.  That said, only management protocols having mutual authentication (e.g., SSH and/or TLS based transport) can access the data.  

Is your concern that the certificate’s content would be visible to the administrators?  Is your comment on end-entity certificates (containing personally-identifying information), more than trust-anchor-certificates?

> "strength" of the transport with the "strength" of the keys being transported.

I saw language like this once in a DoD setting.  I agree that it is difficult to implement in practice.  I used “SHOULD” (not MUST) to buy some leeway for implementations to be compliant.  Makes sense?

FWIW, my YANG-driven server is able to remember what key the client used for authentication (e.g., RSA 2048) and register a callback to test that no greater keys (e.g., 3072 or 4096) are configured by that client.  Additional logic would be needed to prevent a low-strength client from *reading* a high-strength key configured by another client…though the issue can be alternatively resolved by configuring the TLS-stack to prevent low-strength algorithms.

> even for say 256 bits it's infeasible to break.

IDK about this.  Again, I saw this constraint once in a DoD setting.  

> that were evaluated by IETF crypto community.

I added this sentence:

	Implementations SHOULD configure allowed transport 
	algorithms to include only those meeting local
	policy (e.g., listed as "recommended" by the IETF).

Good?

> I also think that lowercase "should" here must be uppercase, or even MUST.

Replaced “unique” with “unpredictable”
Replaced “should” with MUST.

>   unique output for the same input SHOULD be used. 

Egads - fixed!

> I believe "okay' is a bit of slang, isn't it?

Fixed, now reads:

	In order to thwart rainbow attacks, algorithms that result
	in a unique output for the same input SHOULD NOT be used.
	For instance, AES using "ECB" SHOULD NOT be used to
	encrypt passwords, whereas "CBC" mode is permissible
	since an unpredictable initialization vector (IV) MUST be
	used for each use.</t>

>   considerations are primarily for the designers of other modules that

Fixed!

Kent, as author

---

From: "Valery Smyslov" <[email-masked]>
Date: Fri, 27 Aug 2021 19:14:30 +0300

Hi Kent,

thank you for addressing my comments. A bit more inline.

 

Hello Valery.  

More comments below.

Thanks,

Kent

Reviewer: Valery Smyslov

Perfect.

Issues:

 

          Yes, it’s mostly on end-entity certificates, however there may be quite a lot of interesting

          private information besides certificates.

          If this information is only visible to the administrators and the used management protocols must 

          have mutual authentication, then it’s probably not a big deal. I would have still added

          a sentence about privacy of the stored data (i.e. that persons, that are allowed to access this data

          are able to learn quite a lot of private information from it). I don’t insist though, it’s up to you.

Section 3.5.

          My understanding of using RFC2119 language is that SHOULD is very close to MUST,

          but allows some exceptions. So, I still think that you put a responsibility to make

          security-related decisions on implementers, who often are not experts in this area.

FWIW, my YANG-driven server is able to remember what key the client used for authentication (e.g., RSA 2048) and register a callback to test that no greater keys (e.g., 3072 or 4096) are configured by that client.  Additional 

          What if the other key configured for the client is X25519? Which is stronger?

logic would be needed to prevent a low-strength client from *reading* a high-strength key configured by another client…though the issue can be alternatively resolved by configuring the TLS-stack to prevent low-strength algorithms.

          That was my point. I think that it’s better to require (by SHOULD) that only

          those ciphersuites that were “vetted” by IETF (i.e. got “Recommended” status) be used.

          This will make implementers’ life easier.

          My (another) point was that there is generally no point to increase

          security strength beyond some level. Currently it is believed

          that 128 bit of symmetric key is infeasible to break (provided the algorithm is not broken itself),

          If you are lucky have full-sized Post Quantum computer, it’ll be 256 bits.

          It’s enough to transfer symmetric keys with say 1024 bits of entropy

          (FWIW). So the requirement that the strength of transport must

          be always greater than the strength of transported key 

          seems not a good requirement to me. Instead require that

          the strength of transport be sufficient to make

          infeasible for an attacker to break it.

I added this sentence:

Good?

          Perfect.

          Regards,

          Valery.

Section 3.6:
  For instance, AES

 

Typos, nits:

Section 3.6: 

s/SHOULD/SHOULD NOT

Egads - fixed!

  For instance, AES

s/EBC/ECB
s/it//

Fixed, now reads:

          used for each use.</t>

Section 3.8:

s/define/defines

Fixed!

Kent, as author

---

From: "Valery Smyslov" <[email-masked]>
Date: Fri, 27 Aug 2021 19:19:41 +0300

Spotted a typo in own message – of course it was meant Quantum computer, not a Post Quantum one

(I have no idea what Post Quantum computer may mean :-), I know only Post-quantum cryptography).

---

From: Kent Watsen <[email-masked]>
Date: Fri, 27 Aug 2021 20:50:53 +0000

Hi Valery,

> On Aug 27, 2021, at 12:14 PM, Valery Smyslov <[email-masked]> wrote:
> 
> Hi Kent,
>  

All good.  

> On Aug 24, 2021, at 2:29 PM, Valery Smyslov via Datatracker <[email-masked] <mailto:[email-masked]>> wrote:
>  
> Reviewer: Valery Smyslov

I added the following to Section 3.8 (The "ietf-crypto-types" YANG Module).

             The "cert-data" node:

                   The "cert-data" node, defined in both the "trust-anchor-cert-grouping"
                    and "end-entity-cert-grouping" groupings, is additionally sensitive to
                    read operations, as certificates sometimes convey personally identifying
                    information (especially end-entity certificates).  However, as it is
                    commonly understood that certificates are "public", the NACM extension
                    "nacm:default-deny-write" (not "default-deny-all") has been applied. It
                    is RECOMMENDED that implementations adjust read-access to certificates
                    to comply with local policy.

Is this okay?

Separately, I thought about if there are any other values in the module that may have privacy concerns but was unable to locate any.

>> "strength" of the transport with the "strength" of the keys being transported.

All comments from this point to the end regard the Security Consideration "Strength of Keys Conveyed” (was "Strength of Keys Configured”).  I rewrote the section as follows.  Can you please check for accuracy?

      Strength of Keys Conveyed

           When accessing key values, it is desireable that implementations
            ensure that the strength of the keys being accessed is not greater
            than the strength of the underlying secure transport connection
            over which the keys are conveyed.  However, comparing key strengths
            can be complicated and difficult to implement in practice.

           That said, expert Security opinion suggests that already it is
            infeasible to break a 128-bit key using a classical computer, and 
            thus the concern for conveying higher-strength keys begins to lose 
            its allure.

            Implementations SHOULD only use transport algorithms to those 
            meeting local policy.  A reasonable policy may, e.g., state that 
            only algorithms listed as "recommended" by the IETF be used.
            Another reasonable policy may be to only use quantum-resistant 
            algorithms.

Thanks!
Kent, as author

---

From: "Valery Smyslov" <[email-masked]>
Date: Tue, 31 Aug 2021 10:56:00 +0300

Hi Kent,

please. see inline.

Hi Valery,

Hi Kent,

All good.  

 

Hello Valery.  

More comments below.

Thanks,

Kent

On Aug 24, 2021, at 2:29 PM, Valery Smyslov via Datatracker < <mailto:[email-masked]> [email-masked]> wrote:

Reviewer: Valery Smyslov

Perfect.

Issues:

 

 

             The "cert-data" node:

Is this okay?

          Yes, thanks.

          certificate-signing-request?

Section 3.5.

          s/key/symmetric key/

          s/transport algorithms/secure transport/

          s\algorithms/ciphersuites/

          Works for me with changes above. I would only add a few words at the end of the second para that 
          things may change in the future (e.g. if full-size quantum computers appear),
          so it is recommended to follow up-to-date advise from crypto community
          when protecting transport channel.

          I would also remove the last sentence in the last para, mostly because
          it’s difficult to follow in practice (we still know not much about post-quantum crypto
          and generally it’s not yet widely supported in protocols like TLS) and instead reference RFC 7525
          which contains recommendations how to use TLS in applications.
          I don’t know in similar RFC exists for SSH, sorry...

          Regards,
          Valery.

Thanks!

Kent, as author

I added this sentence:

Good?

          Perfect.

          Regards,

          Valery.