1169326 - Several memory-safety bugs due to unsafe use of sprintf

Status: RESOLVED INCOMPLETE

(Core :: General, defect)

Description

[q1]

User Agent: Mozilla/5.0 (Windows; rv:***) Gecko/20100101 Firefox/**.*
Build ID: 20150305021524

Steps to reproduce:

I have found several unsafe calls to sprintf which will or can cause the (usually stack-based) destination buffer to be overrun, possibly leading to crash problems and/or security issues. Here's a list:

38.0.1\media\libstagefright\frameworks\av\media\libstagefright\foundation\AString.cpp
   AString::append (double x) (sprintfs a double (could have 300+ digits) into char s[16])
   AString::append (void *x) (sprintfs a void * into char s[16])

38.0.1\netwerk\protocol\rtsp\rtsp\ASessionDescription.cpp
   ASessionDescription::getDimensions (sprintfs an unsigned long (could have 9 digits) + 12 format chars into char key[20])

38.0.1\mozglue\linker\Mappable.cpp
   _MappableBuffer::Create (sprintfs unchecked string into char path[256])

38.0.1\js\src\jsprf.cpp
   cvt_f (also has a suspect use of js_memcpy on line 303; also can fmt0 > fmt1?)

38.0.1\gfx\thebes\gfxTextRun.cpp
   gfxFontGroup::GetDefaultFont

38.0.1\gfx\layers\LayerSorter.cpp
   SetTextColor

I have not exhaustively surveyed the codebase for this kind of bug, so I guess that there are several more instances of it.

Also I have found several sprintfs of int and related types that are safe only because sizeof(int)==4 bytes on x86 and x64, but would become unsafe if Firefox is ported to an architecture with 8-byte ints. (For an example, see AString::append(int x) in 38.0.1\media\libstagefright\frameworks\av\media\libstagefright\foundation\AString.cpp).

Comment 1

[q1]

See also https://bugzilla.mozilla.org/show_bug.cgi?id=1141227

Comment 2

[Mats Palmgren (inactive)]

(In reply to q1 from comment #0)
> 38.0.
> 1\media\libstagefright\frameworks\av\media\libstagefright\foundation\AString.
> cpp

I think libstagefright might be an upstream library.
I filed bug 1170028.

> 38.0.1\netwerk\protocol\rtsp\rtsp\ASessionDescription.cpp

It looks like bug 1119980 has already fixed this one.

> 38.0.1\mozglue\linker\Mappable.cpp

http://mxr.mozilla.org/mozilla-central/source/mozglue/linker/Mappable.cpp#235
The only caller seems to be mozglue/linker/ElfLoader.cpp and "name"
looks like a file name from our distributed set of files so it's unlikely
to overflow the 256 char buffer.  We should fix this anyway of course.
I filed bug 1170027.

> 38.0.1\js\src\jsprf.cpp

I filed bug 1170025.  (I noted that there is an assertion there
to detect an overflow so it's unlikely to be an issue.)

> 38.0.1\gfx\thebes\gfxTextRun.cpp

This one looks safe because the %.220s format is carefully
chosen so that the whole string fits in the buffer.
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxTextRun.cpp?rev=8a03e892db51#1902
Bug 1119980 has already fixed this one anyway.

> 38.0.1\gfx\layers\LayerSorter.cpp

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayerSorter.cpp#178
This code is #ifdef DEBUG, but we should fix it of course.
I filed bug 1170023.

> I have not exhaustively surveyed the codebase for this kind of bug, so I
> guess that there are several more instances of it.

Thanks for your report.
In the future, please file one bug per directory you find issues in,
so that the bug can be assigned to the responsible parties.

Comment 3

[q1]

[blockquote]In the future, please file one bug per directory you find issues in,
so that the bug can be assigned to the responsible parties.[/blockquote]

Will do.

Comment 4

[Andrew McCreight [:mccr8]]

All of the issues here have had individual bugs filed for them, so I don't think it is useful to leave this one open. I guess I'll mark it "incomplete", though that isn't entirely accurate.