To demonstrate the severity of this problem, let's have a look at the division-by-zero issue related with the assert macro.

In file opj_intmath.h, there are two functions use assert to check conditions. However, it can cause division-by-zero problems if the value of variable b is zero.

/**
Divide an integer and round upwards
@return Returns a divided by b
*/
static INLINE OPJ_INT32 opj_int_ceildiv(OPJ_INT32 a, OPJ_INT32 b) {
	assert(b);
	return (a + b - 1) / b;
}
/**
Divide an integer and round upwards
@return Returns a divided by b
*/
static INLINE OPJ_UINT32  opj_uint_ceildiv(OPJ_UINT32  a, OPJ_UINT32  b) {
	assert(b);
	return (a + b - 1) / b;
}


I'll attach a proof-of-concept file tomorrow.

another security issue related to this topic:
https://bugs.chromium.org/p/chromium/issues/detail?id=613160

> [$3000][613160] High CVE-2016-1681: Heap overflow in PDFium. Credit to Aleksandar Nikolic of Cisco Talos.

Division-By-Zero PoC
--------------------
(4854.354): Integer divide-by-zero - code c0000094 (!!! second chance !!!)
eax=ffffffff ebx=0000001f ecx=0998ae00 edx=ffffffff esi=09986f18 edi=00000002
eip=0137b45a esp=003dd278 ebp=003dd2ac iopl=0         nv up ei ng nz na pe nc
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010286
pdfium_test!opj_int_ceildiv+0x1 [inlined in pdfium_test!opj_pi_next_pcrl+0x1fa]:
0137b45a f77d08          idiv    eax,dword ptr [ebp+8] ss:002b:003dd2b4=00000000

0:000> k
pdfium_test!opj_int_ceildiv+0x1 [third_party\libopenjpeg20\opj_intmath.h @ 121]
pdfium_test!opj_pi_next_pcrl+0x1fa [third_party\libopenjpeg20\pi.c @ 443]
pdfium_test!opj_t2_decode_packets+0x18a [third_party\libopenjpeg20\t2.c @ 447]
pdfium_test!opj_tcd_t2_decode+0x3d [third_party\libopenjpeg20\tcd.c @ 1558]
pdfium_test!opj_tcd_decode_tile+0x54 [third_party\libopenjpeg20\tcd.c @ 1297]
pdfium_test!opj_j2k_decode_tile+0x67 [third_party\libopenjpeg20\j2k.c @ 8069]
pdfium_test!opj_j2k_decode_tiles+0xcd [third_party\libopenjpeg20\j2k.c @ 9606]
pdfium_test!opj_jp2_exec+0x36 [third_party\libopenjpeg20\jp2.c @ 2246]
pdfium_test!opj_j2k_decode+0x50 [third_party\libopenjpeg20\j2k.c @ 9806]
pdfium_test!opj_jp2_decode+0x24 [third_party\libopenjpeg20\jp2.c @ 1487]
pdfium_test!opj_decode+0x27 [third_party\libopenjpeg20\openjpeg.c @ 412]
pdfium_test!CJPX_Decoder::Init+0x17f [core\fxcodec\codec\fx_codec_jpx_opj.cpp @ 764]
pdfium_test!CCodec_JpxModule::CreateDecoder+0x40 [core\fxcodec\codec\fx_codec_jpx_opj.cpp @ 887]
pdfium_test!CPDF_DIBSource::LoadJpxBitmap+0x67 [core\fpdfapi\fpdf_render\fpdf_render_loadimage.cpp @ 636]
pdfium_test!CPDF_DIBSource::CreateDecoder+0x241 [core\fpdfapi\fpdf_render\fpdf_render_loadimage.cpp @ 593]
pdfium_test!CPDF_DIBSource::StartLoadDIBSource+0x17d [core\fpdfapi\fpdf_render\fpdf_render_loadimage.cpp @ 311]
pdfium_test!CPDF_ImageCacheEntry::StartGetCachedBitmap+0x67 [core\fpdfapi\fpdf_render\fpdf_render_cache.cpp @ 277]
pdfium_test!CPDF_PageRenderCache::StartGetCachedBitmap+0xcf [core\fpdfapi\fpdf_render\fpdf_render_cache.cpp @ 124]
pdfium_test!CPDF_ImageLoaderHandle::Start+0x44 [core\fpdfapi\fpdf_render\fpdf_render_loadimage.cpp @ 1504]
pdfium_test!CPDF_ImageLoader::Start+0x5d [core\fpdfapi\fpdf_render\fpdf_render_loadimage.cpp @ 1565]
pdfium_test!CPDF_ImageRenderer::StartLoadDIBSource+0x70 [core\fpdfapi\fpdf_render\fpdf_render_image.cpp @ 342]
pdfium_test!CPDF_ImageRenderer::Start+0x74 [core\fpdfapi\fpdf_render\fpdf_render_image.cpp @ 486]
pdfium_test!CPDF_RenderStatus::ContinueSingleObject+0xc3 [core\fpdfapi\fpdf_render\fpdf_render.cpp @ 288]
pdfium_test!CPDF_ProgressiveRenderer::Continue+0x2f2 [core\fpdfapi\fpdf_render\fpdf_render.cpp @ 1057]
pdfium_test!FPDF_RenderPage_Retail+0x221 [fpdfsdk\fpdfview.cpp @ 918]
pdfium_test!FPDF_RenderPageBitmap+0x99 [fpdfsdk\fpdfview.cpp @ 654]
pdfium_test!RenderPage+0x1d2 [samples\pdfium_test.cc @ 519]
pdfium_test!RenderPdf+0x2b8 [samples\pdfium_test.cc @ 694]
pdfium_test!main+0x42e [samples\pdfium_test.cc @ 975]
pdfium_test!invoke_main+0x1d [f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl @ 74]
pdfium_test!__scrt_common_main_seh+0xff [f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl @ 264]
kernel32!BaseThreadInitThunk+0xe
ntdll!__RtlUserThreadStart+0x70
ntdll!_RtlUserThreadStart+0x1b


