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

[Robert Sparks <[e-mail]>]

Copying websec at Barry's request:

On 8/13/13 2:52 PM, Jari Arkko wrote:
> 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.)
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.
>
> Jari
>
> On Aug 12, 2013, at 4:46 AM, Tobias Gondrom <[e-mail]> wrote:
>
>> 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:
>>> 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.
>> 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.
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
>> clickjacking  description references are not IETF, nor other SDOs and
>> potentially not stable over the full lifecycle of the RFC.
>>
>>
>>
>>> 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".
>> Ok. changed to "definition".
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".


>>
>>
>>> 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.)
Thanks for those edits, and the ones you call out below.

>>
>>
>>> 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.
>>
>>> 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.
>> 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.
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).
>>
>>
>>> 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.
>> 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.
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
>> "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.
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.
>>
>>
>>> 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".
>> Ok. Changed accordingly.
>>
>>> Nit: s/does support only one/only supports one/
>> Ok. Changed.
>>> 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.
>> 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.
>> _______________________________________________
>> Gen-art mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/gen-art