From 28e663cbed28fb6c8c8dec0849e0277daf67651b Mon Sep 17 00:00:00 2001 From: Todd Kennedy Date: Thu, 12 Jul 2018 13:15:54 -0700 Subject: [PATCH] Loosen resource file verification Bug: 77808145 Test: Tried to install corrupt APK prior to the change, install failed Test: Tried to install corrupt APK after the change, install succeeded Test: atest CtsAppSecurityHostTestCases:CorruptApkTests Change-Id: I19a69e52a17c1080beaf2cc575c32f564b1033a3 --- libs/androidfw/ChunkIterator.cpp | 21 ++++++++++++++++++++- libs/androidfw/LoadedArsc.cpp | 12 +++++++++--- libs/androidfw/include/androidfw/Chunk.h | 9 +++++++++ 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/libs/androidfw/ChunkIterator.cpp b/libs/androidfw/ChunkIterator.cpp index d8adbe5ac85d5..8fc3219680559 100644 --- a/libs/androidfw/ChunkIterator.cpp +++ b/libs/androidfw/ChunkIterator.cpp @@ -32,11 +32,30 @@ Chunk ChunkIterator::Next() { if (len_ != 0) { // Prepare the next chunk. - VerifyNextChunk(); + if (VerifyNextChunkNonFatal()) { + VerifyNextChunk(); + } } return Chunk(this_chunk); } +// TODO(b/111401637) remove this and have full resource file verification +// Returns false if there was an error. +bool ChunkIterator::VerifyNextChunkNonFatal() { + if (len_ < sizeof(ResChunk_header)) { + last_error_ = "not enough space for header"; + last_error_was_fatal_ = false; + return false; + } + const size_t size = dtohl(next_chunk_->size); + if (size > len_) { + last_error_ = "chunk size is bigger than given data"; + last_error_was_fatal_ = false; + return false; + } + return true; +} + // Returns false if there was an error. bool ChunkIterator::VerifyNextChunk() { const uintptr_t header_start = reinterpret_cast(next_chunk_); diff --git a/libs/androidfw/LoadedArsc.cpp b/libs/androidfw/LoadedArsc.cpp index 04d506a2d71cc..c2740c9fbaa45 100644 --- a/libs/androidfw/LoadedArsc.cpp +++ b/libs/androidfw/LoadedArsc.cpp @@ -560,7 +560,9 @@ std::unique_ptr LoadedPackage::Load(const Chunk& chunk, if (iter.HadError()) { LOG(ERROR) << iter.GetLastError(); - return {}; + if (iter.HadFatalError()) { + return {}; + } } // Flatten and construct the TypeSpecs. @@ -641,7 +643,9 @@ bool LoadedArsc::LoadTable(const Chunk& chunk, const LoadedIdmap* loaded_idmap, if (iter.HadError()) { LOG(ERROR) << iter.GetLastError(); - return false; + if (iter.HadFatalError()) { + return false; + } } return true; } @@ -673,7 +677,9 @@ std::unique_ptr LoadedArsc::Load(const StringPiece& data, if (iter.HadError()) { LOG(ERROR) << iter.GetLastError(); - return {}; + if (iter.HadFatalError()) { + return {}; + } } // Need to force a move for mingw32. diff --git a/libs/androidfw/include/androidfw/Chunk.h b/libs/androidfw/include/androidfw/Chunk.h index 89b588e2b2e93..99a52dc9244ef 100644 --- a/libs/androidfw/include/androidfw/Chunk.h +++ b/libs/androidfw/include/androidfw/Chunk.h @@ -94,18 +94,27 @@ class ChunkIterator { Chunk Next(); inline bool HasNext() const { return !HadError() && len_ != 0; }; + // Returns whether there was an error and processing should stop inline bool HadError() const { return last_error_ != nullptr; } inline std::string GetLastError() const { return last_error_; } + // Returns whether there was an error and processing should stop. For legacy purposes, + // some errors are considered "non fatal". Fatal errors stop processing new chunks and + // throw away any chunks already processed. Non fatal errors also stop processing new + // chunks, but, will retain and use any valid chunks already processed. + inline bool HadFatalError() const { return HadError() && last_error_was_fatal_; } private: DISALLOW_COPY_AND_ASSIGN(ChunkIterator); // Returns false if there was an error. bool VerifyNextChunk(); + // Returns false if there was an error. For legacy purposes. + bool VerifyNextChunkNonFatal(); const ResChunk_header* next_chunk_; size_t len_; const char* last_error_; + bool last_error_was_fatal_ = true; }; } // namespace android