Proposal for 1.0: Change auto-escaping of EL strings to a MAY

[goosemanjack]

Proposal:

In section 6. Expressions of the Template spec, we'd like to modify
the last paragraph to state that HTML escaping of EL statement results
is an optional decision of the implementer.

http://wiki.opensocial.org/index.php?title=Auto-escaping_of_strings_in_EL_a_MAY

Auto-escaping of EL statements, after conferring with our security
folks, does not appear to be a full enough solution to the myriad
security holes that are possible with apps. We feel it is too soon in
the spec lifecycle to lock in to this as the security solution. We
require the spec to be loose enough for other security solutions to be
used, as well as the liberty to explore features the require the EL to
emit the same content as was originally loaded in the DataContext. As
such we cannot support the EL auto-escaping rules as currently written
in the 0.9 spec.

Modifying the language to make HTML-encoding of EL strings a MAY
instead of a must is a compromise solution that both allows the
Shindig project to continue with the security design they have in
place and allows MySpace to continue pursing features we already have
in place that rely on a more stable input/output model for EL
strings. It also allows both teams to continue to pursue alternate
and additive security models as extensions.
--
clc

[Evan Gilbert]

MAY is not possible for this feature - it means your gadget will be secure on some containers and have a gaping security hole in other containers. Also, there would need to be a way to force escape content for containers that used MAY.

It may not be a complete solution but it's a great start. The default should be to only create text content (and to do extra escaping in certain types of attributes).

One of the biggest security holes in the first rev of OpenSocial was the use of innerHTML in raw JavaScript. Most examples and many of the initial apps had security holes caused by div.innerHTML = '<b>' + user.getDisplayName  + '</b>' (or similar). These security holes allow a malicious user to use a popular gadget to spread an attack.

So I don't think we can have this a MAY

 

--
clc

[Adam Winer]

I entirely agree with Evan. Removing this makes cross-container
compatibility impossible, and makes the default mode insecure. I'm
eager to hear suggestions for improvements, but "not full enough" is a
poor justification for removing it altogether. If your security team
could describe the holes in greater detail, that'd certainly be
appreciated, and would be a far more helpful basis for further
discussion.

MySpace certainly could experiment with optional attributes that
change escaping rules, as long as the default behavior is to be spec
compliant.

-- Adam


>
>>
>> --
>> clc
>>
>>
>
>
>
>
> >
>

[goosemanjack]

We have numerous apps completely dependent on the innerHTML
functionality, so it is not possible to simply turn off the spigot on
allowing this. Given that this security solution is so porous, it's
almost the same as not having security at all. It might catch
inadvertent markup errors that would break layout, but a truly
malicious app will circumvent these restrictions by using proxied
content or any of the existing mechanisms for displaying content that
we still will support going forward.

I would agree that a full battery of escaping/unescaping methods needs
to be supplied for various contexts. It's a set of extensions we'll
be looking at supplying with 0.9. We view the spec as-written WRT the
statement "however based on context they may be..." to make this
section unworkable. There are simply too many edge cases that make
properly detecting content and intent programmatically an impractical
solution. Tracing an attribute back to discover it being an image is
workable enough. Detecting CSS context properly starts to get icky.
Javascript, however, becomes impractical because the parser cannot
know if the intent is for display as escaped code, insertion as a DOM
element, etc.

The alternative proposal for us is to consider this section of the
spec failing to meet the requirement of a workable prototype. Unless
Shindig can produce a functional prototype of of this section that can
fully handle contextual escaping, our position is likely to remain
that this is an error in the spec. I can bring your alternative back
to the team, but I'm not sure how much traction it will get, given how
our installed app base is using the platform.
--
clc

On Oct 15, 8:48 am, Evan Gilbert <uid...@google.com> wrote:
> On Wed, Oct 14, 2009 at 4:36 PM, goosemanjack <cc...@myspace.com> wrote:
>
> > Proposal:
>
> > In section 6. Expressions of the Template spec, we'd like to modify
> > the last paragraph to state that HTML escaping of EL statement results
> > is an optional decision of the implementer.
>

> >http://wiki.opensocial.org/index.php?title=Auto-escaping_of_strings_i...

