Avoid crashing the browser on truncated reads from the cache backend

Avoid crashing the browser on truncated reads from the cache backend

BUG=236384
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197164

[pasko-google - do not use, 2013-04-29 16:53:04]

Adding Jim as a reviewer to histograms.xml

[gavinp, 2013-04-29 16:53:49]

lgtm.

You'll need an owners review from jar as well.

[gavinp, 2013-04-29 16:54:30]

https://codereview.chromium.org/14533007/diff/1/tools/metrics/histograms/hist...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/14533007/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:823: <histogram
name="HttpCache.HeaderTruncated" units="BooleanTruncated">
Would "TruncatedHeader" and "TruncatedMetadata" be better names? They would then
collate next to each other in the dashboard.

[pasko-google - do not use, 2013-04-29 17:31:40]

https://codereview.chromium.org/14533007/diff/1/tools/metrics/histograms/hist...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/14533007/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:823: <histogram
name="HttpCache.HeaderTruncated" units="BooleanTruncated">
On 2013/04/29 16:54:30, gavinp wrote:
> Would "TruncatedHeader" and "TruncatedMetadata" be better names? They would
then
> collate next to each other in the dashboard.

Done.

[jar (doing other things), 2013-04-29 17:33:16]

Patch set 2 histograms.xml LGTM

[commit-bot: I haz the power, 2013-04-29 17:36:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pasko@google.com/14533007/9001

[commit-bot: I haz the power, 2013-04-29 22:31:34]

Change committed as 197164

[rvargas (doing something else), 2013-04-30 18:06:25]

https://codereview.chromium.org/14533007/diff/9001/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/14533007/diff/9001/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:1388: if (io_buf_len_ > 0) {
This doesn't look good to me. The disk cache is never supposed to return a
negative value, and if the value is zero, the old code should handle that
without an issue (not the most efficient code, but it should be handled).

In other words, we should not masks bugs (of negative return values) at this
layer. If we are debugging that case, adding a CHECK here would be more
appropriate than adding histograms.

https://codereview.chromium.org/14533007/diff/9001/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:1497: if (data_size > 0) {
same here

[pasko-google - do not use, 2013-04-30 19:12:38]

https://codereview.chromium.org/14533007/diff/9001/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/14533007/diff/9001/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:1388: if (io_buf_len_ > 0) {
On 2013/04/30 18:06:25, rvargas wrote:
> This doesn't look good to me. The disk cache is never supposed to return a
> negative value, and if the value is zero, the old code should handle that
> without an issue (not the most efficient code, but it should be handled).

If the returned value is zero we create an IOBuffer on line 1399 which would
immediately crash.

> In other words, we should not masks bugs (of negative return values) at this
> layer. If we are debugging that case, adding a CHECK here would be more
> appropriate than adding histograms.

Indeed. Each backend should sanitize its outputs and never return a negative
value treating it as cache corruption. On the zero size I am not so sure. Do we
support HTTP/1.0? It seems to allow empty headers?

Here we are dealing with corrupted cache, potentially done by a malicious
program. This change avoids the browser from crashing, even on rare bugs in the
cache backend, like race conditions. Facing the sad overlook yesterday we made a
preference to put some defense on both sides. The downside of this is added
complexity, code size, minimal overhead. In the exchange we get a feeling of
false security :) Like something we get with .. sandboxes.

[rvargas (doing something else), 2013-04-30 21:09:05]

https://codereview.chromium.org/14533007/diff/9001/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/14533007/diff/9001/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:1388: if (io_buf_len_ > 0) {
On 2013/04/30 19:12:38, pasko wrote:
> On 2013/04/30 18:06:25, rvargas wrote:
> > This doesn't look good to me. The disk cache is never supposed to return a
> > negative value, and if the value is zero, the old code should handle that
> > without an issue (not the most efficient code, but it should be handled).
> 
> If the returned value is zero we create an IOBuffer on line 1399 which would
> immediately crash.

I'm sorry but I don't see where we would crash if the size is zero. Creating a
buffer with 0 bytes is valid, and Reading 0 bytes is also valid (and returns 0
bytes read).

