Allow data URL > 2MB for loadDataWithBaseURL

Allow payload data > 2MB for loadDataWithBaseURL

This is needed for WebView Classic backwards compatibility -
unforks KitKat release branch workaround
https://android.googlesource.com/platform/external/chromium_org/+/63d0ac2aed^!/

Chrome has a limit of 2MB for any URL passed over IPC, to deal with this
payload data in loadDataWithBaseURL is sent as a raw buffer. (This also more
closely matches legacy semantics; e.g. no base64 encode needed).

This does not fix the issue for WebView.loadData() or arbitrary
loadUrl("data:...") navigations. I'll think about that as a followup.

At the same time, upstream the LoadUrlParams creation wrapper methods
from the glue layer into AwContents. This simplifies the underlying classes,
makes the LoadDataWithBaseUrlTest more meaningful, and is needed to avoid the
unnecessary base64 encoding of the data payload.


BUG=298495

[mkosiba (inactive), 2013-11-26 10:07:42]

LGTM

[boliu, 2013-11-26 17:15:15]

https://codereview.chromium.org/84703003/diff/160001/android_webview/native/s...
File android_webview/native/state_serializer.cc (right):

https://codereview.chromium.org/84703003/diff/160001/android_webview/native/s...
android_webview/native/state_serializer.cc:264: if
(!iterator->ReadString(&res->data()))
Not pickle expert, but does can ReadString be matched with WriteData?

https://codereview.chromium.org/84703003/diff/160001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

https://codereview.chromium.org/84703003/diff/160001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:294:
return url.regionMatches(true, start, "data:", 0, 5);
Scary code. Can java.net.URL do this parsing?

[joth, 2013-11-27 01:31:21]

https://codereview.chromium.org/84703003/diff/160001/android_webview/native/s...
File android_webview/native/state_serializer.cc (right):

https://codereview.chromium.org/84703003/diff/160001/android_webview/native/s...
android_webview/native/state_serializer.cc:264: if
(!iterator->ReadString(&res->data()))
On 2013/11/26 17:15:15, boliu wrote:
> Not pickle expert, but does can ReadString be matched with WriteData?

The internal implementation makes them compatible, but I agree it's a bad smell
to depend on this. Will use ReadData like I do in the IPC traits.

Done

https://codereview.chromium.org/84703003/diff/160001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

https://codereview.chromium.org/84703003/diff/160001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:294:
return url.regionMatches(true, start, "data:", 0, 5);
On 2013/11/26 17:15:15, boliu wrote:
> Scary code. Can java.net.URL do this parsing?

The snag is that data:... URLs can be super long, so parsing the entire beast
(let alone copying it over JNI like before) is a waste of effort.
I know I was against using anything other than GURL for parsing before... but
having submitted changes in GURL and being asked to do exactly this sort of fast
pathing there, I feel more comfortable in it now :)

I shoved a generalized version in GURLUtils and added a unit test. wdyt?

[joth, 2013-11-27 01:34:21]

+thakis for base/memory

[boliu, 2013-11-27 01:39:53]

lgtm

[joth, 2013-11-27 01:49:14]

+ppi for net/

[Nico, 2013-11-27 01:49:49]

https://codereview.chromium.org/84703003/diff/170001/base/memory/ref_counted_...
File base/memory/ref_counted_memory.h (right):

https://codereview.chromium.org/84703003/diff/170001/base/memory/ref_counted_...
base/memory/ref_counted_memory.h:26: static base::StringPiece AsString(const
RefCountedMemory* memory);
Huh, why is this a static method on RefCountedMemory that then takes an argument
of RefCountedMemory? Just make it a regular member function?

Since it can be implemented in terms of the public methods of this class, maybe
it could be a function in the place where you need it instead?

(But since it requires a reinterpret_cast I think I'm fine with hiding that line
in base)

[joth, 2013-11-27 01:53:46]

Thanks!


On 26 November 2013 17:49, <thakis@chromium.org> wrote:

>
> https://codereview.chromium.org/84703003/diff/170001/base/
> memory/ref_counted_memory.h
> File base/memory/ref_counted_memory.h (right):
>
> https://codereview.chromium.org/84703003/diff/170001/base/
> memory/ref_counted_memory.h#newcode26
> base/memory/ref_counted_memory.h:26: static base::StringPiece
> AsString(const RefCountedMemory* memory);
> Huh, why is this a static method on RefCountedMemory that then takes an
> argument of RefCountedMemory? Just make it a regular member function?
>
> yeah... it turns out the three call sites where I need this all need to
handle the case of their RefCountedMemory* being NULL too. Once I'm
duplicating that in three places I might as well put the reinterpret case
there too....



> Since it can be implemented in terms of the public methods of this
> class, maybe it could be a function in the place where you need it
> instead?
>
> (But since it requires a reinterpret_cast I think I'm fine with hiding
> that line in base)
>
> https://codereview.chromium.org/84703003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2013-11-27 02:01:53]

On Wed, Nov 27, 2013 at 10:53 AM, Jonathan Dixon <joth@chromium.org> wrote:

> Thanks!
>
>
> On 26 November 2013 17:49, <thakis@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/84703003/diff/170001/base/
>> memory/ref_counted_memory.h
>> File base/memory/ref_counted_memory.h (right):
>>
>> https://codereview.chromium.org/84703003/diff/170001/base/
>> memory/ref_counted_memory.h#newcode26
>> base/memory/ref_counted_memory.h:26: static base::StringPiece
>> AsString(const RefCountedMemory* memory);
>> Huh, why is this a static method on RefCountedMemory that then takes an
>> argument of RefCountedMemory? Just make it a regular member function?
>>
>> yeah... it turns out the three call sites where I need this all need to
> handle the case of their RefCountedMemory* being NULL too. Once I'm
> duplicating that in three places I might as well put the reinterpret case
> there too....
>

Which of these do you find easier to read:

  StringPiece string_data = RefCountedMemory::AsString(memory);

or

  StringPiece string_data;
  if (memory)
    string_data = memory->AsString();

?

I find the second version easier to understand, and it makes it possible to
write a version without the null check in cases where one knowns that one's
RCM isn't ever NULL.


>
>
>
>> Since it can be implemented in terms of the public methods of this
>> class, maybe it could be a function in the place where you need it
>> instead?
>>
>> (But since it requires a reinterpret_cast I think I'm fine with hiding
>> that line in base)
>>
>> https://codereview.chromium.org/84703003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[joth, 2013-11-27 04:34:40]

Thanks Thankis.
With the NULL check at caller, it is just as clear to drop the StringPiece
shenanigans altogether (I was only ever using it as an intermediate format to
get to some other type anyway).
So no base/ review needed now.

[joth, 2013-11-27 04:38:39]

+jochen for content (non-androidy bits)
+jln for content/common/view_messages.h

[joth, 2013-11-27 05:46:23]

-ppi as I removed the net/ edits too. (And ppi is not a net/android owner anyway
:)

Sorry for the noise!

https://codereview.chromium.org/84703003/diff/160001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

https://codereview.chromium.org/84703003/diff/160001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:294:
return url.regionMatches(true, start, "data:", 0, 5);
On 2013/11/27 01:31:22, joth wrote:
> On 2013/11/26 17:15:15, boliu wrote:
> > Scary code. Can java.net.URL do this parsing?
> 
> The snag is that data:... URLs can be super long, so parsing the entire beast
> (let alone copying it over JNI like before) is a waste of effort.
> I know I was against using anything other than GURL for parsing before... but
> having submitted changes in GURL and being asked to do exactly this sort of
fast
> pathing there, I feel more comfortable in it now :)
> 
> I shoved a generalized version in GURLUtils and added a unit test. wdyt?

OK I didn't like it in GURLUtils as it's counter to the name of that class to
not be using GURL for this logic.

turns out that android.net.Uri is almost good for this: it lazily parses the
input. Only snag is it doesn't skip leading whitespace or do case insensetive
compare, both of which my two-liner here handled.

