Implement "replacement" text encoding.

Implement "replacement" text encoding.

The WHATWG Encoding Standard redefines certain legacy encodings
as that are only used as XSS attack vectors as aliases for an
abstract "replacement" encoding.

* Decode (of resources) terminates with a single U+FFFD.
* Encode (e.g. for form submissions) is treated as UTF-8.
* The JS TextDecoder API throws if the aliases are used.
* The "replacement" string itself isn't assigned any meaning.

The last note can violate assumptions that a codec's name
is always an alias for itself, so it is enforced at
the encoding lookup-by-name function and script entry points.

BUG=277037
R=jshin@chromium.org,eseidel@chromium.org

[jungshik at Google, 2014-02-21 22:27:40]

Should I start reviewing the CL or wait? I was a bit confused because the CL
hasn't been published (but still in my incoming review bucket)? 

BTW, thank you for the CL.

[jsbell, 2014-02-21 22:29:11]

On 2014/02/21 22:27:40, Jungshik Shin wrote:
> Should I start reviewing the CL or wait? I was a bit confused because the CL
> hasn't been published (but still in my incoming review bucket)? 
> 
> BTW, thank you for the CL.

Not ready yet

I just wanted to upload it in case my local machine died. 

I can't remember what's not ready, it's been a while since I looked at it.

[jsbell, 2014-02-21 22:35:55]

https://codereview.chromium.org/145973021/diff/20001/Source/wtf/text/TextEnco...
File Source/wtf/text/TextEncodingRegistry.cpp (right):

https://codereview.chromium.org/145973021/diff/20001/Source/wtf/text/TextEnco...
Source/wtf/text/TextEncodingRegistry.cpp:227:
textEncodingNameMap->remove("replacement");
This seems like a hack, but so far as I can tell the alternatives are either
special-casing "replacement" in various entry points so it can't be referenced
by name, or touching all of the places in this file that assert that a codec
have itself as a label.

[jsbell, 2014-03-10 18:32:57]

jshin@ - I think this is ready for a look now. Can you do an initial review
before I loop in a wtf OWNER?

[jsbell, 2014-03-14 20:28:29]

On 2014/03/10 18:32:57, jsbell wrote:
> jshin@ - I think this is ready for a look now. Can you do an initial review
> before I loop in a wtf OWNER?

Actaully, hold off; I failed to notice in the spec that replacement should emit
a U+FFFD before EOF. Need to fix that,

[jsbell, 2014-03-14 20:34:28]

On 2014/03/14 20:28:29, jsbell wrote:
> On 2014/03/10 18:32:57, jsbell wrote:
> > jshin@ - I think this is ready for a look now. Can you do an initial review
> > before I loop in a wtf OWNER?
> 
> Actaully, hold off; I failed to notice in the spec that replacement should
emit
> a U+FFFD before EOF. Need to fix that,

New patch uploaded, but I have mail out to Anne about this. At least one reader
interpreted the spec as emitting one U+FFFD for each input byte, and we should
clarify before landing.

[jsbell, 2014-03-17 16:02:56]

On 2014/03/14 20:34:28, jsbell wrote:
> New patch uploaded, but I have mail out to Anne about this. At least one
reader
> interpreted the spec as emitting one U+FFFD for each input byte, and we should
> clarify before landing.

This is resolved (the reader said he just misread the spec).

So: replacement emits U+FFFD then EOF, which matches this implementation.

jshin@ - Please take a look?

[jungshik at Google, 2014-04-01 21:39:48]

On 2014/03/17 16:02:56, jsbell wrote:
> On 2014/03/14 20:34:28, jsbell wrote:
> > New patch uploaded, but I have mail out to Anne about this. At least one
> reader
> > interpreted the spec as emitting one U+FFFD for each input byte, and we
should
> > clarify before landing.
> 
> This is resolved (the reader said he just misread the spec).
> 
> So: replacement emits U+FFFD then EOF, which matches this implementation.
> 
> jshin@ - Please take a look?

