Handle errors in different phases differently #83

dcreager · 2018-06-27T15:18:19Z

This patch clarifies the different phases that happen while servicing a request. A network error occurs during one of those phases, and we now handle errors differently depending on which phase they occur in:

include_subdomains policies can only be used to generate reports about DNS resolution errors, since the policy author can only confirm ownership of the DNS tree, and not of all of the servers that those domain names resolve to.
If the resolved IP address for an origin changes between when a policy is received, and when its used to generate a report, we don't report any details about the connection and application phases, and only report that the IP address changed. This prevents DNS rebinding attacks.

This patch clarifies the different phases that happen while servicing a request. A network error occurs during one of those phases, and we now handle errors differently depending on which phase they occur in: - `include_subdomains` policies can only be used to generate reports about DNS resolution errors, since the policy author can only confirm ownership of the DNS tree, and not of all of the servers that those domain names resolve to. - If the resolved IP address for an origin changes between when a policy is received, and when its used to generate a report, we don't report any details about the connection and application phases, and only report that the IP address changed. This prevents DNS rebinding attacks.

jyasskin · 2018-07-02T20:40:17Z

index.html

+      <p>
+      Regardless of which fetch algorithm and which underlying application and
+      transport protocols are used, servicing a <a>network request</a> consists
+      of the following <dfn data-lt="phase">phases</dfn>:


I'd love if these phases were defined in a more central place, like Fetch. That'd make it more likely that specs like the Web Packaging specs remember to define which phase they're operating in.

I've added an editor's note that we'd like to move these into Fetch. I'd like to keep the definitions here so that they're available somewhere until that happens.

jyasskin · 2018-07-02T20:54:42Z

index.html

+            <dt><a>received IP address</a></dt>
+            <dd>
+            the IP address of the <a>server</a> that the user agent received
+            <var>response</var> from


I see that response is defined by reference to RFC7230 instead of Fetch. That's probably the wrong choice, particularly because you can pretty easily extend Fetch's data type if you need extra data stored in it.

In this case, I think you need to add the server IP address to https://fetch.spec.whatwg.org/#concept-connection and a way to get the connection for a https://fetch.spec.whatwg.org/#concept-response. @annevk, does that sound right?

I used the lower-level references so that it's hopefully more clear how a non-browser user agent would use NEL — for instance, a native app using OkHttp to make API requests. That's also the rationale behind the particular wording I chose when I introduced phases earlier on. I see what you're saying though — jumping through hoops to make this more generic might not be worth the trouble.

Ditto — I've updated the reference so that it points to Fetch intstead of RFC 7230, but otherwise I've just added an editor's note that we want to plumb this through more explicitly in Fetch.

Thanks!

The HTTP RFCs are currently up for revision in https://github.com/httpwg/http-core#draft-http-core-documents, so it is possible to plumb things through there instead. I suspect that non-browser agents should try to sit on Fetch, though.

jyasskin · 2018-07-02T20:56:53Z

index.html

@@ -812,6 +891,33 @@ <h2>Generate a network error report</h2>
          </dl>
        </li>

+        <li>
+          If <var>request body</var>'s <code>server_ip</code> property is


s/request body/report body/?

jyasskin · 2018-07-02T21:00:29Z

index.html

+
+          <ol>
+            <li>
+              Set <var>request body</var>'s <code>phase</code> to


I might express this as completely overwriting report body instead of clearing particular fields. That way, if someone in the future adds sensitive fields but forgets to update this step, the algorithm will fail closed instead of open. (This might just be temporary paranoia on my part though.)

There's now some text down in Privacy Considerations that calls out what kind of information can be included in a report, so instead of replacing report body I added a step that user agents must clear out any fields that are derived from information that isn't available during DNS resolution.

dcreager · 2018-07-03T15:31:01Z

@igrigorik @yoavweiss, this could use another set of eyes. I've tried to summarize all of the relevant discussion from #74 in the Examples and Privacy Considerations sections.

jyasskin · 2018-07-03T17:32:04Z

index.html

+            <dt><a>received IP address</a></dt>
+            <dd>
+            the IP address of the <a>server</a> that the user agent received
+            <var>response</var> from


Thanks!

The HTTP RFCs are currently up for revision in https://github.com/httpwg/http-core#draft-http-core-documents, so it is possible to plumb things through there instead. I suspect that non-browser agents should try to sit on Fetch, though.

