Proposal: clear description of (un)escapeString

[Reinoud Elhorst]

In the discussion on whether or not AppData should be escaped, I ran
into a rather strange implementation in Shindig of escapeString. In
the current implementation:

gadgets.util.escapeString("&") == "&"

Now looking at the spec ( http://code.google.com/apis/opensocial/docs/0.7/reference/gadgets.util.html#escapeString
) I'm not entirely sure Shindigs implementation is valid (that should
be a JIRA though, and not on this list), I do believe that the spec
should be clearer on what exactly is being escaped in in which way.
Gadget developers are going to rely on this function for security, and
therefor I think this function should output exactly the same string
for each possible input on all containers.

Now I don't know if there is a standard for HTML escaping (like for
example the url escaping in RFC3986), but if there is, that should be
used, else we should specify our own behaviour.

unescapeString can then be specified as the inverse function of
escapeString:

unescapeString(escapeString(x)) == x for all x
(in the current Shindig implementation,
unescapeString(escapeString("&")) == "&" )

or could use some standard for unescaping HTML

[Arne Roomann-Kurrik (Google)]

I'm in favor of making this more explicit in the spec, although I'm
not really sure which characters should be expected to be escaped -
could someone with more insight into this (Kevin, Brian?) elaborate a
bit?

~Arne

On Apr 17, 3:59 am, Reinoud Elhorst <rein...@hyves.nl> wrote:
> In the discussion on whether or not AppData should be escaped, I ran
> into a rather strange implementation in Shindig of escapeString. In
> the current implementation:
>
> gadgets.util.escapeString("&") == "&"
>

> Now looking at the spec (http://code.google.com/apis/opensocial/docs/0.7/reference/gadgets.uti...

[Brian Eaton]

gadgets.util.escapeString() should be idempotent: escaping the same
string N times should give you the same value as escaping the string
once.

gadgets.util.escapeString() should make arbitrary text safe to include
as content in a normal HTML element, e.g. <b>, <div>.

gadgets.util.escapeString() should make arbitrary text safe in
javascript strings. (We may be here, not sure.)

gadgets.util.escapeString() should make arbitrary text safe for some
HTML attributes. (This is definitely ill-specified at the moment.)

iGoogle defined their equivalent of escapeString to escape <>"', I
believe. We could make the opensocial function equivalent, but that
would restrict it's usage in javascript strings and HTML attributes.

Cheers,
Brian

[Zhen Wang]

I wonder why idempotency is a desired feature of gadgets.util.escapeString().

If escapeString("<") and escapeString("&lt;") both return "&lt;", what
are the expected results of gadgets.util.unescapeString("&lt;") and
gadgets.util.unescapeString("&amp;lt;")?

[Zhen Wang]

I think we should make sure that
gadgets.util.unescapeString(gadgets.util.escapeString(S)) == S.

[Kevin Brown]

On Thu, Apr 24, 2008 at 6:32 PM, Zhen Wang <wa...@google.com> wrote:

I wonder why idempotency is a desired feature of gadgets.util.escapeString().

To prevent the common case of double escaping, of course. We can make it idempotent while still escaping &, it's just a little harder to implement.

I actually really dislike escapeString for a few reasons (I prefer sanitization), but since it exists I do think we should be explicit about what is escaped.

Brian Eaton and I determined that the following must be escaped to avoid problems (copied from Shindig source):

  // Maps code points to the value to replace them with.
  // If the value is "false", the character is removed entirely, otherwise

  // it will be replaced with an html entity.
  var escapeCodePoints = {
   // nul; most browsers truncate because they use c strings under the covers.

   0 : false,
   // new line
   10 : true,
   // carriage return

   13 : true,
   // double quote
   34 : true,
   // single quote

   39 : true,
   // less than
   60 : true,
   // greater than
   62 : true,

   // Backslash
   92 : true,
   // line separator
   8232 : true,

   // paragraph separator
   8233 : true
  };

Ideally, I'd prefer:

unescape(escape(S)) === S

and

escape(escape(S)) === escape(S)

[Zhen Wang]

On Thu, Apr 24, 2008 at 7:51 PM, Kevin Brown <et...@google.com> wrote:
>
> Ideally, I'd prefer:
>
> unescape(escape(S)) === S
>
> and
>
> escape(escape(S)) === escape(S)

Hmm... I don't think that's ideal because unescape() would then have
to be an identity function to satisfy both conditions.

Given the two premises:

unescape(escape(S)) === S (1)
escape(escape(S)) === escape(S) (2)

It can be deduced from (1) that:
unescape(unescape(escape(S))) === unescape(S) (3)

It can be obtained from (2) that:
unescape(unescape(escape(S))) === unescape(unescape(escape(escape(S)))) (4)

Substitute "escape(S)" into S in (3), and we obtain from (4) that:
unescape(unescape(escape(escape(S)))) === unescape(escape(S)) (5)

From (3), (4), (5), and (1) we then have:
unescape(S) === S

[Zhen Wang]

A less formal proof:

Let's call S a "terminal" string if escape(S) === S; otherwise,
"non-terminal". An idempotent escape() maps non-terminal strings to
terminal strings and terminal strings to themselves. If escape() is
not a trivial identity function -- in other words, if the set of
non-terminal strings is not empty -- at least one non-terminal string
and one terminal string map to the same terminal string which can't be
correctly decoded by unescape() unless both unescape() and escape()
are identity functions.

[Brian Eaton]

On Thu, Apr 24, 2008 at 7:51 PM, Kevin Brown <et...@google.com> wrote:

> I actually really dislike escapeString for a few reasons (I prefer
> sanitization), but since it exists I do think we should be explicit about
> what is escaped.

Please go say "+1" to the spec proposal to add a sanitization function. =)

