680717 - ABORT: CRT ASSERT C:\Program Files\Microsoft Visual Studio 8\VC\INCLUDE\vector(756) : Assertion failed: vector subscript out of range

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bob Clary [:bc] (inactive)]

0. apply patch from bug 587982
1. http://joomla.fotografszczecin.pl/ in winxp beta, aurora, nightly
2. 
ABORT: CRT ASSERT C:\Program Files\Microsoft Visual Studio 8\VC\INCLUDE\vector(756) : Assertion failed: vector subscript out of range

 	mozalloc.dll!mozalloc_abort(const char * const msg=0x00128b44)  Line 77	C++
 	xul.dll!MSCRTReportHook(int aReportType=2, char * aMessage=0x00128b44, int * oReturnValue=0x00122b0c)  Line 1351 + 0xa bytes	C++
 	msvcr80d.dll!_VCrtDbgReportW(int nRptType=2, const wchar_t * szFile=0x02befcd8, int nLine=756, const wchar_t * szModule=0x00000000, const wchar_t * szFormat=0x02befc98, char * arglist=0x0012cbd4)  Line 608 + 0x18 bytes	C
 	msvcr80d.dll!_CrtDbgReportWV(int nRptType=2, const wchar_t * szFile=0x02befcd8, int nLine=756, const wchar_t * szModule=0x00000000, const wchar_t * szFormat=0x02befc98, char * arglist=0x0012cbd4)  Line 300 + 0x1d bytes	C++
 	msvcr80d.dll!_CrtDbgReportW(int nRptType=2, const wchar_t * szFile=0x02befcd8, int nLine=756, const wchar_t * szModule=0x00000000, const wchar_t * szFormat=0x02befc98, ...)  Line 317 + 0x1d bytes	C++
 	msvcp80d.dll!std::_Debug_message(const wchar_t * message=0x02befc98, const wchar_t * file=0x02befcd8, unsigned int line=756)  Line 22 + 0x16 bytes	C++
 	xul.dll!std::vector<ots::OpenTypeCMAPSubtableVSRange,std::allocator<ots::OpenTypeCMAPSubtableVSRange> >::operator[](unsigned int _Pos=3)  Line 756 + 0x15 bytes	C++
 	xul.dll!`anonymous namespace'::Parse0514(ots::OpenTypeFile * file=0x0012cf50, const unsigned char * data=0x0b600a74, unsigned int length=26670, unsigned short num_glyphs=23058)  Line 491 + 0x1a bytes	C++
>	xul.dll!ots::ots_cmap_parse(ots::OpenTypeFile * file=0x0012cf50, const unsigned char * data=0x0b600a40, unsigned int length=261314)  Line 772 + 0x37 bytes	C++
 	xul.dll!`anonymous namespace'::ProcessGeneric(ots::OpenTypeFile * header=0x0012cf50, ots::OTSStream * output=0x0012cfe8, const unsigned char * data=0x0b600040, unsigned int length=5050524, const std::vector<`anonymous namespace'::OpenTypeTable,std::allocator<`anonymous namespace'::OpenTypeTable> > & tables=[16]({tag=1163084098 chksum=462297304 offset=5045980 ...},{tag=541476419 chksum=1965602577 offset=263908 ...},{tag=1195987780 chksum=1030459575 offset=5046208 ...},{tag=1397706823 chksum=252133089 offset=4718536 ...},{tag=1112888135 chksum=953238870 offset=4748888 ...},{tag=841962319 chksum=2018942663 offset=368 ...},{tag=1196576598 chksum=3560227398 offset=4957768 ...},{tag=1885433187 chksum= off,...), ots::Buffer & file={...})  Line 442 + 0x23 bytes	C++
 	xul.dll!`anonymous namespace'::ProcessTTF(ots::OpenTypeFile * header=0x0012cf50, ots::OTSStream * output=0x0012cfe8, const unsigned char * data=0x0b600040, unsigned int length=5050524)  Line 237 + 0x1d bytes	C++
 	xul.dll!ots::Process(ots::OTSStream * output=0x0012cfe8, const unsigned char * data=0x0b600040, unsigned int length=5050524)  Line 598 + 0x15 bytes	C++
 	xul.dll!SanitizeOpenTypeData(const unsigned char * aData=0x0b600040, unsigned int aLength=5050524, unsigned int & aSaneLength=22562023, bool aIsCompressed=false)  Line 366 + 0x11 bytes	C++
 	xul.dll!gfxUserFontSet::OnLoadComplete(gfxProxyFontEntry * aProxy=0x078044a0, const unsigned char * aFontData=0x0b600040, unsigned int aLength=5050524, unsigned int aDownloadStatus=0)  Line 484 + 0x1f bytes	C++
 	xul.dll!nsFontFaceLoader::OnStreamComplete(nsIStreamLoader * aLoader=0x07b04940, nsISupports * aContext=0x00000000, unsigned int aStatus=0, unsigned int aStringLen=5050524, const unsigned char * aString=0x0b600040)  Line 242 + 0x20 bytes	C++

