Avoid crashing the browser on truncated reads from the cache backend

net/http/http_cache_transaction.cc

Index: net/http/http_cache_transaction.cc
diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index 07fa4dbacc4dbb68763c70cfde3c639a809e6677..57a309e7ba5845358620d08b8af5026f36abbe1d 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -1385,6 +1385,17 @@ int HttpCache::Transaction::DoCacheReadResponse() {
   next_state_ = STATE_CACHE_READ_RESPONSE_COMPLETE;
 
   io_buf_len_ = entry_->disk_entry->GetDataSize(kResponseInfoIndex);
+  if (io_buf_len_ > 0) {

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

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.

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

If the returned value is zero we create an IOBuffer on line 1399 which would
immediately crash.
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]

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).
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]

I was wrong when assuming 0 size would fail, sorry. In fact it failed only with
negative size.
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:07]

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'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).

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

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]

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.
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.

+    UMA_HISTOGRAM_BOOLEAN("HttpCache.TruncatedHeader", false);
+  } else {
+    UMA_HISTOGRAM_BOOLEAN("HttpCache.TruncatedHeader", true);
+    DLOG(WARNING) << "Truncated cache entry header encountered";
+    mode_ = NONE;
+    if (partial_.get())
+      partial_->RestoreHeaders(&custom_request_->extra_headers);
+    next_state_ = STATE_SEND_REQUEST;
+    return OK;
+  }
   read_buf_ = new IOBuffer(io_buf_len_);
 
   net_log_.BeginEvent(NetLog::TYPE_HTTP_CACHE_READ_INFO);
@@ -1482,8 +1493,19 @@ int HttpCache::Transaction::DoCacheReadMetadata() {
   DCHECK(!response_.metadata);
   next_state_ = STATE_CACHE_READ_METADATA_COMPLETE;
 
-  response_.metadata =
-      new IOBufferWithSize(entry_->disk_entry->GetDataSize(kMetadataIndex));
+  int32 data_size = entry_->disk_entry->GetDataSize(kMetadataIndex);
+  if (data_size > 0) {

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

same here

+    UMA_HISTOGRAM_BOOLEAN("HttpCache.TruncatedMetadata", false);
+  } else {
+    UMA_HISTOGRAM_BOOLEAN("HttpCache.TruncatedMetadata", true);
+    DLOG(WARNING) << "Truncated cache entry metadata encountered";
+    mode_ = NONE;
+    if (partial_.get())
+      partial_->RestoreHeaders(&custom_request_->extra_headers);
+    next_state_ = STATE_SEND_REQUEST;
+    return OK;
+  }
+  response_.metadata = new IOBufferWithSize(data_size);
 
   net_log_.BeginEvent(NetLog::TYPE_HTTP_CACHE_READ_INFO);
   ReportCacheActionStart();