Subject: [Gen-art] Genart LC review: draft-ietf-websec-x-frame-options-07

From: Robert Sparks <[email-masked]>
Date: Thu, 08 Aug 2013 15:00:02 -0500

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-websec-x-frame-options-07
Reviewer: Robert Sparks
Review Date: 2013-08-08
IETF LC End Date: 2013-08-12
IESG Telechat date: 2013-08-15

Summary: This draft is basically ready for publication, but has nits 
that should be fixed before publication.

This document is intended to act as a informational reference describing 
what's already deployed, and not to act as a standard for new 
implementations to follow. It's obvious that some care has been taken to 
keep that tone in the text, but there are places where it still looks 
like a proposed standard. Please make it even more clear who the 
audience for this document is, and look for places to reinforce that 
this is documenting what _is_, as opposed to trying to influence what 
should be.

The introduction of this version of the document does a good job of 
describing what a clickjack attack is. Appendix B is not really adding 
to that description. I suggest that the two usecases it call out be 
reduced to a couple of sentences in the introduction as examples, and a 
reference to a more in-depth treatment of scenarios like them be added 
for those looking for more information. Section B.3 is different, and 
doesn't add to the understanding of this document. Again, I suggest 
pointing to somewhere else that provides an in-depth treatment. 
(Hopefully, that source would discuss the nested frame issue the 
security considerations section points out in the context of the this 
specific configuration page). [FRAME-BUSTING] might be enough, but 
pointers to something even more rich would certainly be helpful.

Here are some suggestions in document order:

In the Abstract, I suggest replace "specification" with either 
"definition" as the introduction uses, or with "the syntax and behavior 
observed in current deployments".

The last paragraph of the introduction reads like standards text. Please 
consider rewriting it with a tone of history and current fact. Perhaps 
starting with "In existing implementations, ", and replacing the last 
sentence with something like "The policy this HTTP header declares is 
enforced by the browser implementations documented here".

Since this is documenting an existing deployed header (not standardizing 
it), it would be useful to comment whether the existing known 
implementations allow all the productions of the ABNF described here to 
be used. For instance, would they all honor X-FRAME-OPTIONS: deny? 
(While thinking about this, focus on who this document is for.)

The last sentence of 2.3.1 as written is an attempt to be a standard, 
and is an odd use of 2119. I suggest rewriting it as a warning about 
what happens with existing plugins that ignore the policy carrid in the 
header. That keeps the message aligned with documenting what exists.

The first sentence of 2.3.2 is not adding anything to the document. I 
suggest deleting it. The second sentence is placing an implementation 
requirement again. Please consider rewriting as an observation of what 
existing things do, calling out the ramifications of the ones that 
behave differently.

Similarly, the first paragraphs of Section 2.3.2.1 is written to affect 
new implementation, not document existing implementation. They could be 
re-tuned as above. The third paragraph is good history

The last sentence of 2.3.2.2 should not say "replace this document" - 
this document is informational documentation about existing deployments. 
CSP1.1 won't Obsolete or Replace this document. Rather it should say 
something like "it is expected that newer implementations will use it 
rather than the mechanisms documented here".

Nit: s/does support only one/only supports one/

Do the known implementations follow the pattern described in 2.3.2.3? 
Saying something would help tie this to being information about the 
known deployment rather than trying to constrain new development.

Nit: If Appendix B is kept, please expand CSRF (and consider re-pointing 
to a reference)

---

From: Tobias Gondrom <[email-masked]>
Date: Sun, 11 Aug 2013 21:46:53 +0100

Dear Robert,

thank you very much for the review.
I revised the document to version-08 (also including feedback from other
reviews) accordingly including most of your feedback.
Answers inline.
Best regards, Tobias

On 08/08/13 21:00, Robert Sparks wrote:
> This document is intended to act as a informational reference
> describing what's already deployed, and not to act as a standard for
> new implementations to follow. It's obvious that some care has been
> taken to keep that tone in the text, but there are places where it
> still looks like a proposed standard. Please make it even more clear
> who the audience for this document is, and look for places to
> reinforce that this is documenting what _is_, as opposed to trying to
> influence what should be.
>
> reduced to a couple of sentences in the introduction as examples, and
> a reference to a more in-depth treatment of scenarios like them be
> added for those looking for more information. Section B.3 is
> different, and doesn't add to the understanding of this document.
> Again, I suggest pointing to somewhere else that provides an in-depth
> treatment. (Hopefully, that source would discuss the nested frame
> issue the security considerations section points out in the context of
> the this specific configuration page). [FRAME-BUSTING] might be
> enough, but pointers to something even more rich would certainly be
> helpful.

Apologies, but I would rather keep the three example cases in the
appendix of the document as this will allow better understanding of the
document if references are no longer available. Consider that
clickjacking  description references are not IETF, nor other SDOs and
potentially not stable over the full lifecycle of the RFC.

> "definition" as the introduction uses, or with "the syntax and
> behavior observed in current deployments".
Ok. changed to "definition".

>
> The last paragraph of the introduction reads like standards text.
> Please consider rewriting it with a tone of history and current fact.
> Perhaps starting with "In existing implementations, ", and replacing
> the last sentence with something like "The policy this HTTP header
> declares is enforced by the browser implementations documented here".

Changed accordingly.
Please note: IMHO I did not feel that the last paragraph of the
introduction reads like standards text. However, I appreciate your
feedback and did modify the last sentence of the last paragraph of the
introduction accordingly to remove the word "conforming".
(I believe the remaining text describes sufficiently that this is
documentation. So I did not want to rewrite the whole paragraph and add
history.)

