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

From: Oleg Muravskiy <[email-masked]>
Date: Mon, 3 Sep 2018 00:19:38 +0200

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 <[email-masked]> 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

---

From: Benjamin Kaduk <[email-masked]>
Date: Tue, 4 Sep 2018 12:08:09 -0500

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

I can't fault you for that.  Do you think there is any energy to put into a
standalone document that covers these topics?

> 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 imp…

Of course.  Even with the "broken" SHA-1 hash, though, such accidental
collisions (with different body content) should be vanishingly rare.

> I guess plural “entries” is appropriate here.

Ah.  I would suggest "discovery of manifest entries", then, though of
course the RFC Editor would probably have some suggestsions as well.

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

I was thinking more along the lines of a note in the security
considerations:  "The syntactic validation steps performed in Sections 5.1.1
and 5.1.2 are operating on untrusted input, so particular care should be
taken to avoid buffer overflows and similar attacks."  But this is just a
suggestion and I won't be offended if you decide to not use it.

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

Okay.  That should give the reader a place to start, if they're interested
(please informatively cite 6916, too).

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

Allowing untrusted data that is not authenticated (e.g., by a digital
signature) to overwrite existing data in the store that has been
authenticated, is something I would not want to see in an IETF protocol.
It would only be allowed after the latest manifest has been authenticated
and fully processed, and when the "old" resource is no longer referenced by
the manifest.  This is a matter of the order of operations for processing,
more than a perceived requirement to retain an "old" object that is no
longer referenced.  But, since we are not objecting to the current text,
feel free to ignore this discussion point.

-Benjamin

---

From: Oleg Muravskiy <[email-masked]>
Date: Wed, 5 Sep 2018 09:56:46 +0200

HI Benjamin,

> On 4 Sep 2018, at 19:08, Benjamin Kaduk <[email-masked]> wrote:
> 

Maybe :)

> course the RFC Editor would probably have some suggestsions as well.

Sounds good

> suggestion and I won't be offended if you decide to not use it.

Well, this document describes the existing implementation, not how it should be done.
But there is https://datatracker.ietf.org/doc/draft-ietf-sidrops-rp/, I think it would fit there quite well.

> (please informatively cite 6916, too).

OK

> feel free to ignore this discussion point.

OK, thanks for your input.
I do agree with that, and I think it would not be difficult to fix it in our code and make a maintenance release.

— 
Oleg

---

From: Benjamin Kaduk <[email-masked]>
Date: Wed, 5 Sep 2018 16:22:51 -0500

> > standalone document that covers these topics?

I'd be happy to see one, though I probably could not be the driving force
myself.

> But there is https://datatracker.ietf.org/doc/draft-ietf-sidrops-rp/, I think it would fit there quite well.

Fair enough.  Do I need to send mail or make a pull request or anything
like that, or are the wheels already moving on this one?

Thanks,

Benjamin

---

From: Oleg Muravskiy <[email-masked]>
Date: Mon, 17 Sep 2018 22:40:27 +0200

> like that, or are the wheels already moving on this one?

There’s been a call for input: https://mailarchive.ietf.org/arch/msg/sidrops/zs0-pbiIVzeuhINRbYuBBpXhLME

>> I do agree with that, and I think it would not be difficult to fix it in our code and make a maintenance release.

I made changes to our code and the new release will come soon. So I removed this section from the -03 version of the draft.

Cheers,
Oleg