anyway.... after a bundle more tail chasing I realized this is all pointless....
See loadDataWithBaseURL in
https://android.googlesource.com/platform/frameworks/base/+/jb-mr2-release/co...
 - if the baseUrl scheme is data: it just calls loadData() ignoring the base
URL. WebViewChromium *almost* has the same logic... if I just fix that, then
special case data: base URLs in this class can just be deleted.

[Nico, 2013-11-27 05:50:59]

https://codereview.chromium.org/84703003/diff/230001/android_webview/native/s...
File android_webview/native/state_serializer.cc (right):

https://codereview.chromium.org/84703003/diff/230001/android_webview/native/s...
android_webview/native/state_serializer.cc:188: if (!pickle->WriteData(data,
size))
Do you need to do this call when size is 0? If not, can this be in the if? If
so, is it worth to have local variables for size (maybe not) and data (maybe
yes, not sure)?

[joth, 2013-11-27 06:23:15]

https://codereview.chromium.org/84703003/diff/230001/android_webview/native/s...
File android_webview/native/state_serializer.cc (right):

https://codereview.chromium.org/84703003/diff/230001/android_webview/native/s...
android_webview/native/state_serializer.cc:188: if (!pickle->WriteData(data,
size))
On 2013/11/27 05:50:59, Nico wrote:
> Do you need to do this call when size is 0? If not, can this be in the if? If
> so, is it worth to have local variables for size (maybe not) and data (maybe
> yes, not sure)?

Yes, We need to write something even when the object is NULL or holds zero
bytes, so that when internalizing again we know whether or not to try and read
the data payload back in again. I'm basically using the WriteData(NULL, 0) as
the sentinel so we know we don't need to initialize that member (line 274)

[Nico, 2013-11-27 06:41:42]

On Wed, Nov 27, 2013 at 3:23 PM, <joth@chromium.org> wrote:

>
> https://codereview.chromium.org/84703003/diff/230001/
> android_webview/native/state_serializer.cc
> File android_webview/native/state_serializer.cc (right):
>
> https://codereview.chromium.org/84703003/diff/230001/
> android_webview/native/state_serializer.cc#newcode188
> android_webview/native/state_serializer.cc:188: if
> (!pickle->WriteData(data, size))
> On 2013/11/27 05:50:59, Nico wrote:
>
>> Do you need to do this call when size is 0? If not, can this be in the
>>
> if? If
>
>> so, is it worth to have local variables for size (maybe not) and data
>>
> (maybe
>
>> yes, not sure)?
>>
>
> Yes, We need to write something even when the object is NULL or holds
> zero bytes, so that when internalizing again we know whether or not to
> try and read the data payload back in again. I'm basically using the
> WriteData(NULL, 0) as the sentinel so we know we don't need to
> initialize that member (line 274)
>

(might be worth a comment as it was not obvious to me)


>
> https://codereview.chromium.org/84703003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[joth, 2013-11-27 07:34:10]

On 2013/11/27 06:41:42, Nico wrote:
> On Wed, Nov 27, 2013 at 3:23 PM, <mailto:joth@chromium.org> wrote:
> > Yes, We need to write something even when the object is NULL or holds
> > zero bytes, so that when internalizing again we know whether or not to
> > try and read the data payload back in again. I'm basically using the
> > WriteData(NULL, 0) as the sentinel so we know we don't need to
> > initialize that member (line 274)
> >
> 
> (might be worth a comment as it was not obvious to me)
> 
> 
Done.

[jochen (gone - plz use gerrit), 2013-11-27 08:41:04]

could you add a content browsertest that creating a navigation entry with some
data and navigating to it results into the data being loaded correctly?

[jochen (gone - plz use gerrit), 2013-11-27 08:48:20]

Also, please note in the CL description that the 2MB limit comes from the max
length of URLs allowed in IPCs.

[Ben Murdoch, 2013-11-27 17:00:41]

https://codereview.chromium.org/84703003/diff/250001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