> Since this is documenting an existing deployed header (not
> standardizing it), it would be useful to comment whether the existing
> known implementations allow all the productions of the ABNF described
> here to be used. For instance, would they all honor X-FRAME-OPTIONS:
> deny? (While thinking about this, focus on who this document is for.)

Please note, that section 2.3.2.2. describes the variation in behaviour
between different browsers.
As existing browser implementations may change with regards to whether
they allow _all_ the productions of the ABNF, we intentionally did not
investigate and document all possible outcomes and combinations.

> what happens with existing plugins that ignore the policy carrid in
> the header. That keeps the message aligned with documenting what exists.

Ok. I changed it to non-2119.
However, the remaining text is appropriate and adds clarity to declare
the scope of the x-frame-options header effects. And implementations and
specifications of x-frame-options out there consider it as part that
plug-ins observe x-frame-options if "these plugins appear essentially as
frames".
Please consider that a plug-in that would "essentially appear as a
frame" but would not be observing x-frame-options would in effect break
the security model of the browser implementation, so I think the
remaining text is appropriate and needed in this case.

> existing things do, calling out the ramifications of the ones that

Please consider that even though this is to document existing use
(especially until we get CSP1.1), it's documentation can also help guide
new implementations for the purpose to reduce or avoid divergence of
implementations. This leads to the first sentence and also to the use of
"MAY" in the second sentence, which in fact is not a requirement of the
browser implementation but points out an implementation option.

>
> Similarly, the first paragraphs of Section 2.3.2.1 is written to
> affect new implementation, not document existing implementation. They
> could be re-tuned as above. The third paragraph is good history

As the comment before, this documentation also helps to reduce or avoid
divergence of implementations. So the "SHOULD" is appropriate and as
intended here.

> this document is informational documentation about existing
> deployments. CSP1.1 won't Obsolete or Replace this document. Rather it
> should say something like "it is expected that newer implementations
> will use it rather than the mechanisms documented here".

Ok. Changed accordingly.

> Nit: s/does support only one/only supports one/
Ok. Changed.
> known deployment rather than trying to constrain new development.

Hm. First, as outlined in 2.3.2.2 not all of the browsers support
"Allow-From". And second, the pattern is actually not for the browser
but a way for the server to achieve the specific goal (i.e. of a source
page being framed by more than one web page, even though allow-from
supports only on URI).
To accomodate your feedback and make clear this is not trying to
contrain new development, I changed the word "is recommended" to "can
fulfill that need".

>
> Nit: If Appendix B is kept, please expand CSRF (and consider
> re-pointing to a reference)
>
Ok. Done.

---

From: Jari Arkko <[email-masked]>
Date: Wed, 14 Aug 2013 03:52:20 +0800

Robert: Thank you very much for your review. This has been very helpful. Tobias: Thank you for the changes.

(I can see that there could be some discussion about some of the remaining text changes. Do you Robert think the remaining points are important enough to warrant me raising them in the IESG discussion? For now I have filed a No-Objection position.)

Jari

On Aug 12, 2013, at 4:46 AM, Tobias Gondrom <[email-masked]> wrote:

> _______________________________________________
> Gen-art mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/gen-art

---

From: Robert Sparks <[email-masked]>
Date: Tue, 13 Aug 2013 16:34:28 -0500

Copying websec at Barry's request:

> (I can see that there could be some discussion about some of the remaining text changes. Do you Robert think the remaining points are important enough to warrant me raising them in the IESG discuss…
Thanks for the nudge Jari. I've continued the conversation below.
I would still like to see some adjustment of the character of the text, 
but if I were a balloting AD, I would be putting in comments, not discusses.

Tobias - Thanks for your changes so far. I appreciate that some of what 
I'm pushing for may seem subtle.

I've had to handle several documents that might have been 
standardization fodder at some point in time but for various reasons 
turned out not to be.
The resulting documentation of some of those came out looking to me like 
a standards document with an Informational or Independent Submission label
taped to the top. You've done a good job of avoiding that with this 
document, and the changes I'm pushing for are to make that aspect even 
better (I hope). Thank you (and the WG) for considering them.
>> document if references are no longer available.
I don't think they do, actually. If those references are gone, there is not
enough in this document, particularly in those appendices, to inform
someone who didn't already understand clickjacking. To state that more
strongly, I don't think you're helping the type of future reading you are
trying to help, and as a consequence, you may even add to their confusion.
I don't think adding more to this document is the right thing to do.
I really think you should point to references instead. If you don't 
trust the
ones you have to be stable for the period of time this document is needed
(which surprises me), then perhaps you should look for something more 
stable?

>>   Consider that
I wasn't specific with which instance of specification I meant (I missed 
that there were two).
With the new text (-09), in this paragraph, I would say "this document 
describes" instead of
"this definition describes", and "definition of this X-Frame-Options" 
rather than "specification
of this X-Frame-Options".

>> documentation. So I did not want to rewrite the whole paragraph and add
Thanks for those edits, and the ones you call out below.

>> remaining text is appropriate and needed in this case.
This takes you into talking about what new implementations should do, 
rather than documenting what's in the wild.
You can avoid that by noting that any existing plug-in implementations 
that don't do the right thing here create a problem.
(It would follow that any new ones would have that same problem).
>> implementations.
You can have that influence by reporting on what is rather than trying 
to write constraints on new things.
>> This leads to the first sentence and also to the use of

>> divergence of implementations. So the "SHOULD" is appropriate and as
How about simply saying that any change or divergence to the 
implementations will result in (this list of bad things).
That's informative rather than trying to be normative.