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

[Jari Arkko <[e-mail]>]

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