[Lev Epshteyn]

But Chris - auto escaping was never meant to protect containers from malicious apps developers. Its main goal is to protect apps from malicious users by auto-sanitizing user input in a context-sensitive manner.

Auto escaping is pretty easily achieved in DOM based implementation (when you create a TextNode, and set its content, it will never be treated as HTML. Similarly, if you set an attribute value via DOM APIs, even a value that contains quotes has no chance of spilling over to another attribute or element). Shindig does pretty much this on the client and the server. It is a bit messier, but still possible, to create a routine that would iterate over a string representation of a rendered template keeping state when it's inside a text node / attribute and perform proper escaping. Given this, I don't see your resistance to this clearly useful spec feature.

I believe most of the things included into ${} brackets will be user data in one way or another, so I'm not sure why you are arguing against sanitizing it by default. This changes our platform from trivially exploitable to non-trivially exploitable. It may not be the ultimate solution, but will certainly deter people who are only so motivated to wreak havoc. 

True, this will not fix apps using innerHTML today, but will give the newly written template-based apps a new layer of protection against trivial attack. It was my understanding that this was one of the original value propositions of templates.

[goosemanjack]

I spoke with the team, and at this point we feel the current spec is
unworkable. It lacks the detail or the completeness to make it
practical. Even a trivial implementation requires numerous extensions
to make it quasi-functional. Given that how the EL behaves at a
global level is such a pervasive and wide-reaching aspect of OSML in
general, we must insist on clarity. I reviewed internal conversations
on this aspect of the spec to see what insight I could gain before
replying. We've had long-running misgivings about this section in the
spec, but decided to not oppose it in the 0.9 round in the interest of
ratifying a version of the spec. We expected to have some data from
live implementations to use concrete data from when revisiting this
item. Instead the transition from 0.9 to v.next (we're not willing to
sign off on this being 1.0 yet) comes with neither group being fully
0.9 launched with any depth of data, so we're still shooting from the
hip.

The feeling over here is that if Shindig wants to pursue auto-escaping
within the EL, they are free to do so and re-propose it as an
extension. For now, auto-escaping is unworkable as proposed in the
spec and will be considered a spec error. There are no complete
implementations that implement it to a satisfactory workable model and
inadequate data on usage patterns to warrant it's inclusion.

I, for one, would like to see 1.1 (or v.next.next) all about security,
but we need a more complete model before we're willing to commit to
one particular solution.
--
clc

> Its main goal is to protect apps from malicious
> users by auto-sanitizing user input in a context-sensitive manner.
> Its main goal is to protect apps from malicious
> users by auto-sanitizing user input in a context-sensitive manner.

> I believe most of the things included into ${} brackets will be user data in
> one way or another

[Chris Chabot]

On Tue, Oct 20, 2009 at 5:39 AM, goosemanjack <cc...@myspace.com> wrote:

The feeling over here is that if Shindig wants to pursue auto-escaping
within the EL, they are free to do so and re-propose it as an
extension. 

Extensions are intended for incubating new ideas, and really not for hiding away incompatibilities between containers.

It is a valid point that having different behavior between containers (one escaping and another not) wouldn't make OpenSocial more developer friendly, either they would expect escaping and have security holes on containers that don't, or could end up doing double escaping, which also tends to break things.

While we all represent containers that have certain opinions and priorities, I think it's also important to always keep the bigger picture in mind and consider if it's good for the OpenSocial landscape as a whole, because in the end the better OpenSocial does the better all of our app platforms do. And a proposal to create more incompatibilities and inconsistencies really doesn't pass that litmus test.

   -- Chris

[Lev Epshteyn]

I agree that clarity is important. If you believe that the current wording on auto-escaping is unclear, we should work on it to fix the language. However, I don't see how stripping off an important feature that makes apps minimally secure helps us attain clarity.

As Adam has mentioned, there have been instances of the specific type of attack that this escaping is aiming to stop. I think this is why there is so much resistance to dropping auto-escaping.

What is the concern over it? Is there an implementation issue on your side? Or are you opposed to auto-escaping in principle? Or do you not think HTML injection will be a problem for template-driven apps?

[Jon]

