1307321 - About a truncated csp violation report

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[John Morrison [:jrgm]]

:stomlinson included me on an email thread earlier. I had a look at some
network traces, and I'm filing this bug. I'm not sure what to do with
the traffic sample as it contains some PII (not much, but enough {IP,
UA, the script-sample}. I'll send it to you outside the bug report.

At any rate, as Shane noted, we've been seeing these csp violation
reports that result in HTTP 500 errors. The HTTP request has a valid
Content-Length, but, by inspection, the original JSON serialization had
a larger size than the advertised Content-Length. The common feature I
have seen in these truncated requests is that the `script-sample`
portion contains mostly non-ascii encodings. I suspect that the
Content-Length in the request is being set to the _character_length_ of
the JSON string serialization, not to the _byte_length_ of the JSON
string (Buffer.byteLength() vs. String.prototype.length, in nodejs
terms).

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Thanks for filing the bug. So far I haven't heard any complaints about our csp-reports, but your explanation sounds plausible. In fact, CSP does not generate the actual JSON, it just provides the values for a webidl generated JSON [1]. The reporting was introduced within Bug 1033423 [2]. Unfortunately I don't know much about how the JSON is actually generated. Probably bholley? Or at least he could guide us to the right people.

[1] https://hg.mozilla.org/mozilla-central/annotate/955840bfd3c20eb24dd5a01be27bdc55c489a285/dom/security/nsCSPContext.cpp#l872
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1033423

Comment 2

[Bobby Holley (:bholley)]

Comment 0 doesn't fault the JSON, it faults the Content-Length of a network request somewhere.

WebIDL JSONification happens in generated code in dom/bindings. The networking stuff might be in the dom/base XHR code, or in Necko.

Comment 3

[John Morrison [:jrgm]]

Yep. I don't think the JSON serialization is wrong. It's just that the Content-Length in the network request does not match the actual byte-length of the payload. When the server endpoint consumes this HTTP request, it sees a truncated JSON string.

To clarify my slightly garbled comment #0, in the usual, non-500 case, the javascript script-sample is all ASCII characters.

For the failing cases, the javascript script-sample contains multi-byte characters, and the Content-Length, while it is a non-zero integer, it clearly is less than the actual byte length of the full JSON serialization.

Comment 4

[John Morrison [:jrgm]]

> It's just that the Content-Length in the network request does not match the actual byte-length
> of the payload.

Bah. I meant to say "... the actual byte-length of the full JSON serialized string it is intending to send".

Comment 5

[Ryan Kelly [:rfkelly]]

For example, this line looks pretty suspicious:

  https://dxr.mozilla.org/mozilla-central/rev/7be6b348c431d69f96f0765af3a0c0a0fe56d4bf/dom/security/nsCSPContext.cpp#968

It's converting a UTF16 string to UTF8, then providing the length of the original UTF16 string.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ryan Kelly [:rfkelly] from comment #5)
> For example, this line looks pretty suspicious:
> 
>  
> https://dxr.mozilla.org/mozilla-central/rev/
> 7be6b348c431d69f96f0765af3a0c0a0fe56d4bf/dom/security/nsCSPContext.cpp#968
> 
> It's converting a UTF16 string to UTF8, then providing the length of the
> original UTF16 string.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307321_csp_report_json.patch

(In reply to Ryan Kelly [:rfkelly] from comment #5)
> It's converting a UTF16 string to UTF8, then providing the length of the
> original UTF16 string.

That is indeed a very good catch Ryan - thanks!


John, that looks like the cause of the problem. I updated the testcase as well:
-> csp_report.Length: 329
-> utf8CSPReport.Length: 332

In the test, if we use UTF16 characters as the scriptsample, then verification fails and hence the test fails. So the only thing we need to update for the test is send some UTF16 chars.

Comment 8

[John Morrison [:jrgm]]

Comment on attachment 8800573 [details] [diff] [review]
bug_1307321_csp_report_json.patch

Looks reasonable to me, but it's been a looong time since I wrote any cpp code in Gecko land, so you should probably get a real peer review ;-)

One small suggestion: use this string to test '$£¥µ北

Comment 9

[John Morrison [:jrgm]]

And thanks for the fix!

Comment 10

[John Morrison [:jrgm]]

Hmm, Bugzilla appears to have mangled the string I added above. In JS, it would be:
 '\u00a3\u00a5\u00b5\u5317\ud841\udf79'

Comment 11

[John Morrison [:jrgm]]

Bugzilla's database, perhaps, does not use utf8mb4 encoding, hence it truncate '\ud841\udf79'.

Comment 12

[Ryan Kelly [:rfkelly]]

Heh, yeah, bugzilla eats high unicode characters - Bug 1253535

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307321_csp_report_json.patch

(In reply to John Morrison [:jrgm] from comment #10)
>  '\u00a3\u00a5\u00b5\u5317\ud841\udf79'

Updated the test to include your suggested testcase - thanks - carrying over r+!

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8800995 [details] [diff] [review]
bug_1307321_csp_report_json.patch

Freddy, wanna take a look at the C++ bits - thanks!

Comment 15

[Frederik Braun [:freddy]]

Comment on attachment 8800995 [details] [diff] [review]
bug_1307321_csp_report_json.patch

Review of attachment 8800995 [details] [diff] [review]:
-----------------------------------------------------------------

The test seems incomplete to me.

::: dom/security/test/unit/test_csp_reports.js
@@ +136,5 @@
>            csp.logViolationDetails(Ci.nsIContentSecurityPolicy.VIOLATION_TYPE_EVAL,
>                                    selfuri.asciiSpec,
> +                                  // sending UTF-16 script sample to make sure
> +                                  // csp report in JSON is not cut-off
> +                                  "\u00a3\u00a5\u00b5\u5317\ud841\udf79",

Shouldn't we also test that these bytes (and a correct length) arrive at the other side?

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307321_csp_report_json.patch

As discussed over IRC, there is no need to convert back and forth between UTF16 and UTF8 for the expectedJSON, hence hardcoing the expected UTF8 script-sample.

Comment 17

[Frederik Braun [:freddy]]

Comment on attachment 8801091 [details] [diff] [review]
bug_1307321_csp_report_json.patch

Review of attachment 8801091 [details] [diff] [review]:
-----------------------------------------------------------------

yep, thanks!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f443b21ba9de
Use correct length of CSP report when sending violations. r=jrgm,freddyb

Comment 19

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

Backed out for unexpected passing of scripthash-unicode-normalization.sub.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7107572c7119631ab31161b3d5e7a88f804723d2

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f443b21ba9de2a2acf18b1b07083dfa6fda4fd26
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=37661894&repo=mozilla-inbound

[task 2016-10-14T15:32:40.765608Z] 15:32:40     INFO - TEST-START | /content-security-policy/blink-contrib-2/scripthash-unicode-normalization.sub.html
[task 2016-10-14T15:32:41.635317Z] 15:32:41     INFO - 
[task 2016-10-14T15:32:41.635743Z] 15:32:41     INFO - TEST-PASS | /content-security-policy/blink-contrib-2/scripthash-unicode-normalization.sub.html | Only matching content runs even with NFC normalization. 
[task 2016-10-14T15:32:41.635857Z] 15:32:41     INFO - TEST-UNEXPECTED-PASS | /content-security-policy/blink-contrib-2/scripthash-unicode-normalization.sub.html | Violation report status OK. - expected FAIL

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307321_update_webplatform.patch

Thanks Aryx and sorry for the bustage - at least we know the patch is correct now :-)

Freddy, agreed?

Comment 21

[Frederik Braun [:freddy]]

Comment on attachment 8801152 [details] [diff] [review]
bug_1307321_update_webplatform.patch

Review of attachment 8801152 [details] [diff] [review]:
-----------------------------------------------------------------

Heh, that's good news! :)

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19805d092001
Use correct length of CSP report when sending violations. r=jrgm,freddyb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0687e5322db
Update web platform tests ini files because unicode normalization works now. r=freddyb

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19805d092001
https://hg.mozilla.org/mozilla-central/rev/a0687e5322db