[Kevin Brown]

Er, I didn't see this one. Is it a part of another thread?

[Brian Eaton]

Search your mail for "spec proposal: HTML sanitizer". Adding lryan to
the CC list since he told me at point he thought it ought to happen,
and he hasn't voted either...

[Robert Evans]

Seems like it would be more proper to fill in the intermediaries and it might get around this problem:

bad_chars: {<,>,...}
all_chars: bad_chars ∪ unicode_charsets

S: {s| s ∈S, s ∈ all_chars}
escape(S): S',  {s'| s' ∈S', s' ∉ bad_chars }
unescape(S'): S,{s| s ∈S, s ∈ all_chars }
escape(S'): S'

[Reinoud Elhorst]

Zhen is right that the only function that is idempotent and has a reverse (as in f'(f(x)) == x )  is the identity function. If there is anyone in doubt, we should clear that out of the way first.

This discussion is actually just what I was looking for, I realize now that the current implementation in Shindig is intentional. So the more fundamental discussion is, which do we find important, that escapeString is idempotent, or that unescapeString(escapeString(S)) == S. Whatever the result of this discussion is, should be made clear in the spec (or perhaps there should be a function for both uses).

Personally, I think it more appropriate that unescapeString(escapeString(S)) == S, and if el.innerHTML = escapeString(S), then the user should see S as content of the element in the browser. The reasons for this:

1)  I actually think that the spec is demanding that: http://code.google.com/apis/opensocial/docs/0.7/reference/gadgets.util.html#unescapeString

<static> String unescapeString(str)

Reverses escapeString.

2) It is what other programming languages do when html-escaping strings
3) If I type some story (e.g. "Don't double-escape, because then & will look like &amp;"), I would really hate it if some smart escaper decided not to escape the latter "&", and have my story read "Don't double-escape, because then & will look like &"
4) There are some places in the spec where auto-escaping occurs (although I'm really hoping for a workaround for that: http://groups.google.com/group/opensocial-and-gadgets-spec/browse_thread/thread/974695cf0d0140ff/e04f8ff0e0d93a59 !). If there is no reverse function, how is the developer going to get back his original data.
5) Non-escaping of & results in broken HTML (although this will not be visible on most browsers, I know quite some mobile browsers that will break on it; admittedly, these mobile browsers don't have javascript)

If we agree on these properties, I will try and see what the exact spec should look like, and submit a patch for Shindig.

[Cassie]

+1 on unescapeString(escapeString(S)) == S  and everything else Reinoud said

(btw Zhen, those proofs made me feel like I was in college again, lol)

[Kevin Brown]

On Fri, Apr 25, 2008 at 5:56 AM, Reinoud Elhorst <rei...@hyves.nl> wrote:

Zhen is right that the only function that is idempotent and has a reverse (as in f'(f(x)) == x )  is the identity function. If there is anyone in doubt, we should clear that out of the way first.

Yes, Zhen is right, and I haven't been sleeping enough recently (or maybe I'm just brain damaged from all this spec discussion) :).
 

