522767 - Off by one error in nsDataObj::GetFile() triggers assert

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

nsDataObj::GetFile() has an off by one error on the data flavors array.  Because it increments the counter before reading it skips the first element of the array and tries to read one past the last.  This triggers an assert at xpcom/glue/nsTArray.h at line 317 when it tries to read.

This bug is encountered when trying to drag a file to the desktop when it does not support the type "application/x-moz-nativeimage".

I don't think this is security-sensitive but just covering the bases.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Pretty trivial.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This was broken by http://hg.mozilla.org/mozilla-central/rev/3ca2646bf687

Comment 3

[Jeff Walden [:Waldo]]

(In reply to comment #0)
> I don't think this is security-sensitive but just covering the bases.

Good call.  Generally, unless it's immediately obvious, err on the side of caution.  In this case, since nsTArray has no fenceposting behavior to fill the last spot with some "safe" value (e.g. NULL if it were an array of pointers, or conceivably nsCString() in this case), it's not obvious things couldn't go wrong in attacker-controllable ways.  Exactly what bad would happen if execution continued past that point isn't obvious, so paranoia is desirable.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 406742 [details] [diff] [review]
Patch

We'll want to take this on 1.9.2 as well.  This doesn't affect 1.9.1.

Comment 5

[Reed Loden [:reed]]

Comment on attachment 406742 [details] [diff] [review]
Patch

As per our review policy (http://www.mozilla.org/hacking/reviewers.html), all security bugs require superreview for patches.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #5)
> (From update of attachment 406742 [details] [diff] [review])
> As per our review policy (http://www.mozilla.org/hacking/reviewers.html), all
> security bugs require superreview for patches.

Ah I was not aware of this policy.  Thanks for fixing things up for me.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/13f0f58e3d5c

Thanks!!

Comment 8

[Jeff Muizelaar [:jrmuizel]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e76537cc2bf1