Having been a "-1" voter, The primary reason is: having something this
significant as a MAY would be unworkable for development of portable
gadgets. However, could we do something that puts this in the hands of
the gadget developer?

Templating requires a feature, "opensocial-templates". Perhaps we
could put a parameter there that could control the escaping, even use
an enum rather than a bool for future purposes. Then even if the
default value is a MAY, at least a gadget developer could specify the
behavior and have a portable gadget.

Couple this with "Versioned Core Gadget Features" and you have a way
to evolve, addressing the security concerns, while achieving some
certainty for gadget developers.

Jon

[goosemanjack]

If the spec list is willing to put the brakes on the entire 1.0
version for a few months, we can be brought back to the table to work
this out. Behavior of the EL is too core an element of the overall
templating behavior for us to allow a behavior we generally disagree
with to slide through the spec discussion for two revs un-flagged.
Then the argument becomes "It's been in the spec for two versions, so
we can't possibly change it."

We serve neither our own interests nor the interests of our developer
community by implicitly lending support to a spec that has language we
see as problematic on such a core feature. As we've stated on
multiple occasions in the spec list, we want to see real usage data
from the 0.9 spec implementation before moving forward. Our original
proposed timeline was to start 1.0 spec work in November once we had
our implementation live to the public and weren't in the middle of
heavy coding, not complete the v.Next spec work and have it voted on
before having any usage data whatsoever.

Given the current spec ratification timeline and the disinterest in a
compromise solution, we don't see this as an issue that can be
resolved this round of the spec.

> ...
>
> read more »

[Arne Roomann-Kurrik]

I think you're misrepresenting the spec timeline.  We will obviously be working on 1.0 through November at this pace - if the EL needs to be cleaned up for 1.0, then let's make that a focus for this iteration and go forward as needed.  I'd +1 a proposal to have someone rewrite the templating spec for this round if they want to put the effort into it, like we're doing with REST/RPC.  Derailing the entire spec process and threatening a -1 on 1.0 isn't the correct way to go forward here.  

~Arne

[Jon]

> We serve neither our own interests nor the interests of our developer
> community by implicitly lending support to a spec that has language we
> see as problematic on such a core feature.  

One purpose of this spec will allow Open Social sites to be developed
independently of gadgets, and have gadgets be portable across sites.
Most optional features are all or nothing, but this use of MAY in this
context leads to different, incompatible, implementations. How could
we change the proposal to allow a gadget developer to write once for
MySpace as well as all other compliant sites?

[Jorge Reyes]

I'm all for rewriting El and templating specs. I'm had been trying to implement the spec. The mayor challenge and source of issue in it is that the behavior is contextual. 

example

<javascript ... tag="myapp:tag">

....

</javasript>

to call it I need to use

<myapp:tag ...>

I know that I'm trying to use a tag because was define before and I search in my collection of tags.  

I think a syntax like

<tag key="myapp:tag"> 

will be more efficient to process and reflects intent. 

another example is 

<javascript tag="myapp:tag">

<os:Render content="xxx">

</javascript>

<myapp:tag>

<xxx>content</xxx>

</myapp:tag>

In this case I'm not sure if <xxx> is a variable that needs to be added to My or is something that will be render. (our implementation treat them the same)

I think something like the example bellow will be cleaner, easier to process, easier to understand, and validate also we don't have to deal with name spaces.

<tag key="myapp:tag">

<content key="xxx">content </content>

<var key="colors"><array><item>Red</item></array></var>

<var key="books" value = "${Person.books}" />

</tag>

I also think, as many of you, that is really premature to start voting on 1.0. For us 0.9 still not fully implemented and there has been no time for the users to start playing with it. I think that once the developers star using this version will give us very important feedback of what they like and don't. 

Jorge 

[Arne Roomann-Kurrik]

Note that I wasn't talking about rewriting the template spec from scratch - just fixing the obvious errors and making it more specific where needed (like EL).  Syntax changes like that are out of scope for the time being (but may make good extension proposals?)

~Arne

[Adam Winer]

