On Wed, Jan 4, 2017 at 2:29 PM, Alex Harui <aha...@adobe.com> wrote:
> There is a lot of incorrect assumptions in your email. I'm not going to
> take the time to address all of them. It seems you are very passionate
> about security, and it is great that you identified that directly setting
> HTML into the DOM presents a security issue, so thank you for bringing
> this up.
>
I apologize if I made incorrect assumptions. I know you are very busy and
try not to waste your time by having to write unnecessary responses.
Please feel free to point out the errors at a later point.
>
> However, I disagree with the notion that everything we do must be
> bullet-proof.
I never presented that notion. It seems to me that you are using the
slippery-slope argument. I am suggesting a very basic protection from a
very common type of security risk. I gave you several examples where other
frameworks provide same or similar protection in their UI components.
> Not everybody wants to or needs to ride around in a
> military-grade vehicle. Go make the changes you want in the Express
> component set, but please let others like myself create a low-level direct
> access to the DOM for those who have appropriately sanitized their code
> some other way. I will name it UnsecuredHTMLText.
>
What in my proposal will prevent you from making your own component with
low-level direct access to the DOM?
>
> My main priority is happy customers. Security, Functionality, and
> Performance are trade-offs to reaching that goal.
I am willing to do a tradeoff. Hence the compromise proposal. Building
apps that don't cause obvious security issues results in happy customers as
well.
> Realize that when you
> build the team page, if we sanitize the team data some other way, all of
> the security code you write will run and never catch anything. To me,
> that's not a great example of PAYG.
>
We are not writing the component set for one use case. I have previously
provided examples where the user has no control over where the HTML comes
from. I thinking running a sanitizer on a HTML string is a very good
tradeoff. I guess this is where we disagree.
Even if that is not acceptable, if the user uses the
InsecureNoHTMLSanitizer, it is a simple function call which returns the
same string back, essentially a no-op. There is no time spent on analyzing
the HTML string.
Is this also a not acceptable as a tradeoff? What is your definition of a
trade-off?
>
> I also don't want to put the burden on this group of volunteers right now
> to try to meet an expectation that folks can write apps that are bullet
> proof.
This will not make our framework bullet-proof. I am sorry if I said
something that seemed to suggest that.
> If that's your itch, make a component set that tries to do that,
> but please leave the rest of it alone.
>
Isn't my proposal doing that? i.e. leave the rest of it alone? What am I
proposing that will prevent anyone else to build some other component that
does what they want?
>
> Now if you or others are interested in leveraging our assets to try to
> make a better API for catching/preventing more security issues, we can
> have that discussion. I think there is potential to do more once we stop
> thinking of HTML as a String.
>
> My 2 cents. Other opinions welcome.
> -Alex
>
Thanks,
Om
>
> On 1/4/17, 11:21 AM, "omup...@gmail.com on behalf of OmPrakash Muppirala"
> <omup...@gmail.com on behalf of bigosma...@gmail.com> wrote:
>
> >Before we lose context, I want add more of my thoughts here.
> >
> >I think the priority of things should be:
> >
> >1. Functionality
> >2. Security
> >3. Performance
> >
> >Alex probably thinks it should be:
> >
> >1. Functionality
> >2. Performance
> >3. Security
> >
> >As a compromise, in this context I am going to suggest that we go with:
> >
> >1. Security,
> >2. Performance
> >3. Functionality
> >
> >That is, security is still higher than functionality (which is what I
> >want). And the priority of performance has not changed (I think what Alex
> >wants)
> >
> >To explain it in a bit more detail: My goal is to not allow a developer
> >to
> >inadvertently add a bad piece of HTML to the DOM. That is how security
> >exploits occur. We cannot simply say that it is upon the developer to
> >ensure that a HTML snippet is sanitized before they use HTMLText to render
> >it on the screen. We've seen this happen to Flash Player in the past
> >where
> >ease of use was prioritized over security. This behavior changed
> >eventually, where security was considered when building each feature. I
> >think we should learn from that to avoid being labeled as 'insecure
> >software' .
> >
> >The options that Alex suggested (doing it in the HTTPService layer, etc.)
> >does not cut it. Because:
> >
> >a. It is not appropriate for HTTPService to divine the nature of a piece
> >of data. HTML can be used to display as DOM or shown to user to edit. We
> >could have two UI widgets one, that shows the HTML in an editable text
> >area, and one that previews the HTML. For example, editing an already
> >comment in JIRA or a blog post and previewing it before saving. If we
> >blindly let the HTTPService sanitize the data, we are losing data.
> >b. A scary piece of HTML is not a security threat when it lives as a
> >string. There is no need to do anything about it. It becomes a security
> >threat only when it is added to the DOM. So, sanitizing it just before it
> >is added to the DOM is the most appropriate thing to do. I guess this is
> >perfectly adhering to the 'Pay as you go' philosophy. Don't sanitize the
> >HTML just in case, sanitize it just in time.
> >
> >So, as a compromise, here is what I propose:
> >
> >By default, HTMLText component expects an IHTMLSanitizer bead. If such a
> >bead is not available, the component will throw a runtime exception and it
> >will not render the HTML.
> >
> >IHTMLSanitizer probably looks like this:
> >
> >function sanitizeHTML(v:String) :String;
> >
> >We will have two beads built in:
> >1. HTMLSanitizer implements IHTMLSanitizer and uses a default algorithm
> >to
> >sanitize the given HTML.
> >2. InsecureNoHTMLSanitizer implements IHTMLSanitizer which simply returns
> >the same string back.
> >
> >We can build two 'express' components like this:
> >
> >express:HTMLText looks like this:
> >
> ><basic:HTMLText>
> > <basic:beads>
> > <basic:HTMLSanitizer />
> > </basic:beads>
> ><basic:HTMLText>
> >
> >express:InsecureHTMLText looks like this:
> >
> ><basic:HTMLText>
> > <basic:beads>
> > <basic:InsecureNoHTMLSanitizer />
> > </basic:beads>
> ><basic:HTMLText>
> >
> >This achieves the following goals:
> >
> >1. If someone wants to use basic:HTMLText and they don't provide a
> >IHTMLSanitizer, they wont be able to display any HTML and will encounter
> >runtime errors.
> >2. They can use express:HTMLText which will run all HTML through a
> >sanitizer. They will either be happy or will complain about performance.
> >3. They can switch to express:InsecureHTMLText and get better
> >performance. But since the name of the component clearly says that using
> >it is insecure, we have at least given them an opportunity to understand
> >what is going on.
> >4. Some folks might want their own HTML sanitization logic. They can
> >create their own SpecialHTMLSanitizer which implements IHTMLSanitize and
> >add it to basic:HTMLText
> >
> >Thoughts?
> >
> >Thanks,
> >Om
> >
> >P.S. Does the name 'TrustedHTMLText' work better than 'InsecureHTMLText'?
> >
> >On Tue, Jan 3, 2017 at 8:20 AM, flex capacitor <flexcapaci...@gmail.com>
> >wrote:
> >
> >> I have a few thoughts on that. I need a component like this in the past
> >>for
> >> the times I had separate HTML content already created but needed to be
> >> integrated into a Flex JS app. I would suggest a contentBefore,
> >> contentAfter and contentOverride properties maybe as a bead or a full
> >> separate component.
> >>
> >> Those would be child tag properties that would pass the raw content
> >>through
> >> enclosed in CDATA tags or not and the container or bead would have a
> >> sanitize properties that could be set to true or false.
> >>
> >> The purpose would be to allow HTML embed content. Say you want to drop a
> >> Google maps widget or rich text editor into your Flex JS app. You don't
> >> want the compiler to parse that code. You would just want app to write
> >>the
> >> raw HTML to the document in the container it is declared in. You would
> >>be
> >> responsible for the security as the author and how to talk to it.
> >>
> >> The before and after properties would allow things like HTML comments or
> >> other uses. In the past you had php code that wraps HTML markup but now
> >>it
> >> could be used for some of the other frameworks out there to use markup
> >>to
> >> perform other tasks.
> >>
> >> How that will work with the Flex JS model new App(); I'm not sure.
> >>
> >> On Jan 1, 2017 3:16 AM, "OmPrakash Muppirala" <bigosma...@gmail.com>
> >> wrote:
> >>
> >> > On Sun, Jan 1, 2017 at 12:15 AM, Alex Harui <aha...@adobe.com> wrote:
> >> >
> >> > >
> >> > >
> >> > > On 12/31/16, 11:23 PM, "omup...@gmail.com on behalf of OmPrakash
> >> > > Muppirala" <omup...@gmail.com on behalf of bigosma...@gmail.com>
> >> wrote:
> >> > > >
> >> > > >Okay, I see your point now. To be clear, I did not set out to
> >>write
> >> > HTML
> >> > > >code. The bio in team.json is a html snippet. So, I am just
> >>trying
> >> to
> >> > > >display it as a piece of formatted data. Even in other projects,
> >>I do
> >> > > >need
> >> > > >this kind of thing quite often.
> >> > >
> >> > > IMO, this is use case #1: non-interactive HTML content. We could
> >>just
> >> > > have an HTML component with an "html" property. But it would want
> >>to
> >> > wrap
> >> > > the content in a <div> or <span>
> >> > >
> >> >
> >> > This is all I wanted. I implemented this and it seems to work fine.
> >> >
> >> >
> >> > >
> >> > > Case #2: In MXML, someone wants to mix FlexJS elements with random
> >> HTML.
> >> > >
> >> > > Case #3: In MXML, someone wants to put in random HTML where one
> >>child
> >> > HTML
> >> > > tag has an id and wants to write code that addresses that id.
> >> > >
> >> > > The cheap option is to only support #1 by creating an HTML component
> >> with
> >> > > an "html" property. Everything else needs more thinking, IMO.
> >> > >
> >> >
> >> > 2 and 3 sound like terrible ways to code :-)
> >> >
> >> >
> >> > >
> >> > > >
> >> > > >That brings up a related concern. We need to sanitize such html
> >> content
> >> > > >during runtime [1]
> >> > >
> >> > > IMO, those are just utility functions. Not everybody needs
> >> sanitization,
> >> > > IMO, so PAYG.
> >> > >
> >> >
> >> > Hmm, to play the devil's advocate, security should not be
> >>pay-as-you-go.
> >> > This should be opt-in by default. Someone will have to go the extra
> >>mile
> >> > to turn it off.
> >> >
> >> > This is the sort of thing that will go out in the wild and folks will
> >>get
> >> > affected by it soon enough. We will then need to push out an
> >>emergency
> >> > release to fix an XSS attack made possible by FlexJS.
> >> >
> >> > Either that or we call the default implementation 'InsecureHTMLText'
> >>or
> >> > something like that.
> >> >
> >> >
> >> > >
> >> > > >>
> >> > > >>Not really sure why this is required. I am looking for usage
> >> patterns
> >> > > >>like
> >> > > >>this:
> >> > > >>
> >> > > >><Panel>
> >> > > >> <HTMLText>{myFancyHTMLText}</HTMLText>
> >> > > >> <Image></Image>
> >> > > >></Panel>
> >> > > >>
> >> > > >><TabNavigator>
> >> > > >> <Tab>
> >> > > >> <Button />
> >> > > >> <HTMLText />
> >> > > >> </Tab>
> >> > > >> <Tab><HTMLText></Tab>
> >> > > >></TabNavigator>
> >> > > >>
> >> > > >>I guess the HTMLText component has to be a UIBase for this to
> >>work,
> >> > > >>right?
> >> > > >>
> >> > > >>Maybe I am not understanding the point you are trying to make.
> >> > > >
> >> > > >The above would likely create an HTML DOM like:
> >> > > >
> >> > > ><Div className="Panel">
> >> > > > <Span>the HTML from myFancyHTMLText</Span>
> >> > > > <Img />
> >> > > ></Div>
> >> > > >
> >> > > >
> >> > > >Carlos asked earlier about getting rid of the Span since we all
> >>know
> >> it
> >> > is
> >> > > >legit to have the HTML DOM look like:
> >> > > >
> >> > > ><Div className="Panel">
> >> > > > The HTML from MyFancyHTMLTExt
> >> > > > <img />
> >> > > ></div>
> >> > > >
> >> > > >
> >> > > >I doubt if this HTML can be generated. Are you sure there will be
> >>a
> >> > > >linebreak after the text?
> >> > >
> >> > > No. I just added that to show the "children".
> >> > >
> >> > > >
> >> > > >That looks a TextNode element. Which is lightweight than innerHTML
> >> But
> >> > > >are we sure we don't want formatting for such text? If we want to
> >>be
> >> > able
> >> > > >to format, we need to use a p or a span element.
> >> > >
> >> > > AIUI, a TextNode is just plain text. If the above was:
> >> > >
> >> > > <div className="Panel">
> >> > > The <b>HTML</b> from <strong>MyFancyHTMLTExt</strong>.
> >> > > <img />
> >> > > </div>
> >> > >
> >> > > There would be a TextNode for "The", a Bold node for "HTML", another
> >> > > TextNode for "from" etc.
> >> > >
> >> >
> >> > What I meant was that HTML could be achieved by using the
> >> > document.createTextNode() function which creates a TextNode element.
> >> i.e.
> >> > no need for a wrapping element like P or Span. Sorry for the
> >>confusion.
> >> >
> >> >
> >> > >
> >> > >
> >> > > This doesn't feel right to me. All of us seem tempted to want to
> >> define
> >> > a
> >> > > scrap of complex HTML separate from other child controls in a
> >> container.
> >> > > If there was an HTML control, it would still add a wrapping element.
> >> If
> >> > > you pull of a trick where the HTML control just injects the content,
> >> you
> >> > > run into more complexity dealing with numElements and childIndex.
> >>It
> >> > > would be what the browser says it would be, but not quite as obvious
> >> from
> >> > > the MXML.
> >> > >
> >> > > On the other hand we have those thin-wrapping components like Div,
> >>and
> >> A
> >> > > and you ought to be a able to put in HTML content without it being
> >> > wrapped
> >> > > by another layer.
> >> > >
> >> >
> >> > Yes, I am in agreement with you here. A div or span with innerHTML is
> >> more
> >> > than adequate for this usecase.
> >> >
> >> >
> >> > >
> >> > > >There is a cost in both development time and runtime performance to
> >> > > >generalizing this. Should we do it?
> >> > > >
> >> > > >
> >> > > >Hmm, I don't think this requires a change to MXML parsing.
> >> > >
> >> > > I think it does. The default property is expected to be an array of
> >> > > Ichild elements. When you have random HTML in the default property
> >>the
> >> > > compiler won't accept XML TextNodes in many places and where it
> >>does,
> >> it
> >> > > tries to concatenate TextNodes which is what we don't want.
> >> > >
> >> >
> >> > I see. I did not realize this point. In any case, I would advice not
> >> > adding a random TextNode element like this. This also brings more
> >> security
> >> > issues [1]
> >> >
> >> > Thanks,
> >> > Om
> >> >
> >> > [1] http://benv.ca/2012/10/02/you-are-probably-misusing-DOM-
> >> text-methods/
> >> >
> >> >
> >> >
> >> > >
> >> > > Other opinions welcome...
> >> > > -Alex
> >> > >
> >> > >
> >> >
> >>
>
>
