280082 - Overflow on malicious imap: URL

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Daniel Veditz [:dveditz]]

from Peter Winter-Smith of ngssoftware.com:

Hi Guys,
 
I've found a unicode stack overflow in the process of decoding hex encoded
filenames in Thunderbird. This is the test case which *I* used, but I'm sure
this can be exploited in other ways:
 
<iframe
src="imap://peter@localhost:585/fetch%3EUID%3E.INBOX%3E3022?part=1.3&type=image\xml&filename=lame.gif.this.is.a.cool.picture.aaa.aaa.aaa.gif&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;aaaaabbbbbcccccdddddeeeeeff%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'">
Basically, the incorrectly formed hex encoded string, provided that it is at the
end of the filename attribute, and is under the length of MAX_PATH, will
recursively copy chunks of the preceding url into a stack based buffer (of
MAX_PATH, I presume), and will overflow overwriting the saved return address and
saved base pointer with a widechar version of the string. I am guessing there
are duel pointers and the math is going bad, I haven't checked for definite yet.
 
The image\xml tag will cause the browser to (for some reason) think that the
file type is of the type Gif Image (image\gif causes the IFRAME to try and
render the image, doh!). The &nbsp; 's will pad the string out past the end of
the filename textbox in the download dialog, thereby hiding the filename. And
the %'%'%'%' at the end of the string will cause the decoder to mess up and
smash the stack with the string recursively. This example overwrites the saved
return address with 0x00610061
 