https://codereview.chromium.org/84703003/diff/250001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:119:
assert !baseUrl.startsWith("data:");
toLowerCase?

[joth, 2013-11-27 21:03:06]

Thanks jochen. Should be ready for another look.


On 2013/11/27 08:41:04, jochen wrote:
> could you add a content browsertest that creating a navigation entry with some
> data and navigating to it results into the data being loaded correctly?

Done - added a render_view_browsertest.cc test for load data with Base URL, and
also expanded navigation_controller_impl_unittest.cc to cover the new struct
member.


On 2013/11/27 08:48:20, jochen wrote:
> Also, please note in the CL description that the 2MB limit comes from the max
> length of URLs allowed in IPCs.

Done.

[joth, 2013-11-27 21:05:26]

https://codereview.chromium.org/84703003/diff/250001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

https://codereview.chromium.org/84703003/diff/250001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:119:
assert !baseUrl.startsWith("data:");
On 2013/11/27 17:00:42, Ben Murdoch wrote:
> toLowerCase?

yeah... it gets into a maze - should we trim leading whitespace from the input
too? etc..... this is really just a sanity check for developers and I've never
seen anyone use uppercase chars in any scheme, let alone data: so I'm inclined
to just leave this as a cheap but good-enough safety net....

[joth, 2013-11-28 16:50:21]

Jochen, jln: PTAL


On Wednesday, 27 November 2013, wrote:

>
> https://codereview.chromium.org/84703003/diff/250001/
> content/public/android/java/src/org/chromium/content/
> browser/LoadUrlParams.java
> File
> content/public/android/java/src/org/chromium/content/
> browser/LoadUrlParams.java
> (right):
>
> https://codereview.chromium.org/84703003/diff/250001/
> content/public/android/java/src/org/chromium/content/
> browser/LoadUrlParams.java#newcode119
> content/public/android/java/src/org/chromium/content/
> browser/LoadUrlParams.java:119:
> assert !baseUrl.startsWith("data:");
> On 2013/11/27 17:00:42, Ben Murdoch wrote:
>
>> toLowerCase?
>>
>
> yeah... it gets into a maze - should we trim leading whitespace from the
> input too? etc..... this is really just a sanity check for developers
> and I've never seen anyone use uppercase chars in any scheme, let alone
> data: so I'm inclined to just leave this as a cheap but good-enough
> safety net....
>
> https://codereview.chromium.org/84703003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jln (very slow on Chromium), 2013-12-02 23:05:24]

Adding tsepez@ and palmer@ for advice.

Do you know why URL data payloads are limited to 2MB? Is there any chance this
could be adapted to fulfill your needs instead?

We should make sure we fully understand the reason before adding a bypass
mechanism.

In any case, there needs to be a "reasonable" size limitation. We should not add
more IPCs that allow an attacker to write large amount of arbitrary contiguous
memory.

https://chromiumcodereview.appspot.com/84703003/diff/290001/content/common/co...
File content/common/content_param_traits.cc (right):

https://chromiumcodereview.appspot.com/84703003/diff/290001/content/common/co...
content/common/content_param_traits.cc:36: res->data().assign(data, len);
This clearly needs a reasonable limit on len.

This would allow an attacker to trivially spray the address space of another
process.

[Tom Sepez, 2013-12-02 23:15:49]

On 2013/12/02 23:05:24, jln wrote:
> Adding tsepez@ and palmer@ for advice.
> 
> Do you know why URL data payloads are limited to 2MB?
As with most of these things, its to protect the browser from getting abused by
the renderer.

What size do we need to actually support?  What direction does the data pass
(browser -> renderer)?

[joth, 2013-12-02 23:21:38]

Regarding bumping up the limit, we don't have much numbers beyond knowing that
2MB was not enough.

In quick-fix workaround (applied for KK release only) was bumping the constant
up to 20MB which solved the reported problems (see b/10742235), so if we could
put it that high we know it would be OK.
In my own testing since then, passing around 10MB data URLs was borderline
DoSing the processes due to the overhead of all the copying and reparsing
involved. So perhaps 10MB would do   (especially if we could set this for
webview only)