Source code
-----------
static INLINE OPJ_INT32 opj_int_ceildiv(OPJ_INT32 a, OPJ_INT32 b) {
	assert(b);  // <---------- check passed
	return (a + b - 1) / b;  // <----------- b == 0
}

Deleted: division-by-zero.j2k 293 bytes

division-by-zero.j2k 293 bytes Download

Deleted: division-by-zero.pdf 1.3 KB

division-by-zero.pdf 1.3 KB Download

Oh no, I forgot to add 'PDFium' in the title. ochang@

Tom, can you please triage?

Thanks for the report.

Memory leaks and floating point exceptions aren't considered to be security vulnerabilities.

While there has been a past pattern of asserts that would've prevented security issues, there isn't any vulnerability demonstrated here -- flipping labels.

OK. But I still think use assert to check conditions under Windows is a bad idea.
I'll open a security issue if I'm lucky enough to find a vulnerability. :)

ochang@, can you take a look at this again in light of https://bugs.chromium.org/p/chromium/issues/detail?id=616686?

re https://bugs.chromium.org/p/chromium/issues/detail?id=616686#c11:

stackexploit, I missed your point about Linux release builds including asserts while Windows doesn't -- this shouldn't be the case. How are you building pdfium on Linux? Asserts should be disabled in release builds regardless of platform. 

Enabling all asserts for release builds is not feasible, nor the right way to fix any security vulnerabilities. The asserts you've pointed out are all in third party libraries, where a big chunk of these asserts are almost certainly bad/wrong.

Hi ochang@, thanks for commenting.

For one thing, asserts are definitely disabled in pdfium's release builds regardless of platform. Sorry for my wrong descriptions.

For another, I couldn't agree more with what you said. The assert macro is widely used in these third party libraries. What I want to say is that these asserts may lead to security risks.



 

Dan, for routing this issue. I'm not going to be looking at this.  Thanks.

What are the steps in order to repro this leak? Does it happen under msan builds?

I just debugged it. The assert check will be failed in debug mode because p_tcp->ppt_buffer hold a heap buffer here.