The overflow only activates if the file is found to exist ( my example: email
3022  -- content part 1.3 -- maybe we can use external sources, or files within
the same email, I haven't tried it), when the user attempts to save the file.
 
The html code snippit can be placed in an html file and attached to the email,
it automatically renders.

Comment 1

[Daniel Veditz [:dveditz]]

Apart from the overflow, a mail message has no business referencing or
incorporating bits from a different message.

Comment 2

[David :Bienvenu]

Here's the stack trace. "Suffix" is getting overflowed in nsLocalFile::CreateUnique

NTDLL! 77f813b1()
nsDebugImpl::Assertion(nsDebugImpl * const 0x02fadeb8, const char * 0x0027d1b8,
const char * 0x0027d1a0, const char * 0x0027d160, int 0x00000044) line 290
nsDebug::Assertion(const char * 0x0027d1b8, const char * 0x0027d1a0, const char
* 0x0027d160, int 0x00000044) line 109
nsCSubstringTuple::WriteTo(char * 0x080eb910, unsigned int 0x000000fc) line 68 +
35 bytes
nsCSubstring::Assign(const nsCSubstringTuple & {...}) line 390
nsACString::Assign(const nsCSubstringTuple & {...}) line 239
nsACString::nsACString(const nsCSubstringTuple & {...}) line 465
nsLocalFile::CreateUnique(nsLocalFile * const 0x04a328d8, unsigned int
0x00000000, unsigned int 0x00000180) line 116 + 92 bytes
nsExternalAppHandler::SetUpTempFile(nsIChannel * 0x080b2930) line 1571
nsExternalAppHandler::OnStartRequest(nsExternalAppHandler * const 0x080d7008,
nsIRequest * 0x080b2930, nsISupports * 0x00000000) line 1611 + 17 bytes

Comment 3

[Daniel Veditz [:dveditz]]

Taking, overflows in shared file handling code.

Comment 4

[Asa Dotzler [:asa]]

Dan, can you give us an ETA on this one? 

Comment 5

[David :Bienvenu]

Attachment: fix for one crash/hang

this was the one problem I was able to reproduce...

Comment 6

[Darin Fisher]

Comment on attachment 174541 [details] [diff] [review]
fix for one crash/hang

Is the problem that int(maxRootLength) can sometimes be negative?  or are you
concerned with maxRootLength == 0?  i think the == 0 case is benign, or did i
miss something?

Comment 7

[David :Bienvenu]

it can be negative, which causes us to pass a negative number to SetLength,
which hangs.

Comment 8

[Darin Fisher]

Comment on attachment 174541 [details] [diff] [review]
fix for one crash/hang

so, if it can be negative.  does that mean that suffix is somehow too large? 
what's causing maxRootLength to be negative?

Comment 9

[Darin Fisher]

Attachment: v2 - alternate patch

This is an alternate patch.  I decided to simply make the suffix buffer smaller
(max size 100), and then we can be sure that the computation of maxRootLength
will never go negative.  The rest of the patch is just cleanup.

This patch makes it so that the max extension length is 100 (except on XP_MAC,
but who cares about that, right?)

Comment 10

[Darin Fisher]

I verified this patch with a simple "unit" testcase like this:

void main(int argc, char **argv) {
  nsCOMPtr<nsILocalFile> lf;
  NS_NewNativeLocalFile(nsDependentCString(argv[1]), PR_FALSE, 
                        getter_AddRefs(lf));
  lf->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
}

By inspection, the generated file's extension was appropriately limited in length.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 174754 [details] [diff] [review]
v2 - alternate patch

Comment about how kMaxExtensionLength must be < kMaxFilenameLength - 4, and how
any extension longer than 100 chars can't have a MIME type association, so it's
ok to truncate it.

For bonus points, change indx's type to int from short.  In C and C++ it
doesn't pay to use other than int for such loop control, and it can cost if the
compiler doesn't analyze the loop enough to see that i won't wrap a signed
short.

/be

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 174754 [details] [diff] [review]
v2 - alternate patch

r=dveditz
a=dveditz for the 1.7 and aviary1.0.1 branches

Comment 13

[Darin Fisher]

fixed-on-trunk, fixed1.7.6, fixed-aviary1.0.1

Comment 14

[Peter Winter-Smith]

(In reply to comment #13)
> fixed-on-trunk, fixed1.7.6, fixed-aviary1.0.1

Hi guys,

Thank you all for working on this. I have one question however:

Using the vulnerable version of Thunderbird, I have tried to save a file with a
name of length kMaxFilenameLength of the following construction:

'aaaaaaaa....aaaa.gif'

With no bad effects, however, the 224 byte string:

lame.exe.gif%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'

Will cause the overflow, overwriting the saved return pointer and a large amount
of the stack.

Are you sure that you have fixed the correct issue?

Thanks!
-Peter

Comment 15

[Daniel Veditz [:dveditz]]

We're not sure we fixed the right problem, we've had trouble reproducing it. It
would help us tremendously if you could retest it using a build that contains
this patch.

ftp://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2005-02-19-07-aviary1.0.1/

If you use a "Master Password" this build will most likely crash when that comes
up (bug 282894). If that's the case wait a few hours for today's build. Make
sure you go to a directory which says "aviary1.0.1", the others are development
builds that may be unstable and contain a ton of changes from 1.0

Comment 16

[Peter Winter-Smith]

Hi guys,

Sorry the latest build only seems to add a 'report to mozilla' feature when the 
overflow occurs. No fix :-(

I am completely certain that the filename is being passed through a hex decoder 
which is being messed up -- the overflow still occurs.

You will almost certainly be able to repro this if you alter the url in the 
imap protocol handler to point to a valid file on your mailbox (for example, 
email 10, part 1.1 - the message body). Then save the file to your desktop (I 
am using window xp), the malformed hex encoded values (%'%') cause parts of the 
string to recurse (aaaa%'%', for example, may become aaaa%'%' aaaa%'%' aa). 
This recursion overflows the stack. The integer overflow doesn't even come into 
play. Suffix is the buffer overflowed, as far as I can see, but it isn't 
overflowed during the construction of a filename to save to, like you guys are 
working on now, it's during a decoding routine I believe :-)

Thanks for working on this, sorry I can't be code specific!
-Peter

Comment 17

[Peter Winter-Smith]

Hi guys,

Just send yourselves an email with an attachment with the following mime values:

-------------
------=_NextPart_07D5_02_0ECCD5A9.5329E373
Content-Type: application/x-msdos-program;
	name="lame.gif.this.is.a.cool.picture.aaa.aaa.aaa.gif   &
nbsp;            &nb
sp; aaaaabbbbbcccccdddddeeeeeffaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa  foo.gif     &
nbsp;            &nb
sp;            &nbsp
; %'%'%'%'%'%'%'%'%'"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
	filename="lame.gif.this.is.a.cool.picture.aaa.aaa.aaa.gif  &nb
sp;            &nbsp
;  aaaaabbbbbcccccdddddeeeeeffaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa  foo.gif    &nb
sp;            &nbsp
;             &
nbsp; %'%'%'%'%'%'%'%'%'"

-------------

Thunderbird will stack overflow, and we execute an arbitrary code location 
having overwritten the saved return address.

It doesn't matter whether the filename is specified through an imap protocol 
handler or not, it overflows decoding hex encoded filenames however they are 
detected (ie: regardless of the source of the filename, be it protocol handler 
or just mime value)

I hope this helps -- this will repro without needing that specially crafted 
imap protocol handler. For best results, save to your windows desktop folder 
under NT.

Thanks!
-Peter

Comment 18

[Boris Zbarsky [:bzbarsky]]

Reopening per comments.

Comment 19

[Daniel Veditz [:dveditz]]

I hacked up a piece of mail with the info from comment 17. On tbird 1.0 I go
into a busy-loop when I double-click on the attachment. ON the latest 1.0.1
build the helper app dialog comes up. If I try to save I get an invalid filename
error from windows; if I "open with" notepad--which causes the file to be saved
to a temporary copy--I have no problems at all.

Trying the shorter name from comment 14 pretty much the same thing (hang on 1.0,
OK on 1.0.1) except that windows will let me save files with that name.

When you say "NT" do you really mean some old version of NT, or simply "nt
family" as opposed to Win98/ME? I am testing under Windows XP SP2.

Are you still using IMAP?  What happens if you copy the mail to your local
folders first? If that still crashes could you copy it to a brand new local
folder and then attach that to this bug, or cc me on the mail, or both?

Comment 20

[Johnny Stenback (:jst)]

I just downloaded a nightly build of thunderbird from 21-Feb-2005 09:26 and
tested with a message containing the above on an IMAP server, and I see the same
thing that dveditz sees, I just get a dialog saying that the filename is invalid.

Comment 21

[Peter Winter-Smith]

(In reply to comment #20)
> I just downloaded a nightly build of thunderbird from 21-Feb-2005 09:26 and
> tested with a message containing the above on an IMAP server, and I see the 
same
> thing that dveditz sees, I just get a dialog saying that the filename is 
invalid.

Sorry for the late reply guys, my computer had a couple of issues. I have 
downloaded the latest build and have a few observations:

For some reason, now attaching the file as a mime-type doesn't have any ill 
effect when right-clicking and saving the file. I haven't been able to repro 
this any more through this vector. However, the following code in an html page, 
attached (and therefore automatically rendered by thunderbird) to an email, 
will still caused the stack overflow. I have tested this on a new computer with 
a clean install of WinXPSP1 and thunderbird.

<iframe src="imap://peter@localhost:585/fetch%3EUID%3E.INBOX%3E100?
part=1.1&type=image\xml&filename=lame.exe.gif%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%
'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%
'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%'%
'%'%'%'%'%'%'%'%'">

I am willing to bet the reason you got file not found is relating to this:

/fetch%3EUID%3E.INBOX%3E100?part=1.1

in this example, I am trying to reference email 100 (E100) part 1.1 (message 
body), which always succeeds. Please point the "peter@localhost:585" to your 
imap server, and tell me your results :-)

When the email is viewed, a dialog box will pop up, asking you to save the 
attached file. Select 'save', save as a file on the desktop. An error message 
containing garbage appears (as at this point the stack is well and truly 
damaged), and we get our overflow (whereby we control eax, and eip as 
0x00270025 - unicode "%'" ):

ModLoad: 71500000 715fc000   C:\WINDOWS\System32\browseui.dll
ModLoad: 76990000 769b4000   C:\WINDOWS\System32\ntshrui.dll
ModLoad: 76b20000 76b35000   C:\WINDOWS\System32\ATL.DLL
ModLoad: 71c20000 71c6e000   C:\WINDOWS\System32\NETAPI32.dll
ModLoad: 75a70000 75b15000   C:\WINDOWS\system32\USERENV.dll
ModLoad: 71700000 71849000   C:\WINDOWS\System32\SHDOCVW.dll
ModLoad: 76980000 76987000   C:\WINDOWS\System32\LINKINFO.dll
(b34.b3c): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=00270025 ebx=00000000 ecx=0012a0f8 edx=7ffe0304 esi=00182008 edi=0012a97c
eip=00270027 esp=0012a970 ebp=00270025 iopl=0         nv up ei pl nz na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=0038  gs=0000             efl=00010202
00270027 004b73           add     [ebx+0x73],cl           ds:0023:00000073=??

I hope this helps? 

I am sorry that I couldn't construct a working email to cause the overflow to 
send you guys, but it appears that attaching as a mime filename doesn't seem to 
do anything with the latest build, I was relying on that to produce a test case 
you all can use.

-Peter

Comment 22

[David :Bienvenu]

In Purify, on win2k, I get this error:

    LocalFree      [KERNEL32.DLL]
    EqualRect      [USER32.dll]
    DialogBoxIndirectParamA [USER32.dll]
    GetOpenFileNameW [COMDLG32.dll]
    nsFilePicker::ShowW(short *) [nsFilePicker.cpp:237]
              }
              else if (mMode == modeSave) {
                ofn.Flags |= OFN_NOREADONLYRETURN | OFN_NODEREFERENCELINKS;
     =>         result = nsToolkit::mGetSaveFileName(&ofn);
                if (!result) {
                  // Error, find out what kind.
                  if (::GetLastError() == ERROR_INVALID_PARAMETER ||
    nsFilePicker::Show(short *) [nsFilePicker.cpp:367]
        
        NS_IMETHODIMP nsFilePicker::Show(PRInt16 *aReturnVal)
        {
     =>   return ShowW(aReturnVal);
        }
        
        NS_IMETHODIMP nsFilePicker::GetFile(nsILocalFile **aFile)

I don't know if this is related, or if my test case is slightly different...and
I'm not sure what the problem is, other than that windows is unhappy with the
file name we're passing in.

Comment 23

[Johnny Stenback (:jst)]

I also just finally got Purify to like thunderbird here, and I ran it (release
build, todays 1.0.1 build) with my testcase that does what's described above,
and I see no problems and no complaints from Purify that anything would be
wrong. I don't know what more to do here to reproduce this problem :(

Comment 24

[Peter Winter-Smith]

Hey guys,

How frustrating :-(

I have tested it on two completely different boxes, both with xpsp1, and the 
latest version of TB and it breaks in the same way each time on both of them.

Would it help if I were to create a virtual pc/vmware image and stick an imap 
server on it with a testcase which should break it?

If this is desirable, I can't really promise any delivery times as I won't be 
at home for the next three weeks.

Sorry it's so hard to repro, it works fine for me :-/
-Peter

Comment 25

[Johnny Stenback (:jst)]

How about we try to have you email me a testcase first, once I get it I'll
change your login information etc to my own on the imap server, and then try to
load it and see what happens here? Please email to [e-mail]

Comment 26

[Christopher Aillon (sabbatical, not receiving bugmail)]

Can we get an update here?  Time is short for 1.7.6...

Comment 27

[Scott MacGregor]

Let's go ahead and mark this as fixed for 1.0.1 based on feedback from jst,
dveditz and bienvenu after testing against the v2 patch. 

We can spin off another bug to spend more time looking at why Peter still has
problems with this message. 

Comment 28

[Christopher Aillon (sabbatical, not receiving bugmail)]

What about 1.7.6?

Comment 29

[sairuh (rarely reading bugmail)]

Johnny, how did the testcase work for you? if it went well, we can verify this
--especially if it works using a  tbird 1.0.1 nightly build, but I don't know if
you need a debug build to test it...

Comment 30

[Chase Phillips]

Adding fixed1.7.6 keyword since the only patch we have for the issue we can see
has been applied.

Still trying to get the right conditions for reproducing a possible secondary
problem.

Comment 31

[Johnny Stenback (:jst)]

(In reply to comment #29)
> Johnny, how did the testcase work for you? if it went well, we can verify this
> --especially if it works using a  tbird 1.0.1 nightly build, but I don't know if
> you need a debug build to test it...

I was unable to see any problems with the testcase I got, and as far as I know,
there is no problem. But Peter would be the person to tell for sure.

Comment 32

[Peter Winter-Smith]

Attachment: Test case for IMAP overflow

I have also sent a testcase to [e-mail] which may have been
received by those on the list?

That email is the attached html file attached to an email sent to my address.
If using this testcase, please change the user specific values in the email
address, such as the username, server, port and email number/part if necessary!


All the best! Please let me know how this is working out!
-Peter

Comment 33

[David :Bienvenu]

I tried this again on the trunk, on win xp, with the test case - it behaved as
before - I do get the save as dialog, but there's no overflow or crash that I
can see.

Comment 34

[Peter Winter-Smith]

(In reply to comment #33)
> I tried this again on the trunk, on win xp, with the test case - it behaved as
> before - I do get the save as dialog, but there's no overflow or crash that I
> can see.

I have just downloaded Thunderbird 1.0.2, the latest release version, and the 
imap.htm testcase still cause the stack buffer overrun to occur. I am not too 
sure why it won't repro for you guys, I've tested this with success on XPSP1 
and XPSP2 on different machines with the latest version of Thunderbird. Is 
there anything I can do to help you locate the bug some more?

Kindest regards!
-Peter

Comment 35

[Mike Shaver (:shaver emeritus)]

Should we clear the security flag on this now?