https://chromiumcodereview.appspot.com/84703003/diff/290001/android_webview/j...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://chromiumcodereview.appspot.com/84703003/diff/290001/android_webview/j...
android_webview/java/src/org/chromium/android_webview/AwContents.java:22: import
android.util.Base64;
not needed

https://chromiumcodereview.appspot.com/84703003/diff/290001/content/common/co...
File content/common/content_param_traits.cc (right):

https://chromiumcodereview.appspot.com/84703003/diff/290001/content/common/co...
content/common/content_param_traits.cc:36: res->data().assign(data, len);
On 2013/12/02 23:05:25, jln wrote:
> This clearly needs a reasonable limit on len.
> 
> This would allow an attacker to trivially spray the address space of another
> process.

Good point.

The required use-case is only for sending the data payload from browser ->
renderer. unfortunately by the time it gets to this layer, that direction
information is lost

Further, we only need this support for android webview, which runs in single
process mode so this level of memory protection between renderer and browser is
moot. Again, by the time we get down to this layer that point is also moot.

I'll have to have a think if there's a sensible way to reduce the scope of this
patch to the interesting scenarios then.

[Tom Sepez, 2013-12-02 23:35:11]

> I'll have to have a think if there's a sensible way to reduce the scope of
this
> patch to the interesting scenarios then.
Thanks.  I'd also be happier if we didn't have to introduce traits for
base::RefCountedMemory.
This would apply to too many classes via promotion in the compiler.

[joth, 2013-12-03 00:17:30]

On 2013/12/02 23:35:11, Tom Sepez wrote:
> > I'll have to have a think if there's a sensible way to reduce the scope of
> this
> > patch to the interesting scenarios then.
> Thanks.  I'd also be happier if we didn't have to introduce traits for
> base::RefCountedMemory.
> This would apply to too many classes via promotion in the compiler.

Did you have any opinion on us switching the limit to 10MB (even if only under a
flag for webview) ?
Thanks

[Tom Sepez, 2013-12-03 00:19:49]

> Did you have any opinion on us switching the limit to 10MB (even if only under
a
> flag for webview) ?
> Thanks
Actually, I'm OK with that given that its only a 5x increase, and that the
original
2M was kind of arbitrary.  Can we get ifdef's specific to android (good) and/or
standalone webview (better)?

[joth, 2013-12-03 00:21:22]

OK I'll have a look at that. We only have ifdef for OS_ANDROID.  webview is
only selectable via runtime flag.



On 2 December 2013 16:19, <tsepez@chromium.org> wrote:

>  > Did you have any opinion on us switching the limit to 10MB (even if only
> under a
>
>> flag for webview) ?
>> Thanks
>>
> Actually, I'm OK with that given that its only a 5x increase, and that the
> original
> 2M was kind of arbitrary.  Can we get ifdef's specific to android (good)
> and/or
> standalone webview (better)?
>
> https://chromiumcodereview.appspot.com/84703003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tom Sepez, 2013-12-03 00:27:37]

On the other hand, history is not on our side.  I finally tracked this down. See

https://code.google.com/p/chromium/issues/detail?id=33952

[joth, 2013-12-03 00:38:48]

Ha! Oh yes, the limit was in fact 10MB prior to
https://codereview.chromium.org/1569011/diff/7001/chrome/common/chrome_consta...
Sucks to be me.




On 2 December 2013 16:27, <tsepez@chromium.org> wrote:

> On the other hand, history is not on our side.  I finally tracked this
> down. See
>
> https://code.google.com/p/chromium/issues/detail?id=33952
>
> https://codereview.chromium.org/84703003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jochen (gone - plz use gerrit), 2014-01-03 10:47:18]

i guess this CL is abandoned?

[joth, 2014-01-03 19:17:05]

On 2014/01/03 10:47:18, jochen wrote:
> i guess this CL is abandoned?

yes - it was done a different way in the end, in
https://codereview.chromium.org/112053002