Sorry for the delay. Thanks for the CL and LGTM !

[jsbell, 2014-04-01 21:50:13]

eseidel@ - can you take a look at the wtf/text changes?

I'm not entirely thrilled with the bait-and-switch approach to getting
"replacement" in the right maps, but a big restructure didn't seem worthwhile
either. Guidance appreciated.

[eseidel, 2014-04-01 22:14:42]

I'm confused as to what this is trying to do and why it needs to wire in in this
manner.  Particularly the "decode" function which ignores the input buffer is
odd.

I take it just a simple mapping of these encodings to utf8 is not sufficient?

[jsbell, 2014-04-01 22:41:58]

On 2014/04/01 22:14:42, eseidel wrote:
> I'm confused as to what this is trying to do and why it needs to wire in in
this
> manner.  Particularly the "decode" function which ignores the input buffer is
> odd.
> 
> I take it just a simple mapping of these encodings to utf8 is not sufficient?

The definition of the "replacement" codec is:

http://encoding.spec.whatwg.org/#replacement

With the text "The replacement encoding exists to prevent certain attacks that
abuse a mismatch between encodings supported on the server and the client." -
basically, these encodings are known attack vectors and we shouldn't even try to
decode them - the data should be dropped on the floor. At least, that's the
conclusion Moz came to.

The mapping of labels is towards the bottom of:

http://encoding.spec.whatwg.org/#names-and-labels

The funky thing about these aliases is that the encoding name itself
("replacement") isn't itself referenceable i.e. you shouldn't be able to use
<meta charset="replacement">

Another option is to simply add these to the list of blacklisted codecs (which
right now has only utf-7) and push back on the Encoding spec about the behavior
i.e. to handle it as utf-8. I have to defer to jshin@ about what the desired
behavior Chrome to do here, though - he may be more versed in the details.

[eseidel, 2014-04-01 23:09:08]

I think we should look at encoding numbers.  Last ones I saw didn't even see
these encodings.  I think we should just remove support for them.

https://docs.google.com/a/google.com/spreadsheets/d/1FkCrFqNsMIG3GK7-dZR1Pqmn...

I Think I have a public version of that link somewhere too.

[eseidel, 2014-04-01 23:12:16]

We should remove these and any other possibly dangerous low-usage encodings.

https://codereview.chromium.org/145973021/diff/130001/Source/wtf/text/TextCod...
File Source/wtf/text/TextCodecReplacement.cpp (right):

https://codereview.chromium.org/145973021/diff/130001/Source/wtf/text/TextCod...
Source/wtf/text/TextCodecReplacement.cpp:28: registrar("csiso2022kr",
"replacement");
36 out of 100M pages surveyed had this encoding.

https://codereview.chromium.org/145973021/diff/130001/Source/wtf/text/TextCod...
Source/wtf/text/TextCodecReplacement.cpp:29: registrar("iso-2022-cn",
"replacement");
7 off 100M pages.

https://codereview.chromium.org/145973021/diff/130001/Source/wtf/text/TextCod...
Source/wtf/text/TextCodecReplacement.cpp:30: registrar("iso-2022-cn-ext",
"replacement");
No data, presumably 0 pages.

https://codereview.chromium.org/145973021/diff/130001/Source/wtf/text/TextCod...
Source/wtf/text/TextCodecReplacement.cpp:31: registrar("iso-2022-kr",
"replacement");
I suspect this is lumped in with the 36 above. :)

[jsbell, 2014-04-01 23:26:43]

On 2014/04/01 23:12:16, eseidel wrote:
> We should remove these and any other possibly dangerous low-usage encodings.

Then the question is: what does it mean to "remove" them? Treat as UTF-8 for
decode as we would for an unknown encoding alias, treat as something else (e.g.
x-user-defined), or do the "replacement" behavior (which is reject on decode,
utf-8 on encode)?

