From 5a399de2945d7b244802565d8e9d2f6e662561da Mon Sep 17 00:00:00 2001 From: tsepez Date: Tue, 20 Sep 2016 13:23:21 -0700 Subject: Make CPDF_Array not do indirect object creation. We remove the indirect object holder argument and check that call sites pass ownable objects, adding a reference in one place that always was passing an indirect object. Also check that the invariant isn't violated, we need to fail here in the wild and investigate -- these are existing UAFs. Review-Url: https://codereview.chromium.org/2355083002 --- core/fpdfapi/fpdf_edit/fpdf_edit_create.cpp | 4 +-- core/fpdfapi/fpdf_parser/cpdf_array.cpp | 37 +++++++---------------- core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp | 3 +- core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp | 3 +- core/fpdfapi/fpdf_parser/cpdf_document.cpp | 2 +- core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp | 3 +- core/fpdfapi/fpdf_parser/include/cpdf_array.h | 10 ++---- fpdfsdk/fpdfsave.cpp | 8 ++--- 8 files changed, 26 insertions(+), 44 deletions(-) diff --git a/core/fpdfapi/fpdf_edit/fpdf_edit_create.cpp b/core/fpdfapi/fpdf_edit/fpdf_edit_create.cpp index 45b8711efd..c43306317a 100644 --- a/core/fpdfapi/fpdf_edit/fpdf_edit_create.cpp +++ b/core/fpdfapi/fpdf_edit/fpdf_edit_create.cpp @@ -1940,7 +1940,7 @@ void CPDF_Creator::InitID(FX_BOOL bDefault) { std::vector buffer = PDF_GenerateFileID((uint32_t)(uintptr_t) this, m_dwLastObjNum); CFX_ByteString bsBuffer(buffer.data(), buffer.size()); - m_pIDArray->Add(new CPDF_String(bsBuffer, TRUE), m_pDocument); + m_pIDArray->Add(new CPDF_String(bsBuffer, TRUE)); } } if (!bDefault) { @@ -1955,7 +1955,7 @@ void CPDF_Creator::InitID(FX_BOOL bDefault) { std::vector buffer = PDF_GenerateFileID((uint32_t)(uintptr_t) this, m_dwLastObjNum); CFX_ByteString bsBuffer(buffer.data(), buffer.size()); - m_pIDArray->Add(new CPDF_String(bsBuffer, TRUE), m_pDocument); + m_pIDArray->Add(new CPDF_String(bsBuffer, TRUE)); return; } m_pIDArray->Add(m_pIDArray->GetObjectAt(0)->Clone()); diff --git a/core/fpdfapi/fpdf_parser/cpdf_array.cpp b/core/fpdfapi/fpdf_parser/cpdf_array.cpp index 5e103fa423..da935af47d 100644 --- a/core/fpdfapi/fpdf_parser/cpdf_array.cpp +++ b/core/fpdfapi/fpdf_parser/cpdf_array.cpp @@ -13,6 +13,7 @@ #include "core/fpdfapi/fpdf_parser/include/cpdf_reference.h" #include "core/fpdfapi/fpdf_parser/include/cpdf_stream.h" #include "core/fpdfapi/fpdf_parser/include/cpdf_string.h" +#include "third_party/base/logging.h" #include "third_party/base/stl_util.h" CPDF_Array::CPDF_Array() {} @@ -145,30 +146,22 @@ void CPDF_Array::RemoveAt(size_t i, size_t nCount) { m_Objects.erase(m_Objects.begin() + i, m_Objects.begin() + i + nCount); } -void CPDF_Array::SetAt(size_t i, - CPDF_Object* pObj, - CPDF_IndirectObjectHolder* pObjs) { +void CPDF_Array::SetAt(size_t i, CPDF_Object* pObj) { ASSERT(IsArray()); + CHECK(!pObj || pObj->GetObjNum() == 0); if (i >= m_Objects.size()) { ASSERT(false); return; } if (CPDF_Object* pOld = m_Objects[i]) pOld->Release(); - if (pObj->GetObjNum()) { - ASSERT(pObjs); - pObj = new CPDF_Reference(pObjs, pObj->GetObjNum()); - } + m_Objects[i] = pObj; } -void CPDF_Array::InsertAt(size_t index, - CPDF_Object* pObj, - CPDF_IndirectObjectHolder* pObjs) { - if (pObj->GetObjNum()) { - ASSERT(pObjs); - pObj = new CPDF_Reference(pObjs, pObj->GetObjNum()); - } +void CPDF_Array::InsertAt(size_t index, CPDF_Object* pObj) { + ASSERT(IsArray()); + CHECK(!pObj || pObj->GetObjNum() == 0); if (index >= m_Objects.size()) { // Allocate space first. m_Objects.resize(index + 1, nullptr); @@ -179,37 +172,29 @@ void CPDF_Array::InsertAt(size_t index, } } -void CPDF_Array::Add(CPDF_Object* pObj, CPDF_IndirectObjectHolder* pObjs) { - if (pObj->GetObjNum()) { - ASSERT(pObjs); - pObj = new CPDF_Reference(pObjs, pObj->GetObjNum()); - } +void CPDF_Array::Add(CPDF_Object* pObj) { + ASSERT(IsArray()); + CHECK(!pObj || pObj->GetObjNum() == 0); m_Objects.push_back(pObj); } void CPDF_Array::AddName(const CFX_ByteString& str) { - ASSERT(IsArray()); Add(new CPDF_Name(str)); } void CPDF_Array::AddString(const CFX_ByteString& str) { - ASSERT(IsArray()); Add(new CPDF_String(str, FALSE)); } void CPDF_Array::AddInteger(int i) { - ASSERT(IsArray()); Add(new CPDF_Number(i)); } void CPDF_Array::AddNumber(FX_FLOAT f) { - ASSERT(IsArray()); - CPDF_Number* pNumber = new CPDF_Number(f); - Add(pNumber); + Add(new CPDF_Number(f)); } void CPDF_Array::AddReference(CPDF_IndirectObjectHolder* pDoc, uint32_t objnum) { - ASSERT(IsArray()); Add(new CPDF_Reference(pDoc, objnum)); } diff --git a/core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp b/core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp index ef9b65c5c4..f08ee6ca63 100644 --- a/core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp +++ b/core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp @@ -117,8 +117,7 @@ TEST(cpdf_array, Clone) { // Starts object number from 1. int obj_num = i * kNumOfRowElems + j + 1; obj_holder->ReplaceIndirectObjectIfHigherGeneration(obj_num, obj); - arr_elem->InsertAt(j, new CPDF_Reference(obj_holder.get(), obj_num), - obj_holder.get()); + arr_elem->InsertAt(j, new CPDF_Reference(obj_holder.get(), obj_num)); } arr->InsertAt(i, arr_elem); } diff --git a/core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp b/core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp index aeee3382f9..1a84fcb142 100644 --- a/core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp +++ b/core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp @@ -17,6 +17,7 @@ #include "core/fpdfapi/fpdf_parser/include/cpdf_stream.h" #include "core/fpdfapi/fpdf_parser/include/cpdf_string.h" #include "third_party/base/stl_util.h" +#include "third_party/base/logging.h" CPDF_Dictionary::CPDF_Dictionary() {} @@ -169,7 +170,7 @@ bool CPDF_Dictionary::IsSignatureDict() const { } void CPDF_Dictionary::SetFor(const CFX_ByteString& key, CPDF_Object* pObj) { - ASSERT(!pObj || pObj->GetObjNum() == 0); + CHECK(!pObj || pObj->GetObjNum() == 0); auto it = m_Map.find(key); if (it == m_Map.end()) { if (pObj) diff --git a/core/fpdfapi/fpdf_parser/cpdf_document.cpp b/core/fpdfapi/fpdf_parser/cpdf_document.cpp index 020e3544e9..bd11ed26e6 100644 --- a/core/fpdfapi/fpdf_parser/cpdf_document.cpp +++ b/core/fpdfapi/fpdf_parser/cpdf_document.cpp @@ -302,7 +302,7 @@ int InsertNewPage(CPDF_Document* pDoc, pPagesList = new CPDF_Array; pPages->SetFor("Kids", pPagesList); } - pPagesList->Add(pPageDict, pDoc); + pPagesList->Add(new CPDF_Reference(pDoc, pPageDict->GetObjNum())); pPages->SetIntegerFor("Count", nPages + 1); pPageDict->SetReferenceFor("Parent", pDoc, pPages->GetObjNum()); } else { diff --git a/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp b/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp index 5772043367..cbfdf2b9ac 100644 --- a/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp +++ b/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp @@ -813,7 +813,8 @@ TEST(PDFObjectTest, CloneCheckLoop) { objects_holder.AddIndirectObject(dict_obj); EXPECT_EQ(1u, dict_obj->GetObjNum()); dict_obj->SetFor("arr", arr_obj); - arr_obj->InsertAt(0, dict_obj, &objects_holder); + arr_obj->InsertAt( + 0, new CPDF_Reference(&objects_holder, dict_obj->GetObjNum())); CPDF_Object* elem0 = arr_obj->GetObjectAt(0); ASSERT_TRUE(elem0); ASSERT_TRUE(elem0->IsReference()); diff --git a/core/fpdfapi/fpdf_parser/include/cpdf_array.h b/core/fpdfapi/fpdf_parser/include/cpdf_array.h index 8c89a060eb..1e8c612710 100644 --- a/core/fpdfapi/fpdf_parser/include/cpdf_array.h +++ b/core/fpdfapi/fpdf_parser/include/cpdf_array.h @@ -43,15 +43,11 @@ class CPDF_Array : public CPDF_Object { CFX_Matrix GetMatrix(); CFX_FloatRect GetRect(); - void SetAt(size_t index, - CPDF_Object* pObj, - CPDF_IndirectObjectHolder* pObjs = nullptr); - void InsertAt(size_t index, - CPDF_Object* pObj, - CPDF_IndirectObjectHolder* pObjs = nullptr); + void SetAt(size_t index, CPDF_Object* pObj); + void InsertAt(size_t index, CPDF_Object* pObj); void RemoveAt(size_t index, size_t nCount = 1); - void Add(CPDF_Object* pObj, CPDF_IndirectObjectHolder* pObjs = nullptr); + void Add(CPDF_Object* pObj); void AddNumber(FX_FLOAT f); void AddInteger(int i); void AddString(const CFX_ByteString& str); diff --git a/fpdfsdk/fpdfsave.cpp b/fpdfsdk/fpdfsave.cpp index 307163d1af..e5938b2542 100644 --- a/fpdfsdk/fpdfsave.cpp +++ b/fpdfsdk/fpdfsave.cpp @@ -185,10 +185,10 @@ bool SaveXFADocumentData(CPDFXFA_Document* pDocument, } else { CPDF_Stream* pData = new CPDF_Stream; pData->InitStreamFromFile(pDsfileWrite.get(), pDataDict); - pPDFDocument->AddIndirectObject(pData); + uint32_t objnum = pPDFDocument->AddIndirectObject(pData); iLast = pArray->GetCount() - 2; pArray->InsertAt(iLast, new CPDF_String("datasets", FALSE)); - pArray->InsertAt(iLast + 1, pData, pPDFDocument); + pArray->InsertAt(iLast + 1, new CPDF_Reference(pPDFDocument, objnum)); } fileList->push_back(std::move(pDsfileWrite)); } @@ -206,10 +206,10 @@ bool SaveXFADocumentData(CPDFXFA_Document* pDocument, } else { CPDF_Stream* pData = new CPDF_Stream; pData->InitStreamFromFile(pfileWrite.get(), pDataDict); - pPDFDocument->AddIndirectObject(pData); + uint32_t objnum = pPDFDocument->AddIndirectObject(pData); iLast = pArray->GetCount() - 2; pArray->InsertAt(iLast, new CPDF_String("form", FALSE)); - pArray->InsertAt(iLast + 1, pData, pPDFDocument); + pArray->InsertAt(iLast + 1, new CPDF_Reference(pPDFDocument, objnum)); } fileList->push_back(std::move(pfileWrite)); } -- cgit v1.2.3