On Mon, Oct 19, 2009 at 8:39 PM, goosemanjack <cc...@myspace.com> wrote:
>
> I spoke with the team, and at this point we feel the current spec is
> unworkable.  It lacks the detail or the completeness to make it
> practical.  Even a trivial implementation requires numerous extensions
> to make it quasi-functional.  Given that how the EL behaves at a
> global level is such a pervasive and wide-reaching aspect of OSML in
> general, we must insist on clarity. I reviewed internal conversations
> on this aspect of the spec to see what insight I could gain before
> replying.  We've had long-running misgivings about this section in the
> spec, but decided to not oppose it in the 0.9 round in the interest of
> ratifying a version of the spec.  We expected to have some data from
> live implementations to use concrete data from when revisiting this
> item.  Instead the transition from 0.9 to v.next (we're not willing to
> sign off on this being 1.0 yet) comes with neither group being fully
> 0.9 launched with any depth of data, so we're still shooting from the
> hip.
>
> The feeling over here is that if Shindig wants to pursue auto-escaping
> within the EL, they are free to do so and re-propose it as an
> extension.  For now, auto-escaping is unworkable as proposed in the
> spec and will be considered a spec error.  There are no complete
> implementations that implement it to a satisfactory workable model and
> inadequate data on usage patterns to warrant it's inclusion.

... and I'd *really* like you to give specific and complete feedback
about why it is "unworkable" and a "spec error", when it's in the spec
and being used in Shindig to serve huge numbers of requests every day.
I think you're talking about how to do escaping of HTML vs.
Javascript vs. CSS, but I'm just guessing. (And it's entirely true
that Shindig hasn't tackled that, but I don't see it as fundamentally
impossible.)

You've continually stated it just doesn't work, but haven't given us
the hard details *why* it doesn't, which makes it impossible to have a
productive discussion. And you're flipping the onus of proof around
in an incorrect manner: it *is* in the spec, and you have to prove
why it needs to be taken out. Shindig doesn't have to re-propose it
as an extension until after that discussion.

And we do in fact have plenty of data (personally anecdotal, though we
could dig up hard data) about attack vectors due to lack of escaping
prior to Opensocial 0.9.

> --~--~---------~--~----~------------~-------~--~----~
> You received this message because you are subscribed to the Google Groups "OpenSocial and Gadgets Specification Discussion" group.
> To post to this group, send email to opensocial-an...@googlegroups.com
> To unsubscribe from this group, send email to opensocial-and-gadg...@googlegroups.com
> For more options, visit this group at http://groups.google.com/group/opensocial-and-gadgets-spec?hl=en
> -~----------~----~----~----~------~----~------~--~---
>
>

[goosemanjack]