[Kevin Brown]

Er, and to add to this --

If the decision is that escape is not idempotent, then we really *must* remove auto escaping to prevent all sorts of weirdness. If memory serves me correctly (and it might not, given what I just said), the reason for making it idempotent in the first place was because we were seeing situations where the auto escaping was happening and then the developer was manually calling escape().

[Graham Spencer]

+1 for Reinoud's message, unescapeString(escapeString(S)) == S.

--g

[Brian Eaton]

On Fri, Apr 25, 2008 at 9:58 AM, Kevin Brown <et...@google.com> wrote:
> If the decision is that escape is not idempotent, then we really *must*
> remove auto escaping to prevent all sorts of weirdness. If memory serves me
> correctly (and it might not, given what I just said), the reason for making
> it idempotent in the first place was because we were seeing situations where
> the auto escaping was happening and then the developer was manually calling
> escape().

Not doing autoescaping leads immediately to XSS everywhere. If
removing autoescaping is a goal of this proposal, -1.

(Tempted to go register throw-away e-mail accounts to vote -1 more
times on any such proposal.)

[Peter Valchev]

-1 from me too.

When we discussed this and the change was made, I did bring up the
point of escaping & to avoid these issues, but it was decided to stick
to not escaping & due to double escaping, etc. Still, this had no
security implications so we agreed on it - but it does bring up some
issues brought up above that I do agree with. Nevertheless, there is a
reason we settled on this solution as a compromise, and not escaping
stuff at all is currently not an option, until we can override
innerHTML with a html-sanitized version (caja). The risk is too big,
and this was already decided/discussed at meetings months ago.

The idea about providing (non-default!) ways to stop escaping is OK.

[Kevin Marks]

This is where escaping in the right place is key. Idempotency is what you want in the final output case. You should keep the intermediate data in natural form (unescape on the way in) , and escape on output.

There are lots of different kinds of escaping and encoding involved at different layers of web development; you want your data in a pure form (ie unicode without escaping) when you are manipulating it with your programming language, and you want to add the escaping required by whatever you are doing (generating HTML, generating XML, generating Javascript, generating URLs) in that output phase.

Yes, development tool chains often get this wrong, causing vulnerabilities, but that means you need to catch them at the right place. Caja is the ne plus ultra of this approach.

[Louis Ryan]

+1 on unescape(escape(s)) == s

-1 on removal of autoescaping without a safety net.

An open question is whether we can achieve a safety net that is approximately as effective with the proposed sanitizeHTML method without requiring Caja?

[Peter Valchev]

On Fri, Apr 25, 2008 at 1:27 PM, Louis Ryan <lr...@google.com> wrote:
> +1 on unescape(escape(s)) == s
>
> -1 on removal of autoescaping without a safety net.
>
> An open question is whether we can achieve a safety net that is
> approximately as effective with the proposed sanitizeHTML method without
> requiring Caja?

Since caja has a client-side JS innerHTML sanitizer it should be easy
to copy this and tell people to use it. The problem is, we can't
guarantee (or force) that people use it without something like Caja.
Since we don't control what gadgets people write, arguably that won't
solve anything. But it would still be nice to have...

[Brian Eaton]

On Fri, Apr 25, 2008 at 1:27 PM, Louis Ryan <lr...@google.com> wrote:

> An open question is whether we can achieve a safety net that is
> approximately as effective with the proposed sanitizeHTML method without
> requiring Caja?

Not possible, unless you want to replace the default string escaping
with default HTML sanitizing instead. That seems contrary to the
general theme of the comments on this thread, however.

[Reinoud Elhorst]

OK, I'm slowly starting to see what the rationale behind this was.