Again, hoping jshin@ can jump in here, as I only know enough to be paranoid.

[jungshik at Google, 2014-04-02 00:36:40]

On 2014/04/01 23:26:43, jsbell wrote:
> On 2014/04/01 23:12:16, eseidel wrote:
> > We should remove these and any other possibly dangerous low-usage encodings.
> 
> Then the question is: what does it mean to "remove" them? Treat as UTF-8 for
> decode as we would for an unknown encoding alias, treat as something else
(e.g.
> x-user-defined), or do the "replacement" behavior (which is reject on decode,
> utf-8 on encode)?
> 
> Again, hoping jshin@ can jump in here, as I only know enough to be paranoid.

I agree about the need to get rid of them (including HZ-GB) and also agree that
the question is how. To me, W3C encoding spec's approach is pretty 'secure' if
not very pretty. 

Treating it as UTF-8 (or ASCII) might have a visual-spoofing (although machines
will not be spoofed because ESC, SI, SO etc will be caught).

[jungshik at Google, 2014-04-02 00:37:05]

+tsepez for securtiy perspective.

[eseidel, 2014-04-02 04:47:02]

I'm confused.  Why can't we just register a decoder for these 4 types w/o all
the other hackery?

[eseidel, 2014-04-02 04:48:27]

We should just completely remove support for these encodings in whatever is
deemed a secure way.  What do we do for the "foo-bar" encoding?  We just treat
it as utf8, right?   Or I guess we just ignore the unrecognized name and try to
detect what the actual encoding?  I'm unclear as to the risk of doing the same
for these?

[jungshik at Google, 2014-04-02 10:47:15]

On 2014/04/02 04:48:27, eseidel wrote:
> We should just completely remove support for these encodings in whatever is
> deemed a secure way.  What do we do for the "foo-bar" encoding?  We just treat
> it as utf8, right?   Or I guess we just ignore the unrecognized name and try
to
> detect what the actual encoding?  I'm unclear as to the risk of doing the same
> for these?

For HZ-GB and UTF-7 (that do not use any "default-invisible' control
characters), treating it as UTF-8 might be ok. These two encodings are not
related to this CL, though.


For ISO-2022-* encodings (that do use 'default-invisible' control characters),
treating the input as UTF-8 (or any ASCII-compatible encodings) still poses some
risks. 
See http://crbug.com/15701 (ISO-2022-KR) and http://crbug.com/21354.  See also
http://crbug.com/114941

To take care of these two bugs, I added back the support for ISO-2022-KR and
'pseudo-conversion tables' for ISO-2022-CN(-Ext) to Chrome's copy of ICU. What
'pseudo-conversion tables' do is to turn everything between escape-sequences to
null/empty (or a substitution character - I have to check). However, if we treat
them as replacement-encoding per the current version of the encoding spec, we
can avoid the issue raised in bug 114941.

[jungshik at Google, 2014-04-02 10:50:39]

> What do we do for the "foo-bar" encoding?  We just treat
> it as utf8, right?   Or I guess we just ignore the unrecognized name and 
> try to detect what the actual encoding?

With AutoDetector OFF (which is the default), 'foo-bar' would be treated as the
default character (Latin1/Win 1252 in case of English/French/etc UI). With
AutoDetector ON, they'd be subject to the autodetection. 

Anyway, see bug 15701 and 21354 about the past issue arising from that.

[jungshik at Google, 2014-04-02 17:03:38]

I read http://crbug.com/21354. What I did for ISO-2022-CN(-Ext) is that turn
every 'character' (that is usually represented in two ASCII bytes) to U+FFFD.
This was done without any code change in Blink but was done by adding a
fake-conversion table to the ICU. 

This decoding behavior is similar to but different from the encoding spec.  The
encoding behavior is entirely different. The current behavior is turn every
Unicode character to a substitution character (due to an empty fake conversion
table in ICU).

[jungshik at Google, 2014-04-02 17:52:26]

