Re: [secdir] [Last-Call] Secdir last call review of draft-ietf-netconf-crypto-types-20

[Kent Watsen <[e-mail]>]

[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 <[e-mail]> wrote:
> 
> 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.

Perfect.


> 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).


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?


> 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.

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.


> 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.

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


> 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.

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?


> 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.

Replaced “unique” with “unpredictable”
Replaced “should” with 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

Egads - fixed!


>   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?

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>


> 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

Fixed!



Kent, as author