I think the main problem why the idempotence was needed, was the implicit/magic/automatic html-escaping on data on some places (when getting appdata, and on getField). I think in http://groups.google.com/group/opensocial-and-gadgets-spec/browse_thread/thread/974695cf0d0140ff/e04f8ff0e0d93a59 we more or less agree to make the escaping more explicit by having an opt_param that controls it (even though the default is to escape the data). In addition we'll update the spec so that it is very clear on what places auto-HTML-escaping occurs. In this case, gadget developers know what data they can trust to be HTML escaped (and does not need any escaping afterwards). Hereby avoiding the problem that data gets escaped twice.

I wouldn't mind diving deeper into the whole escaping controversy for the 0.9 release ( I actually think String should be extended to contain extra data on what kind of escaping has been done on it; not sure whether something like that is in Caja). I do think that for the 5 reasons I stated before that something needs to be done for the 0.8 spec.

My proposal is:
Let browser(x) be the function that the browser uses to translate from HTML-->text. In that case, (un)escape should be defined as:
unescape(escape(S)) == S
and
browser(escape(S)) == S
(note that unescape and browser do not have to be the same function, they are only guaranteed to output the same on the result from the escape function)


I'd like to hear from Kevin Brown, Brian and Peter whether this is something they can live with. Kevin Marks, I do agree that escaping is done (always in the 0.7 spec, by default in the 0.8 spec (after the change suggested in the thread mentioned above)) on the wrong place, please feel free to chime in on that other thread.

As to HTML sanitizing: I do agree that we need something for that as well (I think that made it into the 0.8 spec already), but for me, the main goal of the HTML-escape function is to allow the browser to display the text I entered (which, by definition, removes any markup, so makes it safe to display).

[Cassie]

the escaping relating threads all seemed to get side tracked pretty easily :)
the only thread which is actually discussing whether data should be auto escaped is here:
http://groups.google.com/group/opensocial-and-gadgets-spec/browse_thread/thread/974695cf0d0140ff

okay, whether or not we auto escape aside...

- I think the -1s were all for removing auto-escaping which isn't up for vote here
- we have 3 +1s I think for the "unescape(escape(S)) == S" portion of Reinoud's latest statement
- we only have one +1 on Reinoud's latest statement (which comes from him)

So, to be completely clear here I'd like to wait until some more people confirm that this last thread comment is what we all want. Escaping is an important issue so I want to be sure to get it right :)

Thanks!

- Cassie

[Brian Eaton]

On Sun, Apr 27, 2008 at 1:30 AM, Reinoud Elhorst <rei...@hyves.nl> wrote:
> My proposal is:
> Let browser(x) be the function that the browser uses to translate from
> HTML-->text. In that case, (un)escape should be defined as:
>
> unescape(escape(S)) == S
> and
> browser(escape(S)) == S
> (note that unescape and browser do not have to be the same function, they
> are only guaranteed to output the same on the result from the escape
> function)

Hrm. I'm confused. Can you explain this a different way, perhaps
with some examples?

> I'd like to hear from Kevin Brown, Brian and Peter whether this is something
> they can live with.

I'm not sure giving up idempotence is a good idea. I like the idea of
being able to tell gadget developers "gadgets.util.escapeString is
magic security dust, sprinkle it liberally around anything you are
going to assign to innerHTML". That's not a good idea unless
escapeString is idempotent, because you end up with double-escaping
issues.

Compare that with the explanation you need to give to a gadget
developer about how to avoid XSS if escapeString isn't idempotent.
They need to rewrite their code to make sure that escaping only
happens in a single place, and adding a new escape or unescape may
introduce either functionality or security bugs. A non-idempotent
escapeString will make people work harder to be secure.

All that said, having escapeString() and unescapeString() not be
functional inverses is pretty weird. The weirdness might be worth it,
since it makes escapeString easier to use. Assuming we have a solid
definition of what escapeString and unescapeString do and how they
should be used, can we live with the weirdness?

> I wouldn't mind diving deeper into the whole escaping controversy for the 0.9 release

Agreed.

Cheers,
Brian

[Zhen Wang]

I'm not aware of any other unescaping API that doesn't always return
the same string you pass to its escaping counterpart. I honestly doubt
(un)escapeString will be easier to use if developers have to learn the
quirky escaping/unescaping rules.