@jsbell, what does the encoding spec say about 'charset=foobar' (unrecognized
labels)? Apparently, if an encoding name/label is unrecognized, the decoder is
supposed to throw a 'type error'. What does it mean for a web page loading (as
opposed to a 'decode()' call in JS)?  

The current Blink behavior is 1) assume the default encoding (if AutoDetection
is OFF, which is defalut out of box in Chrome) 2) run our autodetector (if it is
turned on by a user). 

I think this behavior is not desirable. We'd better turn the input to U+FFFD as
we do with 'replacement encodings'. So, instead of having a special class of
encodings ('replacement encoding'), I think it's better to treat all the
unrecognized encodings (not listed in the encoding spec) in an identical manner
that is secure. One possibility is the current behavior defined for 'replacement
encodings'.

[eseidel, 2014-04-02 18:00:15]

In summary, I think we should add the minimum amount of complexity necessary to
remove support for all low-usage encodings in as secure a way as possible.  If
that means adding a decoder which turns all low-usage encodings into just the
replacement character that's fine with me.

Continuing to focus on the 99.9999% of users who never see these encodings and
making their Chrome browsing experience as safe and as fast as possible (by
keeping Chrome code as simple as possible) is my goal. :)

I'm OK with this patch, I'm also OK with something simpler and more aggressive. 
l-g-t-m either way.

[jsbell, 2014-04-02 18:07:37]

On 2014/04/02 17:52:26, Jungshik Shin wrote:
> @jsbell, what does the encoding spec say about 'charset=foobar' (unrecognized
> labels)? Apparently, if an encoding name/label is unrecognized, the decoder is
> supposed to throw a 'type error'.

Correct. The API (being new) is conservative and throws.

> What does it mean for a web page loading (as
> opposed to a 'decode()' call in JS)?  

For a page, the HTML spec takes precedence:

http://www.whatwg.org/specs/web-apps/current-work/multipage/parsing.html#dete...

... which says that the default is going to be locale-dependent. When the <meta>
tag is spotted,
http://www.whatwg.org/specs/web-apps/current-work/multipage/semantics.html#ch...
says it must be a match for a label in [ENCODING]. It's not, so that fails and
it's ignored, so we're back to the default.
 
> The current Blink behavior is 1) assume the default encoding (if AutoDetection
> is OFF, which is defalut out of box in Chrome) 2) run our autodetector (if it
is
> turned on by a user). 

... which sounds like it matches the HTML spec, for encodings like "foobar" that
are not listed in [ENCODING].

[jungshik at Google, 2014-04-02 19:07:03]

On 2014/04/02 18:07:37, jsbell wrote:
> On 2014/04/02 17:52:26, Jungshik Shin wrote:

> For a page, the HTML spec takes precedence:
> 
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/parsing.html#dete...
> 
> ... which says that the default is going to be locale-dependent. When the
<meta>
> tag is spotted,
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/semantics.html#ch...
> says it must be a match for a label in [ENCODING]. It's not, so that fails and
> it's ignored, so we're back to the default.
>  
> > The current Blink behavior is 1) assume the default encoding (if
AutoDetection
> > is OFF, which is defalut out of box in Chrome) 2) run our autodetector (if
it
> is
> > turned on by a user). 
> 
> ... which sounds like it matches the HTML spec, for encodings like "foobar"
that
> are not listed in [ENCODING].

Thanks for the reply. I should have known that :-). Anyway, I realized that
treating all 'unknown encoding labels' as replacement encoding does not have
much merit from the security pov because invisible-control-character-ridden
attack vectors can be just made by not specifying encoding at all instead of
labeling them as an unrecognized encoding. So, we have to find a way to block
this kind of attack vectors, I guess.

[jsbell, 2014-04-14 21:28:52]

Sorry about the long cycle time on this CL...

On 2014/04/02 04:47:02, eseidel wrote:
> I'm confused.  Why can't we just register a decoder for these 4 types w/o all
> the other hackery?