> 
> > In other words, we should not masks bugs (of negative return values) at this
> > layer. If we are debugging that case, adding a CHECK here would be more
> > appropriate than adding histograms.
> 
> Indeed. Each backend should sanitize its outputs and never return a negative
> value treating it as cache corruption. On the zero size I am not so sure. Do
we
> support HTTP/1.0? It seems to allow empty headers?
> 
> Here we are dealing with corrupted cache, potentially done by a malicious
> program. This change avoids the browser from crashing, even on rare bugs in
the
> cache backend, like race conditions. Facing the sad overlook yesterday we made
a
> preference to put some defense on both sides. The downside of this is added
> complexity, code size, minimal overhead. In the exchange we get a feeling of
> false security :) Like something we get with .. sandboxes.

Nope. Bugs that violate contracts should be fixed, and usually the fastest way
to fix a bug is if we crash each time we see that. So I would support a DCHECK,
or a CHECK if we are trying to find violations from the field (ideally we do
that by other means, before releasing code to users).

As I said, 0 bytes is a valid return value and it should be handled (and afaik
we do it). Negative values are breaking the contract and it is the
responsibility of the lower layer to handle that type of corruption.

[pasko-google - do not use, 2013-04-30 22:13:33]

https://codereview.chromium.org/14533007/diff/9001/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/14533007/diff/9001/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:1388: if (io_buf_len_ > 0) {
On 2013/04/30 21:09:05, rvargas wrote:
> > If the returned value is zero we create an IOBuffer on line 1399 which would
> > immediately crash.
> 
> I'm sorry but I don't see where we would crash if the size is zero. Creating a
> buffer with 0 bytes is valid, and Reading 0 bytes is also valid (and returns 0
> bytes read).

I was wrong when assuming 0 size would fail, sorry. In fact it failed only with
negative size.

> > Here we are dealing with corrupted cache, potentially done by a malicious
> > program. This change avoids the browser from crashing, even on rare bugs in
> the
> > cache backend, like race conditions. Facing the sad overlook yesterday we
made
> a
> > preference to put some defense on both sides. The downside of this is added
> > complexity, code size, minimal overhead. In the exchange we get a feeling of
> > false security :) Like something we get with .. sandboxes.
> 
> Nope. Bugs that violate contracts should be fixed, and usually the fastest way
> to fix a bug is if we crash each time we see that. So I would support a
DCHECK,
> or a CHECK if we are trying to find violations from the field (ideally we do
> that by other means, before releasing code to users).

Should we also disable the chromium sandbox? It would be fun to blame Blink for
all contract violations.
</sarcasm>

I generally agree that double-checking the contract is not a good style. Do we
have an extensive test to verify this specific part of the contract? Death tests
are probably the best we have? Unittests did not look very extensive regarding
cache corruptions, but I did not look through all of them yet.

P.S.: please don't worry if I do not reply today/tomorrow, it would be a public
holiday, I'll return to this conversation as normal on Thursday.

[rvargas (doing something else), 2013-04-30 22:50:06]

https://codereview.chromium.org/14533007/diff/9001/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/14533007/diff/9001/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:1388: if (io_buf_len_ > 0) {
On 2013/04/30 22:13:33, pasko wrote:
> On 2013/04/30 21:09:05, rvargas wrote:
> > > If the returned value is zero we create an IOBuffer on line 1399 which
would
> > > immediately crash.
> > 
> > I'm sorry but I don't see where we would crash if the size is zero. Creating
a
> > buffer with 0 bytes is valid, and Reading 0 bytes is also valid (and returns
0
> > bytes read).
> 
> I was wrong when assuming 0 size would fail, sorry. In fact it failed only
with
> negative size.
> 
> > > Here we are dealing with corrupted cache, potentially done by a malicious
> > > program. This change avoids the browser from crashing, even on rare bugs
in
> > the
> > > cache backend, like race conditions. Facing the sad overlook yesterday we
> made
> > a
> > > preference to put some defense on both sides. The downside of this is
added
> > > complexity, code size, minimal overhead. In the exchange we get a feeling
of
> > > false security :) Like something we get with .. sandboxes.
> > 
> > Nope. Bugs that violate contracts should be fixed, and usually the fastest
way
> > to fix a bug is if we crash each time we see that. So I would support a
> DCHECK,
> > or a CHECK if we are trying to find violations from the field (ideally we do
> > that by other means, before releasing code to users).
> 
> Should we also disable the chromium sandbox? It would be fun to blame Blink
for
> all contract violations.
> </sarcasm>