jyasskin · 2018-07-03T17:43:08Z

index.html

              <code>elapsed_time</code> properties.
            </li>
+            <li>
+              If the user agent has added any additional fields to <var>report


I think your inclusion of a rationale for this trimming fixes the problem I was worried about where we might update the spec and forget to list new fields here. However, I don't think this step can replace the explicit list of fields to clear: it just signals to someone reading the spec that they ought to file a spec bug if we miss something.

For that purpose, maybe an Assert is the best approach? You'd write something like

Assert: All fields in report body that are derived from information not available during DNS resolution have been cleared.

Domenic, Anne, or Ryan should feel free to override me on this.

Done, changed this to an assert. (The step just before this one already still lists status_code and elapsed_time explicitly as fields that need to be cleared.)

jyasskin · 2018-07-03T18:09:52Z

index.html

+      </p>
+
+      <p>
+      To prevent information leakage, NEL reports about a <a>request</a> MUST


Beware "MUST"s in security and privacy implications. Since they don't appear in the algorithms that have to actually make them happen, it's easy for implementers to miss them. That said, I don't have a good guideline for what to do instead.

All of these statements should be implied by the algorithms earlier in the document, so I changed the wording so that this doesn't imply that it's adding additional requirements, just elaborating on the rationale for the existing ones.

* gh-pages: Update WICG references to W3C (w3c#87) Adding baseline CODE_OF_CONDUCT.md

yoavweiss

I second @jyasskin's comments about Fetch integration. Not sure this should be a blocker though.

* gh-pages: Fix typo

yoavweiss · 2018-07-09T13:55:26Z

index.html

+        </li>
+
+        <li>
+          <dfn>Secure connection establishment</dfn>: The user agent opens a


Should we distinguish TCP from TLS here? Would also be good to properly define what happen with those phases in protocols which don't include them (TCP-Fast-Open + TLS/1.3 0-RTT, QUIC).

I tried to make this not too granular, exactly because of the different protocols and fast paths that there are. I didn't want this to be "a taxonomy of connection methods as of the time of this writing"; instead I wanted "in the broadest possible strokes, here's what happens when the user agent makes a request over the network". So I tried to merge phases together as much as I could.

These three seemed like the minimum — I couldn't glom any more of them together. dns has to be separate because it's treated differently, both for downgraded reports and for the handling of include_subdomains. application has to be separate because it's the only mandatory phase. It didn't seem necessary to split apart the connection phase — at least here in NEL — because it wouldn't have any impact on whether we're allowed to collect a report, or on what information that report could contain.

OK. I can see use cases for distinguishing between them on the collector side (e.g. distinguishing between "host is down" to "cert issues"). At the same time, not sure how important that distinction is, so fine with leaving it out for now. I also agree that in a world where everything is QUIC, that distinction may not make much sense.

There are still all of the tcp.* and tls.* error codes, so the collector will still be able to see a fine-grained description of what error occurred. It's just that all of those error codes would have the same value (connection) for their phase field.

Oh, OK. missed that

yoavweiss · 2018-07-09T14:13:09Z

index.html

+        <li>
+          If <var>report body</var>'s <code>server_ip</code> property is
+          non-empty, and not equal to <var>policy</var>'s <a>received IP
+            address</a>:


Is the reasoning behind this section is to avoid sending reports to addresses that didn't send the original policy? (so "downgrading" the reports, in a sense)
If so, can you add a note on that?

Exactly! There is some text down in the privacy § describing the rationale but I added a note here, too, so that there's an explanation close to the algorithm step.

yoavweiss · 2018-07-09T14:23:51Z

index.html

@@ -713,7 +803,7 @@ <h2>Generate a network error report</h2>
        policy</a>, and queues it for delivery.
      </p>

-      <ol>
+      <ol class="algorithm">

        <li>
          If the result of executing the <a>is-origin-trustworthy</a> algorithm


"potentially trustworthy" would've been more appropriate

This step requires the result to be "Potentially Trustworthy". Do you mean simplifying the text to just:

If request's origin is not potentially trustworthy

and not mentioning the name of the algorithm?

The name of the algorithm is "is origin potentially trustworthy?", despite the fact that its link drops the "potentially" part: https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy

I think it'd be better to change the <dfn> to indicate the full name. Might be better to do that as a separate PR.

Went ahead and did it here.

yoavweiss · 2018-07-09T14:24:14Z

index.html

+
+      <p>
+      To mitigate some of the above risks, NEL registration is restricted to
+      <a>trustworthy origins</a>, and delivery of network error reports is


"potentially trustworthy" would've been more appropriate

yoavweiss · 2018-07-09T14:28:11Z

index.html

+        </li>
+
+        <li>
+          If <var>policy</var>'s <a>subdomains</a> flag is <code>include</code>,


"included"?

include is consistent with the rest of the text. I'm happy to change this if you feel strongly, but I'd prefer to do that in a separate PR to ensure that I get all of them.

yoavweiss · 2018-07-09T14:33:02Z

index.html

+      <a>trustworthy origins</a>, and delivery of network error reports is
+      similarly restricted to <a>trustworthy origins</a>. This disallows a
+      transient HTTP MITM from trivially abusing NEL as a persistent
+      tracker.


IIUC, that means that NEL policies can effectively act as third party cookies, when delivered on third party responses. Is that correct?

If so, might be worth while to mention that and indicate that:

The policy cache should be purged when cookies are purged

Third party policies should be treated as third party cookies (and e.g. not be saved when the user preferences prevent third party cookies)

I think a NEL policy can act as a first-party cookie, but not as a third-party one. (Unless I'm misunderstanding the threat you're suggesting.) NEL policies can only be set by the origin that the requests will be sent to; this ¶ describes how we try to enforce that. i.e. a NEL policy can't be delivered on a third-party response — the policy doesn't contain an explicit origin describing what it should apply to, it implicitly applies to the origin of the response itself.

Re clearing the policy cache, there's an existing ¶ at the bottom of the section that requires that.

yoavweiss · 2018-07-10T13:28:14Z

Thanks for following up on the various comments. Good to merge from my perspective :)

