Re: [Sidrops] Benjamin Kaduk's No Objection on draft-ietf-sidrops-rpki-tree-validation-02: (with COMMENT)

[Oleg Muravskiy <[e-mail]>]

Hi Benjamin,

Thanks for your comments. I am updating the draft, please see my inline comments below:

> On 28 Aug 2018, at 18:35, Benjamin Kaduk <[e-mail]> wrote:
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> This document is Informational, so I will make my comments non-blocking,
> but I think that the security considerations could be improved.  It
> currently does a good job talking about cases where the procedure can
> receive inconsistent inputs (and how it behaves in the face of those
> inputs), as well as some other considerations, and this is great!  I think
> that the discussion could be more powerful if it also considered how those
> inconsistent inputs could be produced, in particular which capabilities an
> attacker could have.  There seem to be roughly three classes of actors for
> active attacks -- an attacker in the network could modify the transferred
> content (including number of files and directory layout) over unecrypteed
> rsync or http, an attacker that compromises the webserver's certificate
> (or obtains a fraudulent one) could do the same over encrypted https, and
> an attacker that compromises the TA signing key could produce
> fake-but-"valid" manifests, CRLs, etc..  (Is there more that could be done
> with a compromised non-TA CA?  The potential hazards to this algorithm
> don't seem to be noticably distinct, though.)  Some consumers may be
> willing to trust in the TA's integrity or even the Web PKI and not worry
> about those risks.  Though there is always risk of accidental error, of
> course, and the current coverage in this document seems adequate for that
> risk.

In the Security Considerations we tried to describe issues specific to our implementation.
What you suggest seems to be a general discussion of vulnerabilities in the RPKI repository architecture that is defined by current standards. I agree that it is missing in the current set of RFCs and it would be great to have it, but on the other hand I do not think it should be done it this document. It would also require another round of going through the WG discussion, I believe. So I left this section as is.

> Some additional section-by-section comments follow.
> 
> Section 3.1
> 
> The key properties needed of cryptographic hash functions are either
> "collision resistance" or "second preimage resistance", depending on
> whether the attacker gets to pick both hash inputs (the first case) or only
> one of them (the second).  Which one is needed here would probably depend
> on whether the CA/issuer is trusted to not be an attacker or not, but
> regardless, please use the more detailed terminology.

I’m going for the “collision resistance properties” in that paragraph.
But also note that it isn’t only about attacks on the repository objects: if there are legitimate objects anywhere in fetched repositories that produce the same hashes, they will collide in our implementation.

> Section 3.2
> 
>   [...], or use all objects whose AKI extension
>   matches the Subject Key Identifier (SKI) extension (Section 4.2.1 of
>   [RFC5280]) of a CA certificate.
> 
> "all objects" as discovered within what scope?

Essentially these are all objects known to a validator at the time the comparison is performed.
I’m changing the draft to read “all known repository objects”.

> Section 4.2
> 
> Please expand SIA on first usage.

ACK

> In bullet point 1, it might help the uninitiated reader to say something
> after "and pass it to the object fetcher" to note that "the fetcher will
> then fetch all objects available from that repository”.

ACK

> In bullet point 3, please be more clear about which manifest is "this
> manifest object" that is used to continue validation processing.

ACK

>   4.  Perform manifest entries discovery and validation as described in
>       Section 4.2.2.
> 
> nit: "manifest entry discovery" (singular "entry”)

The manifest contains an entry per object in the publication point, so there are at least 2 entries – for a CA cert and a manifest.
I guess plural “entries” is appropriate here.

>       (Note that this implementation uses the operator configuration to
>       decide which algorithm to use for path validation.  It applies
>       selected algorithm to all resource certificates, rather than
> 
> nit: "the selected algorithm”

ACK

> Please expand ROA on first usage.

ACK

> Section 5.1.1, 5.1.2
> 
> The syntactic verification performed here is done on what should be
> considered "untrusted input", which means that the verification code needs
> to be written in a robust manner.  (Given the historical recurrences of,
> e.g., ASN.1 decoder security vulnerabilities, we probably need to
> explicitly state this in the security considerations.)

Well, I could say that the validation “is written and performed in a robust manner”, but I do not see how this will improve the content...

> Section 9.1
> 
> If I understand correctly, RFC 6485 allows for (and predicts the need for)
> hash agility in the file hash algorithm.  In this case, it would probably
> be appropriate to say something about how "the security of the system as a
> whole is limited to that of the weakest hash function allowed by consumers,
> but the hash agility provided for by RFC 6485 allows new (stronger) hashes
> to be introduced and old hash functions phased out before they are
> critically broken”.

The sentence you propose describes the current situation of the RPKI in general, it is not specific to our implementation, or to RPKI tree validation. 

RFC 6485 said:
   The recommended procedures to implement such a
   transition of key sizes and algorithms is not specified in this
   document.

As such, our validator does not support any other algorithms for keys and hashes.
By now, however, RFC 6485 has been obsoleted by 7935, which refers to RFC 6916 for details of algorithm agility.

I’m adding a sentence that agility is not supported in our implementation.

> Section 9.2
> 
>   In case of a mismatch described above this implementation will not
>   exclude an object from further validation merely because it's actual
> 
> nit: "its" (no apostrophe)

ACK

> Section 9.3
> 
> nit: Which behavior is allowed-but-not-required -- a CA signing things not
> listed in the manifest, or an RP ignoring things not listed in the
> manifest?  I suggest "This RP behavior is allowed [...]”.

ACK

> Section 9.4
> 
> This kind of behavior would be seen as an unacceptable vulnerability in a
> standards-track protocol, though since this document is only informational
> it does not block publication.

From the RP perspective it is not possible to know whether the object it received from the remote repository is genuine or tampered with. Current standards require that invalid objects must not be used for validation, and this is what our implementation does. There is no requirement to keep and use an “old” object (whatever that might be) if the “new” one is not valid.


> Section 9.5
> 
> It might be useful to reference Section 5.1.1 and how we sometimes fetch
> the entire repository contents, not limited by a manifest listing.

ACK


Cheers,
Oleg