Bug 24525

Summary:	poppler: missing readEmbFontFile return value checks
Product:	poppler	Reporter:	Tomas Hoger <thoger>
Component:	general	Assignee:	poppler-bugs <poppler-bugs>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	medium
Version:	unspecified
Hardware:	All
OS:	All
Whiteboard:
i915 platform:		i915 features:
Attachments:	Missing readEmbFontFile return value checks reproducer

Description Tomas Hoger 2009-10-14 06:47:43 UTC

Created attachment 30400 [details] [review]
Missing readEmbFontFile return value checks

GfxFont::readEmbFontFile can return NULL if object expected to contain font stream is not stream.  There are couple of calls where the return value is not checked - in PSOutputDev and GfxFont.  There are checks for NULL return value in SplashOutputDev and ArthurOutputDev.

Value returned is later passed to FoFi*::make, which leads to FoFi*::parse that assumes that file is not NULL.  NULL file can hence cause crash.

Attached is possible patch that adds checks where they are missing.

Comment 1 Albert Astals Cid 2009-10-17 15:51:56 UTC

Do you have any file that hits those paths?

Also i think we should add error() when that happens

Comment 2 Kees Cook 2009-10-19 10:52:03 UTC

Created attachment 30562 [details]
reproducer

Here is a reproducer that I stripped the streams out of -- the originator of this PDF did not want it used in public, so I've done my best to remove anything identifying in it.  I still see a NULL deref crash when running it under pdftops, so hopefully it's still useful.

Comment 3 Albert Astals Cid 2009-10-19 14:33:11 UTC

I don't get any crash in pdftops from poppler 0.12.1 with that file :-/

Comment 4 Tomas Hoger 2009-10-20 00:02:40 UTC

Can you try checking the value of fontLen before / after call to readEmbFontFile?  That variable is not initialized anywhere, if you happen to have it set to 0 (or <= 3), it will avoid crash for TrueType font:

http://cgit.freedesktop.org/poppler/poppler/tree/fofi/FoFiBase.cc#n150

called from FoFiTrueType::parse().  So you may not see the crash on certain builds.  Forcing fontLen to non-0 value prior to call to readEmbFontFile can help reproduce.

Comment 5 Albert Astals Cid 2009-10-22 11:32:57 UTC

Ok, i see the problem, i think doing
  *len = 0;
in the error if of GfxFont::readEmbFontFile will fix the problem in a less code intrusive way, what do you think?

Comment 6 Tomas Hoger 2009-10-22 12:21:54 UTC

I got that idea too, but wasn't sure if that's sufficient for FoFiType1C.

After another look at FoFiType1C::parse (and FoFiTrueType::parse), it should actually be sufficient.

Comment 7 Albert Astals Cid 2009-10-22 12:28:48 UTC

Change commited, thanks for reporting :-)

Comment 8 Tomas Hoger 2009-10-22 23:26:06 UTC

ty, np!

for future reference: 4a9bdd30dc353865685e03eb1c1ac6093797695a

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.