We don't need to disable the sandbox... we crash right away when the renderer
does something that is not supposed to do. :p

> 
> I generally agree that double-checking the contract is not a good style. Do we
> have an extensive test to verify this specific part of the contract? Death
tests
> are probably the best we have? Unittests did not look very extensive regarding
> cache corruptions, but I did not look through all of them yet.

I'm not sure I know what you mean by death tests. There is extensive testing for
corruption, but most tests are quite specific to the corruption that could
happen for a type of backend (in other words, they only cover the current
implementation). I'm sure you understand that predicting what a new backend is
going to use is a little tricky :)

There are of course tests that verify that GetDataSize() returns the correct
value, but again, it is hard to predict what real world conditions would trigger
a given implementation to return the wrong value. That should mostly come from
code inspection focused on what can go wrong (and in general, anything coming
from disk can go wrong... so a cache should never blindly trust what it sees).

> 
> P.S.: please don't worry if I do not reply today/tomorrow, it would be a
public
> holiday, I'll return to this conversation as normal on Thursday.
>

[pasko-google - do not use, 2013-05-02 16:56:37]

https://codereview.chromium.org/14533007/diff/9001/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/14533007/diff/9001/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:1388: if (io_buf_len_ > 0) {
On 2013/04/30 22:50:07, rvargas wrote:
> > Should we also disable the chromium sandbox? It would be fun to blame Blink
> for
> > all contract violations.
> > </sarcasm>
> 
> We don't need to disable the sandbox... we crash right away when the renderer
> does something that is not supposed to do. :p

We have the sandbox because the renderer, if tricked, can do really nasty things
without a crash or prior to crashing. And there were too many things to fix
there, instead we reduced the attack surface by placing the renderer process
into the sandbox and putting extra guards around talking to the renderer.

Luckily disk cache backends are less nasty, but they too are not free from
occasional human error, and are somewhat complex, multithreaded components. I do
_not_ suggest placing the cache into the sandbox right now, though it's a viable
thing to do when sandbox supports chroot which seccomp-bpf does.

On the other hand, what is wrong with being on a slightly safer side with almost
no cost?

I uploaded a change to revert this CL, if after this much discussion you still
think it is not a proper layer here to put safety checks, lgtm that one:
https://codereview.chromium.org/14740016

[rvargas (doing something else), 2013-05-02 17:52:31]

https://codereview.chromium.org/14533007/diff/9001/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/14533007/diff/9001/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:1388: if (io_buf_len_ > 0) {
On 2013/05/02 16:56:37, pasko wrote:
> On 2013/04/30 22:50:07, rvargas wrote:
> > > Should we also disable the chromium sandbox? It would be fun to blame
Blink
> > for
> > > all contract violations.
> > > </sarcasm>
> > 
> > We don't need to disable the sandbox... we crash right away when the
renderer
> > does something that is not supposed to do. :p
> 
> We have the sandbox because the renderer, if tricked, can do really nasty
things
> without a crash or prior to crashing. And there were too many things to fix
> there, instead we reduced the attack surface by placing the renderer process
> into the sandbox and putting extra guards around talking to the renderer.

And that is why the sandbox has nothing to do with this code (I mean, it was not
a very good example to pick). The sandbox is a security feature; it has nothing
to do with enforcing any point of view on code correctness or API contracts.

> 
> Luckily disk cache backends are less nasty, but they too are not free from
> occasional human error, and are somewhat complex, multithreaded components. I
do
> _not_ suggest placing the cache into the sandbox right now, though it's a
viable
> thing to do when sandbox supports chroot which seccomp-bpf does.
> 
> On the other hand, what is wrong with being on a slightly safer side with
almost
> no cost?

It is wrong to mask bugs by _handling_ cases that an API forbids, when both
sides of that API are fully implemented by code under our control. That muddy
the waters about whose responsibility is to do something, and that is not a good
thing to have in the long term.

> 
> I uploaded a change to revert this CL, if after this much discussion you still
> think it is not a proper layer here to put safety checks, lgtm that one:
> https://codereview.chromium.org/14740016