3810    assert(p_tcp->ppt_buffer == NULL);   // SKIPPED IN RELEASE VERSION!!!
38XX    p_tcp->ppt_buffer = (OPJ_BYTE *) opj_malloc(l_ppt_data_size);   // MEMORY LEAK!!!

I've uploaded a patch for this issue which is available at https://codereview.chromium.org/2353463002

I'm not sure what you mean by debugged it, is there a pdf that fails with this assert enabled? Is there some way we can reproduce the issue in order to verify that it is fixed?

Yes of course. I've uploaded a pdf file in the original post. Please download the memory_leak.pdf file to reproduce it.

Too bad nobody forwarded this upstream. FYI, openjpeg finally fixed this recently. It's effectively https://codereview.chromium.org/2353463002

We'll patch it in.

https://github.com/uclouvain/openjpeg/commit/0c913b0aba409148b51ca43d45c50ae595449723

There was a previous attempt to fix it in a different way, but that got reverted.
https://github.com/uclouvain/openjpeg/commit/9906fbf737692486cebabe98169988d818e2e66a
https://github.com/uclouvain/openjpeg/commit/832dfd18665da08745748bde2d2563f00c7cd9e7

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/0fa150a12267b69abcfe5e380b698bbbbd37d5de

commit 0fa150a12267b69abcfe5e380b698bbbbd37d5de
Author: Lei Zhang <thestig@chromium.org>
Date: Wed Aug 01 17:44:48 2018

Fix assertion in opj_j2k_merge_ppt().

This patches in:
https://github.com/uclouvain/openjpeg/commit/832dfd18
https://github.com/uclouvain/openjpeg/commit/0c913b0a

Also clean up a duplicate patch number and update README.pdfium.

BUG= chromium:614691 

Change-Id: I282abfe227e2f667418e5d9058e96e253b220de7
Reviewed-on: https://pdfium-review.googlesource.com/39352
Reviewed-by: Nicolás Peña Moreno <npm@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>

[rename] https://crrev.com/0fa150a12267b69abcfe5e380b698bbbbd37d5de/third_party/libopenjpeg20/0036-opj_j2k_update_image_dimensions.patch
[modify] https://crrev.com/0fa150a12267b69abcfe5e380b698bbbbd37d5de/third_party/libopenjpeg20/j2k.c
[add] https://crrev.com/0fa150a12267b69abcfe5e380b698bbbbd37d5de/third_party/libopenjpeg20/0037-opj_j2k_merge_ppt_leak.patch
[modify] https://crrev.com/0fa150a12267b69abcfe5e380b698bbbbd37d5de/third_party/libopenjpeg20/README.pdfium

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e1cecd2f6b0dbafadae7529e413b6d1c66d57f92

commit e1cecd2f6b0dbafadae7529e413b6d1c66d57f92
Author: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Wed Aug 01 20:48:23 2018

Roll src/third_party/pdfium ad39141331e5..2563fc3895f2 (5 commits)

https://pdfium.googlesource.com/pdfium.git/+log/ad39141331e5..2563fc3895f2


git log ad39141331e5..2563fc3895f2 --date=short --no-merges --format='%ad %ae %s'
2018-08-01 tsepez@chromium.org Make FPDF_FormHandle be represented as an incomplete type.
2018-08-01 thestig@chromium.org Encapsulate some public static methods in CFXJSE_FormCalcContext.
2018-08-01 thestig@chromium.org Fix assertion in opj_j2k_merge_ppt().
2018-08-01 thestig@chromium.org Refactor PatternStringType().
2018-08-01 thestig@chromium.org Roll third_party/freetype/src/ b532d7ce7..578bcf103 (28 commits)


Created with:
  gclient setdep -r src/third_party/pdfium@2563fc3895f2

The AutoRoll server is located here: https://pdfium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.



BUG= chromium:614691 
TBR=dsinclair@chromium.org

Change-Id: Ia0d452e71ddb24a981209c38b55c480b94d7b0e1
Reviewed-on: https://chromium-review.googlesource.com/1159024
Reviewed-by: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#579928}
[modify] https://crrev.com/e1cecd2f6b0dbafadae7529e413b6d1c66d57f92/DEPS