bp-9dca633e-4093-4b79-bb09-43d102110820
Crash [@ WaitForSingleObjectEx | WaitForSingleObject | google_breakpad::ExceptionHandler::WriteMinidumpOnHandlerThread(_EXCEPTION_POINTERS*, MDRawAssertionInfo*) ]

Associated socorro signature for the url was [@ _invalid_parameter_noinfo ]

Security Sensitive due to the vector subscript out of range.

Comment 1

[Bob Clary [:bc] (inactive)]

This is also Windows 7 as well. For some reason I didn't get the abort message and missed it the first time.

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: testcase

Download and save this in the same directory:
http://joomla.fotografszczecin.pl/kozuka.otf

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: patch v1 - use the correct loop index variable

Looks like a simple typo in the OTS cmap-parsing code - it uses the wrong loop index (i instead of j) for one of the vector-subscript operations.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Bob Clary [:bc:] from comment #2)
> Created attachment 554686 [details]
> testcase

Note that the testcase as attached is missing a closing } on the @font-face rule, which may prevent it actually loading the font and hence crashing.

Comment 5

[Jonathan Kew [:jfkthame]]

Filed http://code.google.com/p/chromium/issues/detail?id=93654 against the upstream OTS code.

Comment 6

[Bob Clary [:bc] (inactive)]

Jonathan, please remove the security flag if this isn't sensitive. Thanks!

Comment 7

[John Daggett (:jtd)]

Comment on attachment 554701 [details] [diff] [review]
patch v1 - use the correct loop index variable

Yikes!

Comment 8

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3d4e0b24e7d1

Comment 9

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/mozilla-central/rev/3d4e0b24e7d1

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to Bob Clary [:bc:] from comment #6)
> Jonathan, please remove the security flag if this isn't sensitive. Thanks!

I think this is a potential DoS/crash, if a downloaded font can cause the out-of-bounds vector access to attempt to read an invalid memory location (or if the CRT throws an exception or otherwise terminates the program).

As the problem is only a read access, not write, it can't directly be used to cause further mischief.

And if it doesn't lead to program termination, I think the worst effect it might have is that we incorrectly reject a valid font, or incorrectly accept one with a bad Variation Selector subtable in the cmap. There's a (remote, I guess) possibility that might lead to further problems when the font is passed to platform APIs, if they don't handle the incorrect table robustly.

So I think the risk here is pretty low; still, my inclination would be to leave it marked as security-sensitive for a little while. Given how simple and safe the fix is, we might also want to consider it for Aurora and Beta so as to eliminate even this low risk from those releases.

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 554701 [details] [diff] [review]
patch v1 - use the correct loop index variable

Nominating this for Aurora and Beta consideration, as it's a very simple and safe fix for a potential DoS/crash due to a maliciously-crafted downloadable font. It's not clear whether there might be a possibility of building a more serious exploit on top of this - I think it's unlikely, but can't rule it out entirely. (See comment #10.)

I'd say the risk involved in taking this patch is about as close to zero as a code patch can be - it's a tiny and obviously-correct change to a single line.

Comment 12

[Christopher Blizzard (:blizzard)]

Comment on attachment 554701 [details] [diff] [review]
patch v1 - use the correct loop index variable

Approved for Aurora (Update 8) and Beta (Update 7.)

Can you tell us if this affects shipping Firefox 6 as well?  If so, we might have to re-spin there.

Comment 13

[Christopher Blizzard (:blizzard)]

(Err, spin for an update?)

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to Christopher Blizzard (:blizzard) from comment #12)
> Comment on attachment 554701 [details] [diff] [review]
> patch v1 - use the correct loop index variable
> 
> Approved for Aurora (Update 8) and Beta (Update 7.)
> 
> Can you tell us if this affects shipping Firefox 6 as well? 

If I'm reading the repositories correctly, the same code is in the shipping Fx6, at http://hg.mozilla.org/releases/mozilla-release/file/fe6301c8b0e0/gfx/ots/src/cmap.cc#l491.

So the issue is present, and it'd be a good thing to fix if we need to do a 6.0.1 update for some other reason. IMO, I don't think it justifies an update by itself. Unless we see a POC for something more sinister, or at least a plausible idea of how an exploit could be achieved, the fact that it could probably be used to crash the browser doesn't make it any worse than simple out-of-memory crashes that can be crafted in a few lines of JS.

Comment 15

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/94acb6de0e16
http://hg.mozilla.org/releases/mozilla-beta/rev/c133ee57ff47

Comment 16

[John Daggett (:jtd)]

To be clear, the out-of-range reference is a read, not a write.

Comment 17

[LegNeato]

Does this affect 1.9.2? If so we'll need to take it for 1.9.2.21

Comment 18

[Jonathan Kew [:jfkthame]]

No, 1.9.2 has an earlier version of OTS that didn't include the code where this bug lurked.

Marking status1.9.2=unaffected.

Comment 19

[u279076]

qa- as no QA fix verification needed