> (And it's entirely true that Shindig hasn't tackled that,
> but I don't see it as fundamentally impossible.)

As I read this, even Shindig is not complying with the contextual
escaping of EL outlined in the spec, having chosen instead to ignore
this problem. This sets off yet another red flag for me and is an
indication that as-written this section on EL escaping rules is
problematic.

We appear to be at an impasse. From the tone of this thread and
arguments on either side, I don't see anyone being convinced to change
their position in the short term. As I see it, the spec group has two
options:

1. Delay or block 1.0 from moving forward until the question of EL
auto-escaping is settled.

2. Agree to disagree on this point and move forward with 1.0, making
resolving EL (and security in general) a high priority item for 1.1.

Option 1 doesn't appear to serve anyone's interests. There are many
good and uncontested proposals out right now. The behavior of
OpenSocial EL is somewhat independent of most of these proposals and
shouldn't be blocking.

Option 2 allows us to focus on areas of mutual agreement while
gathering more usage data. Since a compromise proposal has not been
accepted by Shindig and MySpace is unwilling to give a rubber-stamp on
this item, accepting mutual disagreement allows time for both groups
to work offline to develop something acceptable.

The Shindig and MySpace implementations of OpenSocial have always been
incompatible, as is often the case in early iterations of a spec.
With 0.9 MySpace is beefing up our GadgetXML support to a point where
it is workable with many existing Shindig-based gadgets. We're
comfortable moving forward with 1.0 work under our interpretation of
the current spec.
--
clc

[Arne Roomann-Kurrik]

I think we need to resolve EL (hopefully including escaping) for 1.0.  Obviously it's a contentious discussion and needs time to be resolved for the sake of any developers attempting cross-platform development.

I don't think we need to block progress on this issue right now - it's on the table for discussion for 1.0, and I'd love to see someone step up and try to formalize the templating spec language, especially with regard to the EL section.  Since there's not been an official deadline for finalizing the discussion for all the open spec issues, we still have a bit more time to work this out.  Worst case, we have to close discussion to get 1.0 out the door at some point in the future and we'll roll this to 1.1 (which is equivalent to proposal 2, as I see it).

Personally I would not like to see the same code produce two different outputs across two OpenSocial containers, given the same inputs.  If we can avoid this through the introduction of additional properties or elements, then I'd consider that within the scope for 1.0 and would support any such modification.

~Arne

> > -~----------~----~----~----~------~----~------~--~---

--

You received this message because you are subscribed to the Google Groups "OpenSocial and Gadgets Specification Discussion" group.

To post to this group, send email to opensocial-an...@googlegroups.com.
To unsubscribe from this group, send email to opensocial-and-gadg...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/opensocial-and-gadgets-spec?hl=en.

[Chris Chabot]

On Fri, Oct 23, 2009 at 9:03 PM, Arne Roomann-Kurrik <kur...@google.com> wrote:

I think we need to resolve EL (hopefully including escaping) for 1.0.  Obviously it's a contentious discussion and needs time to be resolved for the sake of any developers attempting cross-platform development.

+1

We often get feedback from developers that cross container compatibility is one of their main issues, embracing and extending this willingly wouldn't do OpenSocial and developers that use it any favors, so if can be avoided I believe we should make all effort to do so.

While a lot of strong language is used "block the spec", "red flags", "unworkable solutions" there hasn't been a lot of in depth discussion about the actual details or time lines involved, I think that if we abandon the superlatives and have an open discussion about what the exact issues are we might unblock this surprisingly easily

   -- Chris

[goosemanjack]

I'm +1 on taking a whack at coming up with escaping behavior we can
all live by. Are we willing to have this proposal morph into
something like "Clarify escaping rules and behavior for EL
statements"? It would involve starting a new thread (for clarity) and
having Jon (the current -1 voter) also agree to this change.

Do we have enough support to move forward with this change of purpose
(currently 3 +1)?

[Arne Roomann-Kurrik]

+1

~Arne

[Jon]

+1
Jon

[Adam Winer]

+1

[goosemanjack]

I have created an updated proposal that hopefully will satisfy
everyone's needs. Review here:

http://wiki.opensocial.org/index.php?title=Clarify_escaping_behavior_of_EL

On Oct 27, 1:10 pm, Adam Winer <awi...@gmail.com> wrote:
> +1
>

[Lev Epshteyn]

Is there a thread for this proposal yet? I have comments, but don't want to resurrect this one, and can't seem to find a newer one.

[goosemanjack]

New thread has been opened for discussion at:

http://groups.google.com/group/opensocial-and-gadgets-spec/t/7a5e227bc2636ff0?hl=en

On Nov 2, 6:02 am, Lev Epshteyn <le...@google.com> wrote:
> Is there a thread for this proposal yet? I have comments, but don't want to
> resurrect this one, and can't seem to find a newer one.
>
> On Fri, Oct 30, 2009 at 6:06 PM, goosemanjack <cc...@myspace.com> wrote:
> > I have created an updated proposal that hopefully will satisfy
> > everyone's needs.  Review here:
>

> >http://wiki.opensocial.org/index.php?title=Clarify_escaping_behavior_...

> > opensocial-and-gadg...@googlegroups.com<opensocial-and-gadgets-spec%2Bunsu...@googlegroups.com>
> > .

> > > > For more options, visit this group athttp://
> > groups.google.com/group/opensocial-and-gadgets-spec?hl=en.
>

> > --
>
> > You received this message because you are subscribed to the Google Groups
> > "OpenSocial and Gadgets Specification Discussion" group.
> > To post to this group, send email to
> > opensocial-an...@googlegroups.com.
> > To unsubscribe from this group, send email to

> > opensocial-and-gadg...@googlegroups.com<opensocial-and-gadgets-spec%2Bunsu...@googlegroups.com>
> > .