I've simplified this remove the hackery. The registration of replacement
encodings is now explicitly done by the TextEncodingRegistry itself, similar to
the other alias churn. Less self contained, but more explicit which I think is
more maintainable. It also moves the registration into the extendTextCodecMaps()
which means no extra work is done in the common (latin1/utf-8/16) cases.

eseidel@ - how does this look?

jshin@ - should we go forward with this? Should I go ahead and add hz-gb-2312
per https://www.w3.org/Bugs/Public/show_bug.cgi?id=25339 ?

[jsbell, 2014-04-21 17:11:40]

eseidel@, jshin@ - ping? See #msg26

[jungshik at Google, 2014-04-21 22:36:03]

On 2014/04/14 21:28:52, jsbell wrote:
> Sorry about the long cycle time on this CL...
> 
> On 2014/04/02 04:47:02, eseidel wrote:
> > I'm confused.  Why can't we just register a decoder for these 4 types w/o
all
> > the other hackery?
> 
> I've simplified this remove the hackery. The registration of replacement
> encodings is now explicitly done by the TextEncodingRegistry itself, similar
to
> the other alias churn. Less self contained, but more explicit which I think is
> more maintainable. It also moves the registration into the
extendTextCodecMaps()
> which means no extra work is done in the common (latin1/utf-8/16) cases.
> 
> eseidel@ - how does this look?
> 
> jshin@ - should we go forward with this? Should I go ahead and add hz-gb-2312
> per https://www.w3.org/Bugs/Public/show_bug.cgi?id=25339 ?

Yes, I'd love to see that happen :-). Then, I'll adjust the icu to get rid of a
bit more data *and* code (perhaps, together, we can save > 10kB or so).

[jungshik at Google, 2014-04-21 22:46:33]

On 2014/04/21 17:11:40, jsbell wrote:
> eseidel@, jshin@ - ping? See #msg26

Thank you and sorry for the delay. LGTM from me with hz-gb added to the list.

[jsbell, 2014-04-24 20:53:05]

On 2014/04/21 22:46:33, Jungshik Shin wrote:
> On 2014/04/21 17:11:40, jsbell wrote:
> > eseidel@, jshin@ - ping? See #msg26
> 
> Thank you and sorry for the delay. LGTM from me with hz-gb added to the list.

hz-gb-2312 added.

eseidel@ - can I bug you for another look, or can you recommend another wtf
owner?

[jsbell, 2014-05-01 16:15:44]

FYI, this patch still needs rethinking on my part. The assumption (and ASSERTs)
that an encoding's name is an alias for itself is used too widely in Blink to
break it.

The change to pass the label and encoding name separately - removed in PS#9
https://codereview.chromium.org/145973021/#ps200001 - had originally been placed
there to avoid this problem (c++ pseudocode):

encoding = TextEncoding(alias);
newTextCodec(encoding.name());

... which would ASSERT in newTextCodec() e.g. for "iso-2022-kr", since
encoding.name() is "replacement". This can be prevented by explicitly testing
encoding.name() before calling newTextCodec(), but this pattern likely repeats
elsewhere in the codebase so it can't be trusted.

I believe it will be necessary to leave the "encoding is alias for itself"
invariant intact and block "replacement" at the use points (e.g. HTML parse,
Encoding API) as eseidel@ suggested on IRC.

...

I can probably split this patch up into adding TextCodecReplacement with its
aliases including "replacement" (which violates the spec, but shouldn't break
any content), then a series of follow-up patches that detect the "replacement"
string from various entry points and bail.

[eseidel, 2014-05-01 17:28:35]

sgtm.

[jsbell, 2014-05-02 18:24:51]

Closing this. I've got a set of 3 CLs lined up:

1: script API impl simplification and test cleanup -
https://codereview.chromium.org/269593009 - no wtf changes

2: Introduce TextCodecReplacement and tests - no script API changes

3: Block use of 'replacement" alias from content/script, add tests