JsonParser measures stack depth during the parsing: https://cs.chromium.org/chromium/src/base/json/json_parser.cc?q=StackMarker&sq=package:chromium&l=171

Alternatively we can pass a depth counter to ValueResultFromWKResult and bail if it reaches a certain number.

We should at the very least have unit test coverage that includes some stack-smashing and other pathological cases.

We have a test file for ValueResultFromWKResult function, where we could write these tests: web_view_js_utils_unittest.mm

Why do we convert the returned dictionary to base::Value in the first place?

We use base::Value to match Chromium on other platforms: https://cs.chromium.org/chromium/src/content/public/renderer/render_frame.h?q=ExecuteJavaScript+file:content&sq=package:chromium&l=142&dr=CSs

Long time ago it was decided that ios/web should be as close as possible to content in terms of API, so people who familiar with content can easily use ios/web. We are not nearly close to it, but it's the ultimate goal.

Ok, thanks for the context.

One small note: even with the depth check, a malicious webpage could crash the Bling process (though this time, not with a stack overflow).

Consider the dictionary returned by this JS code sample: 
oneMegaByteString = ".........."; // a 1 MB string
return {'key1' : oneMegaByteString, 'key2' : oneMegaByteString, 'key3' : oneMegaByteString, ... etc ...  };

For this dictionary, ValueResultFromWKResult would instantiate multiple instances of the oneMegaByteString, which I believe would result in our process crashing/being killed.


Is that a problem? Do we try to fight webpages that try to fill Chrome's memory?

That's an interesting observation. Is evaluateJavaScript:completionHandler: smart enough to share the same string value if it backed up by the same string in JavaScript?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8c6bd84b9151d53c938205ad7809cd7784980403

commit 8c6bd84b9151d53c938205ad7809cd7784980403
Author: jif <jif@chromium.org>
Date: Tue Sep 13 17:50:46 2016

Limit depth of parsing of dictionaries returned by JS evaluation.

BUG= 645486 

Review-Url: https://codereview.chromium.org/2335483004
Cr-Commit-Position: refs/heads/master@{#418289}

[modify] https://crrev.com/8c6bd84b9151d53c938205ad7809cd7784980403/ios/web/web_state/ui/web_view_js_utils.h
[modify] https://crrev.com/8c6bd84b9151d53c938205ad7809cd7784980403/ios/web/web_state/ui/web_view_js_utils.mm
[modify] https://crrev.com/8c6bd84b9151d53c938205ad7809cd7784980403/ios/web/web_state/ui/web_view_js_utils_unittest.mm

8> Yes, |evaluateJavaScript:completionHandler:| is smart enough to share the string value.

The following javascript:

var foo = "lorem ipsum dolor";
var res = {"a" : foo, "b" : foo, "c" : "lorem ipsum dolor"}
res;

Is converted into an NSDictionary where all the entries point to the same __NSCFString.

We can't cleanly do this sort of conversion with base::DictionaryValue because it only takes values as unique_ptr.

Do we want to address this a problem? If yes, then just like we are checking if we are not recursing too much, we could check that we are not allocating too much memory.

>>> Do we want to address this a problem?
Thanks for checking that! So in theory the web page can force Chrome to allocated large number of strings and cause OOM crash. I don't see any potential security issues however (unlike stack smashing, which may be actually a security vulnerability). jif@ can you try constructing a script to cause OOM? If making such script is impossible, then I don't think we should care about fixing this. Mark, do you have any thoughts on this?

I didn't find any feature that uses |ExecuteJavaScript|, so I wasn't able to create a test page that attempted to crash Chrome.

Reading list uses it, so Elodie can point you to directions. 

jif@, I think you fixed this already, please reopen if you think we need to address an issues with duplicated keys.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot