signs of int overflow in nsCanvasRenderingContext2D::GetImageData

RESOLVED FIXED

Status

()

Core
Canvas: 2D
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: georgi - hopefully not receiving bugspam, Assigned: vlad)

Tracking

({fixed1.8.1})

Trunk
x86
Linux
fixed1.8.1
Points:
---
Bug Flags:
blocking1.8.1 -
blocking1.8.0.8 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?][rc ridealong] post ff1.5)

Attachments

(2 attachments, 1 obsolete attachment)

signs of int overflow in nsCanvasRenderingContext2D::GetImageData 

forked from https://bugzilla.mozilla.org/show_bug.cgi?id=353144#c39

testcase seems to overflow probably because of this:

    nsAutoArrayPtr<jsval> jsvector(new jsval[w * h * 4]);

w*h*4 > 1G

(gdb) bt
#0  0xffffe410 in __kernel_vsyscall ()
#1  0xb73827b6 in nanosleep () from /lib/tls/libc.so.6
#2  0xb73825df in sleep () from /lib/tls/libc.so.6
#3  0xb7eedbeb in ah_crap_handler (signum=11) at nsSigHandlers.cpp:134
#4  0xb7f06948 in nsProfileLock::FatalSignalHandler (signo=11)
    at nsProfileLock.cpp:210
#5  <signal handler called>
#6  0xb65d8162 in nsCanvasRenderingContext2D::GetImageData (this=0x8fc11a0)
    at
/opt/joro/firefox/mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp:2910
(gdb) frame 6
#6  0xb65d8162 in nsCanvasRenderingContext2D::GetImageData (this=0x8fc11a0)
    at
/opt/joro/firefox/mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp:2910
2910                *dest++ = INT_TO_JSVAL(b);
(gdb) p dest
$7 = (jsval *) 0xb1695000

$ grep -i b16950 /proc/1561/maps 
715f3000-b1695000 rwxp 715f3000 00:00 0 
b1695000-b16b9000 r-xp 00000000 03:08 1541471   
/usr/share/fonts/default/Type1/n021023l.pfb

(gdb) p (w*h*4)/(1024*1024)
$9 = 1024
Created attachment 241040 [details]
probably int overflow, slow, uses ~ 2G VM
(Reporter)

Updated

12 years ago
Component: Security → Security
Product: Firefox → Core
(Reporter)

Updated

12 years ago
Component: Security → Layout: Canvas
ffs.. jsval is a 32-bit value, so yeah, this can overflow.  I need to write a helper function for these kinds of allocations, but, for now, I'll have a quickie patch shortly.
Flags: blocking1.8.1?
Created attachment 241075 [details] [diff] [review]
fix

Fix -- needs review, I don't have a full tree to build this moment but I can do so in about an hour.
Assignee: nobody → vladimir
Status: NEW → ASSIGNED
Attachment #241075 - Flags: superreview?(bugmail)
Attachment #241075 - Flags: review?
Attachment #241075 - Flags: approval1.8.1?
Attachment #241075 - Flags: review? → review?(mconnor)
Vlad, mconnor tells me that you were thinking this wasn't bad enough to cause a respin on its own, so I'm minusing and adding a whiteboard comment to take it as a ridealong if we do respin the RC. Renominate if you think that it's bad enough to cause a respin on its own.
Flags: blocking1.8.1? → blocking1.8.1-
Whiteboard: [rc ridealong]
Attachment #241075 - Flags: superreview?(bugmail)
Attachment #241075 - Flags: superreview+
Attachment #241075 - Flags: review?(mconnor)
Attachment #241075 - Flags: review+
Comment on attachment 241075 [details] [diff] [review]
fix

Sorry, didn't mean to steal the review as well
Attachment #241075 - Flags: review+ → review?(mconnor)
Flags: blocking1.9?
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
disclaimer: my c++ is lame.

i suspect that in some cases of operator new[], g++ stores *somewhere* the number of objects to be allocated (probably to know how many destructors to call on delete[]). *probably* this is in the allocated block. this seems confirmed by a very ugly overload of `operator new`.

so i suggest a fix along the lines of:

if ( (PRUint32) w*h*4 > (PRUint32) -(1<<20) ) 
 return NS_ERROR_INVALID_ARG;
(In reply to comment #6)
oops
i meant:
if ( (PRUint32) w*h*4 > ((PRUint32) -(1<<20))/sizeof(jsval) ) 
  return NS_ERROR_INVALID_ARG;
 

brendan, bz:

can someone comment how g++'s delete[] know how many destructors to call without storing the number of objects of new[] in the allocated block?

last time i read about it was explained as 'magic'.
(In reply to comment #8)
> brendan, bz:
> 
> can someone comment how g++'s delete[] know how many destructors to call
> without storing the number of objects of new[] in the allocated block?

That's a good point.  I like your suggested fix -- I'll respin the patch shortly.
on g++ 4 classes without destructors seem safe, but ones with destructors overflow like this:

#include <stdio.h>
using namespace std;
#include <new>

class S {
  int i;
  
public:
  S()  { puts("S::S()"); i=1;}
  ~S() { puts("S::~S()"); }
};

int main()
{
	S *b=NULL;
	volatile size_t si= (size_t)-4;
	b=new S[(si)/sizeof(S)];
	delete[] b;
	return(0);
}
Created attachment 241099 [details] [diff] [review]
overflow fix #2

Checked in to branch and trunk, over-the-shoulder review from mscott and a+ from schrep.
Attachment #241075 - Attachment is obsolete: true
Attachment #241099 - Flags: review+
Attachment #241075 - Flags: review?(mconnor)
Attachment #241075 - Flags: approval1.8.1?
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Flags: blocking1.9?
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Keywords: fixed1.8.1
Resolution: --- → FIXED
Flags: blocking1.8.0.8?
Please, please add some comments explaining the magic here....
from inspection the patch seems to have very low risk of regression, except for the following minor side effect:

on 64+ bit systems with a lot of vm, lusers won't be able to use very large objects. doesn't seem a risk at the moment. actually previous checks for |w| and |h| already do this, so this is not limited to the patch.
(In reply to comment #12)
> Please, please add some comments explaining the magic here....
> 

if you are writing about comments in the patch i suspect they were removed on purpose so `bad guys' don't learn some tricks ;) patch 'fix' has nice comments. 
> i suspect they were removed on purpose so `bad guys' don't learn some tricks

I think that's bullshit, sorry.  It's clear what the patch is doing -- preventing overflow.  The checkin comment said so.  What's not clear is why this particular algorithm is needed to prevent overflow.  Simply mentioning the business about array size info being stored in the allocated block by GCC etc would help a lot to keep this code maintainable and wouldn't tell the skript kiddiez anything useful, imo.
With the testcase, I get this with a branch build:
Error: uncaught exception: [Exception... "Component returned failure code: 0x8007000e (NS_ERROR_OUT_OF_MEMORY) [nsIDOMCanvasRenderingContext2D.getImageData]"  nsresult: "0x8007000e (NS_ERROR_OUT_OF_MEMORY)"  location: "JS frame :: https://bugzilla.mozilla.org/attachment.cgi?id=241040 :: v :: line 14"  data: no]
I get this error with branch builds before and after the patch has been checked in.

With the 2006-10-04 trunk build, I still crash.
Talkback ID: TB24140179K
kernel32.dll + 0x12a5b (0x7c812a5b)
MSVCR80.dll + 0x28dd3 (0x60258dd3)
MSVCR80.dll + 0x30e59 (0x60260e59)
nsCanvasRenderingContext2D::GetImageData  [mozilla\content\canvas\src\nscanvasrenderingcontext2d.cpp, line 2874]
XPTC_InvokeByIndex  [mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp, line 102]
XPCWrappedNative::CallMethod  [mozilla\js\src\xpconnect\src\xpcwrappednative.cpp, line 2197]

So I'm reopening the bug for that.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 17

12 years ago
(In reply to comment #16)

I think this is the new throws on vc8 trunk

 	kernel32.dll!7c812a5b() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	
 	kernel32.dll!7c812a5b() 	
>	msvcr80d.dll!_onexit_nolock(int (void)* func=0xe06d7363)  Line 157 + 0x9 bytes	C
 	msvcr80d.dll!_CxxThrowException(void * pExceptionObject=0x0012dbe8, const _s__ThrowInfo * pThrowInfo=0x103037e4)  Line 166	C++
 	msvcr80d.dll!operator new(unsigned int size=1073872900)  Line 64	C++
 	gklayout.dll!operator new[](unsigned int count=1073872900)  Line 7 + 0x9 bytes	C++
 	gklayout.dll!nsCanvasRenderingContext2D::GetImageData()  Line 2880 + 0x10 bytes	C++
this is different from |new| throwing.

but to reproduce it is needed a lot of VM, otherwise new throws or maybe null dereference.

tested on linux 2.6 with 1.5G ram and 4G swap.

building at the moment, will let you know.


on a 1.8 branch nightly i get NS_ERROR_ILLEGAL value and firefox survives.
fixed on cvs trunk and 1.8 branch:

JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIDOMCanvasRenderingContext2D.getImageData]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: https://bugzilla.mozilla.org/attachment.cgi?id=241040 :: v :: line 14"  data: no]

Martijn, i consider this fixed by vlad.

if your concerns are about new throwing check:
|new| throws in canvas Bug 353144

though the testcases are similar, this is different from just out of memory.

if you have a linux box, you may try adding 4G of swap in a file via
mkswap(8) and swapon(8)
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
blocking1.8.0.8 doesn't seem to make sense - 1.8.0.7 does support getImageData
(In reply to comment #21)
> blocking1.8.0.8 doesn't seem to make sense - 1.8.0.7 does support getImageData
> 
1.8.0.7 does NOT support getImageData
(In reply to comment #20)
> Martijn, i consider this fixed by vlad.
> 
> if your concerns are about new throwing check:
> |new| throws in canvas Bug 353144

Ok, I filed bug 355498, because I still crash with the testcase on trunk windows builds. Bug 353144 doesn't seem related to this.

(In reply to comment #23)
> Ok, I filed bug 355498, because I still crash with the testcase on trunk
> windows builds. Bug 353144 doesn't seem related to this.

ok. can't see the bug so can't comment, but according to my tests this is fixed on linux.

 

This is not needed on the 1.8.0 branch.
Flags: blocking1.8.0.8? → blocking1.8.0.8-
Whiteboard: [rc ridealong] → [rc ridealong] post ff1.5
(In reply to comment #12)
> Please, please add some comments explaining the magic here....

Sorry, I should have done so;  I'll add comments to the trunk at least.
Whiteboard: [rc ridealong] post ff1.5 → [sg:critical?][rc ridealong] post ff1.5
Group: security
You need to log in before you can comment on or make changes to this bug.