Seriously, can we not treat gadget developers like they are
incompetent? (Hey, we have made you a knife. We don't trust you not to
cut your fingers off, so the blade is plastic and blunt. We know it
looks funny and doesn't match the fork. Just deal with it.)

[Reinoud Elhorst]

On Mon, Apr 28, 2008 at 4:34 PM, Brian Eaton <bea...@google.com> wrote:

On Sun, Apr 27, 2008 at 1:30 AM, Reinoud Elhorst <rei...@hyves.nl> wrote:
> My proposal is:
> Let browser(x) be the function that the browser uses to translate from
> HTML-->text. In that case, (un)escape should be defined as:
>
> unescape(escape(S)) == S
> and
> browser(escape(S)) == S
>  (note that unescape and browser do not have to be the same function, they
> are only guaranteed to output the same on the result from the escape
> function)

Hrm.  I'm confused.  Can you explain this a different way, perhaps
with some examples?

Since the browser() function is browser-specific, it would be a lot of trouble to introduce a unescape function that is exactly the same.

For example, I can see how certain IE version might show the html " &MSspecificLeftQuote; " as some sort of quote, whereas firefox will just show the text " &MSspecificLeftQuote; ". Our unescape function need not care about this, since the escape function will never create the &MSspecificLeftQuote; statement; If, much like the current function, escape will only create &#xxx; values, unescape only needs to worry about those. It would be OK with me if unescape will not transform &quot; into ", as long as escape() never creates an &quot;
 

> I'd like to hear from Kevin Brown, Brian and Peter whether this is something
> they can live with.

I'm not sure giving up idempotence is a good idea.  I like the idea of
being able to tell gadget developers "gadgets.util.escapeString is
magic security dust, sprinkle it liberally around anything you are
going to assign to innerHTML".  That's not a good idea unless
escapeString is idempotent, because you end up with double-escaping
issues.

Compare that with the explanation you need to give to a gadget
developer about how to avoid XSS if escapeString isn't idempotent.
They need to rewrite their code to make sure that escaping only
happens in a single place, and adding a new escape or unescape may
introduce either functionality or security bugs.  A non-idempotent
escapeString will make people work harder to be secure.

All that said, having escapeString() and unescapeString() not be
functional inverses is pretty weird.  The weirdness might be worth it,
since it makes escapeString easier to use.  Assuming we have a solid
definition of what escapeString and unescapeString do and how they
should be used, can we live with the weirdness?

I'll comment on the rest of the pixie-dust :) after I've had some time to think about it, I just wanted to share this thought with you:

I don't think anyone cares about idempotence. The thing that matters is that:
browser(escape(S)) == browser(escape(escape(S)))
if I understand the comments made here correctly.

For instance, the escape function could escape:
"you & me"
to
"you &amp; me"
and escape that to
"you &amp;<!-- 1 escape --> me"
and escape that to
"you &amp;<!-- 2 escape --> me"
etc.
No idempotence, still the browser will always display the same thing!
That way we could make an inverse function to escape() (although both the escape and unescape will become much more complicated)

I still would have very ambivalent feelings about that though, because it will still not result in my story:"Don't double-escape, because then & will look like &amp;" being shown correctly.

[Reinoud Elhorst]

I think the main problem here is that we assume that escape is only useful for security. I don't believe that: escaping is making sure that whatever string you put in will display exactly te same in the browser. This does result in security-improvement, but that is not the main goal of escaping data (not from my point of view as webdeveloper in any case).

Security, on the other hand, is obtained through sanitizing functions. Whether this is a simple "strip all HTML tags", or "kill all non-alphanumeric characters", or super complicated caja HTML sanitizer, sanitazion functions are the "magic security dusts" that can not be spilled around too often (except for performance reasons). (I'm starting to see why Kevin Brown talked about them before)

We will have an HTML sanitizer in the 0.8 spec, I would suggest to urge gadget developers to use that function.

In addition, I fully agree with Zhen, I think we can trust gadget developers up to a certain level. There may be a couple of apps out there that will end up with double escaping because of this, but I'm sure that will solve itself over time. In the meantime, no security holes are created because of the suggested change.
 

[Cassie]

Alright, let's continue this discussion and get this all into 0.9
This is closed for 0.8

Thanks.
- Cassie