dcreager · 2018-07-10T13:52:59Z

Thanks Yoav!

* gh-pages: Clean up network error reports section (w3c#89) NEL reports are not observable (w3c#77) Handle errors in different phases differently (w3c#83) update repo config Fix typo Update WICG references to W3C (w3c#87) Adding baseline CODE_OF_CONDUCT.md Factor out a Concepts section (w3c#82) Add request method to report body (w3c#80) Update examples to latest JSON schema (w3c#81)

dcreager mentioned this pull request Jun 27, 2018

Verify "ownership" when generating include-subdomain reports #74

Closed

dcreager force-pushed the phases branch from 9784197 to 6b8a2c7 Compare July 2, 2018 16:19

dcreager changed the base branch from concepts to gh-pages July 2, 2018 16:19

jyasskin reviewed Jul 2, 2018

View reviewed changes

Douglas Creager added 2 commits July 3, 2018 11:17

add examples and privacy notes

6d6197f

edits from jyasskin

ab175da

dcreager requested review from igrigorik and yoavweiss July 3, 2018 15:30

jyasskin reviewed Jul 3, 2018

View reviewed changes

Douglas Creager added 2 commits July 7, 2018 09:41

reword privacy §; use assert in report generation

9ca7a68

Merge branch 'gh-pages' into phases

e2b36ed

* gh-pages: Update WICG references to W3C (w3c#87) Adding baseline CODE_OF_CONDUCT.md

yoavweiss approved these changes Jul 9, 2018

View reviewed changes

Merge branch 'gh-pages' into phases

8927388

* gh-pages: Fix typo

yoavweiss reviewed Jul 9, 2018

View reviewed changes

Douglas Creager added 2 commits July 9, 2018 15:52

edits from yoav

1f43778

fix name of potentially trustworthy algo

b8f8f47

fix some typos

87386f7

dcreager merged commit e707a78 into w3c:gh-pages Jul 10, 2018

dcreager deleted the phases branch July 10, 2018 13:55

dcreager mentioned this pull request Jul 10, 2018

DNS errors? #48

Closed

This was referenced Jul 19, 2018

Specify a maximum 'max-age' #65

Closed

Clarify when reports are sent #66

Closed

Handle errors in different phases differently #83

Handle errors in different phases differently #83

Conversation

dcreager commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcreager commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoavweiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoavweiss commented Jul 10, 2018

dcreager commented Jul 10, 2018