From 8dee6cab8f10a257d3b551ede6ca85466bf0bac7 Mon Sep 17 00:00:00 2001 From: JUN FANG Date: Wed, 30 Jul 2014 13:46:39 -0700 Subject: Fix the potential integer overflow from 'offset+size' in extension.h and fpdfview.cpp BUG=397258 R=tsepez@chromium.org Review URL: https://codereview.chromium.org/419063002 --- core/include/fxcrt/fx_stream.h | 1 + core/include/fxcrt/fx_system.h | 4 ++++ core/src/fxcrt/extension.h | 46 +++++++++++++++++++++++++++--------------- fpdfsdk/src/fpdfview.cpp | 32 +++++++++++++++++++++-------- 4 files changed, 59 insertions(+), 24 deletions(-) diff --git a/core/include/fxcrt/fx_stream.h b/core/include/fxcrt/fx_stream.h index ef730bb967..8e298f7727 100644 --- a/core/include/fxcrt/fx_stream.h +++ b/core/include/fxcrt/fx_stream.h @@ -30,6 +30,7 @@ FX_DEFINEHANDLE(FX_HFILE) #endif #define FX_FILESIZE off_t #endif +typedef base::CheckedNumeric FX_SAFE_FILESIZE; #define FX_GETBYTEOFFSET32(a) 0 #define FX_GETBYTEOFFSET40(a) 0 #define FX_GETBYTEOFFSET48(a) 0 diff --git a/core/include/fxcrt/fx_system.h b/core/include/fxcrt/fx_system.h index 7488e9d2dc..391380304c 100644 --- a/core/include/fxcrt/fx_system.h +++ b/core/include/fxcrt/fx_system.h @@ -276,6 +276,10 @@ int FXSYS_round(FX_FLOAT f); #ifdef __cplusplus }; +#include "../../../third_party/numerics/safe_math.h" +typedef base::CheckedNumeric FX_SAFE_DWORD; +typedef base::CheckedNumeric FX_SAFE_INT; +typedef base::CheckedNumeric FX_SAFE_SIZET; #if defined(__clang__) || _MSC_VER >= 1700 #define FX_FINAL final #elif defined(__GNUC__) && __cplusplus >= 201103 && \ diff --git a/core/src/fxcrt/extension.h b/core/src/fxcrt/extension.h index b8dce7c97d..d04d1da550 100644 --- a/core/src/fxcrt/extension.h +++ b/core/src/fxcrt/extension.h @@ -7,8 +7,6 @@ #ifndef _FXCRT_EXTENSION_IMP_ #define _FXCRT_EXTENSION_IMP_ -#include "../../../third_party/numerics/safe_math.h" - class IFXCRT_FileAccess { public: @@ -68,9 +66,17 @@ public: } virtual FX_BOOL SetRange(FX_FILESIZE offset, FX_FILESIZE size) { - if (offset < 0 || offset + size > m_pFile->GetSize()) { + if (offset < 0 || size < 0) { + return FALSE; + } + + FX_SAFE_FILESIZE pos = size; + pos += offset; + + if (!pos.IsValid() || pos.ValueOrDie() >= m_pFile->GetSize()) { return FALSE; } + m_nOffset = offset, m_nSize = size; m_bUseRange = TRUE; m_pFile->SetPosition(m_nOffset); @@ -82,13 +88,18 @@ public: } virtual FX_BOOL ReadBlock(void* buffer, FX_FILESIZE offset, size_t size) { + if (m_bUseRange && offset < 0) { + return FALSE; + } + FX_SAFE_FILESIZE pos = offset; + if (m_bUseRange) { - if (offset + size > (size_t)GetSize()) { + pos += m_nOffset; + if (!pos.IsValid() || pos.ValueOrDie() >= (size_t)GetSize()) { return FALSE; } - offset += m_nOffset; } - return (FX_BOOL)m_pFile->ReadPos(buffer, size, offset); + return (FX_BOOL)m_pFile->ReadPos(buffer, size, pos.ValueOrDie()); } virtual size_t ReadBlock(void* buffer, size_t size) { @@ -184,10 +195,12 @@ public: } virtual FX_BOOL SetRange(FX_FILESIZE offset, FX_FILESIZE size) { - base::CheckedNumeric range = size; - range += size; - - if (!range.IsValid() || offset <= 0 || size <= 0 || range.ValueOrDie() > m_nCurSize) { + if (offset < 0 || size < 0) { + return FALSE; + } + FX_SAFE_FILESIZE range = size; + range += offset; + if (!range.IsValid() || range.ValueOrDie() >= m_nCurSize) { return FALSE; } @@ -206,7 +219,7 @@ public: return FALSE; } - base::CheckedNumeric safeOffset = offset; + FX_SAFE_FILESIZE safeOffset = offset; if (m_bUseRange) { safeOffset += m_nOffset; } @@ -217,9 +230,9 @@ public: offset = safeOffset.ValueOrDie(); - base::CheckedNumeric newPos = size; + FX_SAFE_SIZET newPos = size; newPos += offset; - if (!newPos.IsValid() || newPos.ValueOrDefault(0) == 0 || newPos.ValueOrDie() > m_nCurSize) { + if (!newPos.IsValid() || newPos.ValueOrDefault(0) == 0 || newPos.ValueOrDie() >= m_nCurSize) { return FALSE; } @@ -269,7 +282,7 @@ public: offset += (FX_FILESIZE)m_nOffset; } if (m_dwFlags & FX_MEMSTREAM_Consecutive) { - base::CheckedNumeric newPos = size; + FX_SAFE_SIZET newPos = size; newPos += offset; if (!newPos.IsValid()) return FALSE; @@ -295,10 +308,11 @@ public: return TRUE; } - base::CheckedNumeric newPos = size; + FX_SAFE_SIZET newPos = size; newPos += offset; - if (!newPos.IsValid()) + if (!newPos.IsValid()) { return FALSE; + } if (!ExpandBlocks(newPos.ValueOrDie())) { return FALSE; diff --git a/fpdfsdk/src/fpdfview.cpp b/fpdfsdk/src/fpdfview.cpp index b950ed8641..63d4fbdcde 100644 --- a/fpdfsdk/src/fpdfview.cpp +++ b/fpdfsdk/src/fpdfview.cpp @@ -9,7 +9,7 @@ #include "../include/fsdk_rendercontext.h" #include "../include/fpdf_progressive.h" #include "../include/fpdf_ext.h" - +#include "../../third_party/numerics/safe_conversions_impl.h" CPDF_CustomAccess::CPDF_CustomAccess(FPDF_FILEACCESS* pFileAccess) { @@ -35,18 +35,27 @@ FX_BOOL CPDF_CustomAccess::GetByte(FX_DWORD pos, FX_BYTE& ch) FX_BOOL CPDF_CustomAccess::GetBlock(FX_DWORD pos, FX_LPBYTE pBuf, FX_DWORD size) { - if (pos + size > m_FileAccess.m_FileLen) return FALSE; + FX_SAFE_DWORD newPos = size; + newPos += pos; + if (!newPos.IsValid() || newPos.ValueOrDie() >= m_FileAccess.m_FileLen) { + return FALSE; + } + return m_FileAccess.m_GetBlock(m_FileAccess.m_Param, pos, pBuf, size); } FX_BOOL CPDF_CustomAccess::ReadBlock(void* buffer, FX_FILESIZE offset, size_t size) { - // m_FileAccess = *pFileAccess; - // m_BufferOffset = (FX_DWORD)-1; - if (offset + size > m_FileAccess.m_FileLen) return FALSE; - return m_FileAccess.m_GetBlock(m_FileAccess.m_Param, offset,(FX_LPBYTE) buffer, size); + if (offset < 0) { + return FALSE; + } + FX_SAFE_FILESIZE newPos = base::checked_cast(size); + newPos += offset; + if (!newPos.IsValid() || newPos.ValueOrDie() >= m_FileAccess.m_FileLen) { + return FALSE; + } - // return FALSE; + return m_FileAccess.m_GetBlock(m_FileAccess.m_Param, offset,(FX_LPBYTE) buffer, size); } //0 bit: FPDF_POLICY_MACHINETIME_ACCESS @@ -292,8 +301,15 @@ public: virtual FX_FILESIZE GetSize() {return m_size;} virtual FX_BOOL ReadBlock(void* buffer, FX_FILESIZE offset, size_t size) { - if(offset+size > (FX_DWORD)m_size) return FALSE; + if (offset < 0) { + return FALSE; + } + + FX_SAFE_FILESIZE newPos = base::checked_cast(size); + newPos += offset; + if (!newPos.IsValid() || newPos.ValueOrDie() >= (FX_DWORD)m_size) return FALSE; FXSYS_memcpy(buffer, m_pBuf+offset, size); + return TRUE; } private: -- cgit v1.2.3