388424 - (CVE-2007-5691) Crash when decoding FTP directory items

Status: VERIFIED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Priit Laes]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en; rv:1.8.1.4) Gecko/20070617 Epiphany/2.18 Firefox/2.0.0.4
Build Identifier: 

There seems to be an out of bounds read on filenames like this: "file 2007 Vol 54 2007 by plaes"

Observed in i386, ia64, amd64 and alpha platforms.

Reproducible: Always

Steps to Reproduce:
1. Go to ftp://album:[e-mail]
2. Observe the crash




Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 47659470231072 (LWP 31907)]
0x00002b5896b3af3c in ParseFTPList (line=<value optimized out>, state=0x7fff18ea5010, result=0x7fff18ea50b0) at ParseFTPList.cpp:1039
1039              if (isdigit(*tokens[pos]) /* size */
(gdb) bt full
#0  0x00002b5896b3af3c in ParseFTPList (line=<value optimized out>, state=0x7fff18ea5010, result=0x7fff18ea50b0) at ParseFTPList.cpp:1039
        tokens = {0x2aaab107ed80 "-rw-r--r--    1 0        0", ' ' <repeats 15 times>, "0 Jul 17 13:24 file 2007 Vol 54 2007 by plaes", 
  0x2aaab107ed8e "1 0        0", ' ' <repeats 15 times>, "0 Jul 17 13:24 file 2007 Vol 54 2007 by plaes", 
  0x2aaab107ed90 "0        0", ' ' <repeats 15 times>, "0 Jul 17 13:24 file 2007 Vol 54 2007 by plaes", 
  0x2aaab107ed99 "0", ' ' <repeats 15 times>, "0 Jul 17 13:24 file 2007 Vol 54 2007 by plaes", 
  0x2aaab107eda9 "0 Jul 17 13:24 file 2007 Vol 54 2007 by plaes", 0x2aaab107edab "Jul 17 13:24 file 2007 Vol 54 2007 by plaes", 
  0x2aaab107edaf "17 13:24 file 2007 Vol 54 2007 by plaes", 0x2aaab107edb2 "13:24 file 2007 Vol 54 2007 by plaes", 
  0x2aaab107edb8 "file 2007 Vol 54 2007 by plaes", 0x2aaab107edbd "2007 Vol 54 2007 by plaes", 0x2aaab107edc2 "Vol 54 2007 by plaes", 
  0x2aaab107edc6 "54 2007 by plaes", 0x2aaab107edc9 "2007 by plaes", 0x2aaab107edce "by plaes", 0x2aaab107edd1 "plaes", 0x2b5896b908b0 "D"}
        linelen_sans_wsp = 86
        numtoks = 15
        month_num = <value optimized out>
        toklen = {10, 1, 1, 1, 1, 3, 2, 5, 4, 4, 3, 2, 4, 2, 5, 32767}
        tokmarker = 11096
        lstyle = -1324880446
        carry_buf_len = 0
        linelen = 86
        pos = <value optimized out>
        p = 0x2aaab107edc2 "Vol 54 2007 by plaes"
        month_names = 0x2b5896b90a00 "JanFebMarAprMayJunJulAugSepOctNovDec"
#1  0x00002b5896b3db2f in nsFTPDirListingConv::DigestBufferLines (this=<value optimized out>, aBuffer=<value optimized out>, 
    aString=@0x7fff18ea5220) at nsFTPDirListingConv.cpp:334
        state = {magic = 0x2b5896b39ed0, now_time = 0, now_tm = {tm_usec = 0, tm_sec = 0, tm_min = 0, tm_hour = 0, tm_mday = 0, 
    tm_month = 0, tm_year = 0, tm_wday = 0 '\0', tm_yday = 0, tm_params = {tp_gmt_offset = 0, tp_dst_offset = 0}}, lstyle = 0, 
  parsed_one = 0, carry_buf = '\0' <repeats 83 times>, carry_buf_len = 0, numlines = 1}
        type = -1324532016
        buf = {<nsFixedCString> = {<nsCString> = {<nsCSubstring> = {<nsACString_internal> = {mVTable = 0x1, 
          mData = 0x2b5891f3683a "H\203�([]A\\A]�E1���L\211��������\213\203�", mLength = 0, 
          mFlags = 0}, <No data fields>}, <No data fields>}, mFixedCapacity = 0, mFixedBuf = 0x0}, 
  mStorage = "\000\000\000\000\000\000\000\000P1\n\222X+\000\000\210Q�\030�\177\000\000\000\000\000\000\021\000\001\000?\000\000\000\000\000\000\000\210Q�\030�\177\000\000\000j�\221X+\000\000�>\r��*\000"}
        buffer = '\0' <repeats 16 times>, "\207", '\0' <repeats 47 times>, "�P\a�\t\000\000\000 \000\000��*\000\000\207\000\000\000\000\000\000\000�O�\030�\177\000\000\000P�\030�\177\000\000\034\000\000\000\000\000\000\0005\000\000\000\000\000\000\000���\222X+\000\000\177\000\000\000\000\000\000\000 R�\030�\177\000\000�O�\030�\177\000\000�\220�\221X+\000\000~\000\000\000\000\000\000\000\232\221�\221X+\000\000 R�\030�\177\000\000\034\000\000\000\000\000\000\000 R�\030�\177\000\000Q\000\000\000\000\000\000\0005\000\000\000\000\000\000\000\204\233�\221X+\000\000\000\000\000\000\000\000\000\000HR�\030�\177\000\000\021\000\001"...
        escapedDate = <value optimized out>
        result = {fe_type = 0, fe_fname = 0x0, fe_fnlen = 0, fe_lname = 0x0, fe_lnlen = 0, fe_size = '\0' <repeats 39 times>, fe_time = {
---Type <return> to continue, or q <return> to quit---
    tm_usec = 0, tm_sec = 0, tm_min = 0, tm_hour = 0, tm_mday = 0, tm_month = 0, tm_year = 0, tm_wday = 0 '\0', tm_yday = 0, tm_params = {
      tp_gmt_offset = 0, tp_dst_offset = 0}}, fe_cinfs = 0}
        line = 0x2aaab107ed80 "-rw-r--r--    1 0        0", ' ' <repeats 15 times>, "0 Jul 17 13:24 file 2007 Vol 54 2007 by plaes"
        eol = 0x20 <Address 0x20 out of bounds>
        cr = 1
#2  0x00002b5896b3e0b0 in nsFTPDirListingConv::OnDataAvailable (this=0x2aaab10d3ed0, request=<value optimized out>, ctxt=0x0, 
    inStr=0x2b5896b5801c, sourceOffset=<value optimized out>, count=<value optimized out>) at nsFTPDirListingConv.cpp:188
        rv = 0
        channel = {<nsCOMPtr_base> = {mRawPtr = 0x2aaab1113c20}, <No data fields>}
        read = 88
        streamLen = 88
        indexFormat = {<nsFixedCString> = {<nsCString> = {<nsCSubstring> = {<nsACString_internal> = {mVTable = 0x2b58920a3150, 
          mData = 0x2aaab107efa8 "300: ftp://[e-mail]/\n200: filename content-length last-modified file-type\n", mLength = 81, 
          mFlags = 65541}, <No data fields>}, <No data fields>}, mFixedCapacity = 63, 
    mFixedBuf = 0x7fff18ea5248 "300: ftp://[e-mail]/\n"}, 
  mStorage = "300: ftp://[e-mail]/\n\000*\000\000\000\000\000\000\000\000\000\000h|\002��*\000\000�R�\030�\177\000\000`S�\030�\177\000"}
        line = <value optimized out>
        inputData = {<nsCOMPtr_base> = {mRawPtr = 0x70080}, <No data fields>}
#3  0x00002b5896b58527 in DataRequestForwarder::OnDataAvailable (this=0x2aaab1113c20, request=<value optimized out>, ctxt=0x0, 
    input=0x2aaab10bc7b0, offset=<value optimized out>, count=88) at nsFtpConnectionThread.cpp:377
        rv = <value optimized out>
#4  0x00002b5896b09778 in nsInputStreamPump::OnStateTransfer (this=0x2aaab109f2e0) at nsInputStreamPump.cpp:494
        offsetBefore = <value optimized out>
        seekable = {<nsCOMPtr_base> = {mRawPtr = 0x2aaab10bc7b8}, <No data fields>}
        rv = 0
        avail = 88
#5  0x00002b5896b09894 in nsInputStreamPump::OnInputStreamReady (this=0x21, stream=<value optimized out>) at nsInputStreamPump.cpp:397
        nextState = 36
#6  0x00002b5891f36d97 in nsInputStreamReadyEvent::EventHandler (plevent=<value optimized out>) at nsStreamUtils.cpp:120
        ev = (nsInputStreamReadyEvent *) 0x24
#7  0x00002b5891f4f201 in PL_HandleEvent (self=0x21) at plevent.c:688
        result = <value optimized out>
#8  0x00002b5891f4f468 in PL_ProcessPendingEvents (self=0x5b4f10) at plevent.c:623
        event = (PLEvent *) 0x20
        count = 4
#9  0x00002b5891f50acb in nsEventQueueImpl::ProcessPendingEvents (this=0x5b34a0) at nsEventQueue.cpp:417
        correctThread = 32
---Type <return> to continue, or q <return> to quit---
#10 0x00002aaaaca579de in event_processor_callback (source=<value optimized out>, condition=G_IO_OUT, data=0x24) at nsAppShell.cpp:67
        eventQueue = (class nsIEventQueue *) 0x21
#11 0x00002b5893c113af in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
No symbol table info available.
#12 0x00002b5893c142a7 in ?? () from /usr/lib/libglib-2.0.so.0
No symbol table info available.
#13 0x00002b5893c145fc in g_main_loop_run () from /usr/lib/libglib-2.0.so.0
No symbol table info available.
#14 0x00002b58925584d8 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#15 0x00002aaaaca57d3c in nsAppShell::Run (this=0x6a0c90) at nsAppShell.cpp:139
No locals.
#16 0x00002aaaad14cd57 in nsAppStartup::Run (this=0x6a0c10) at nsAppStartup.cpp:151
        rv = <value optimized out>
#17 0x000000000040725f in XRE_main (argc=<value optimized out>, argv=<value optimized out>, aAppData=<value optimized out>)
    at nsAppRunner.cpp:2642
        remoteService = {<nsCOMPtr_base> = {mRawPtr = 0x2aaab0233240}, <No data fields>}
        rv = 0
        gtkModules = <value optimized out>
        i = <value optimized out>
        dirProvider = {<nsIDirectoryServiceProvider2> = {<nsIDirectoryServiceProvider> = {<nsISupports> = {
        _vptr.nsISupports = 0x5135f0}, <No data fields>}, <No data fields>}, <nsIProfileStartup> = {<nsISupports> = {
      _vptr.nsISupports = 0x513638}, <No data fields>}, mAppDir = {<nsCOMPtr_base> = {mRawPtr = 0x5176f0}, <No data fields>}, 
  mXULAppDir = {<nsCOMPtr_base> = {mRawPtr = 0x0}, <No data fields>}, mProfileDir = {<nsCOMPtr_base> = {
      mRawPtr = 0x548c30}, <No data fields>}, mProfileLocalDir = {<nsCOMPtr_base> = {mRawPtr = 0x548d00}, <No data fields>}, 
  mProfileNotified = 1}
        glib2 = (PRLibrary *) 0x516dc0
        nativeApp = {<nsCOMPtr_base> = {mRawPtr = 0x545e60}, <No data fields>}
        canRun = 1
        registryFile = {<nsCOMPtr_base> = {mRawPtr = 0x548b40}, <No data fields>}
        xremotearg = <value optimized out>
        ar = <value optimized out>
        profileLock = {<nsCOMPtr_base> = {mRawPtr = 0x5506b0}, <No data fields>}
        startOffline = 0
        profD = {<nsCOMPtr_base> = {mRawPtr = 0x548c30}, <No data fields>}
        profLD = {<nsCOMPtr_base> = {mRawPtr = 0x548d00}, <No data fields>}
        upgraded = <value optimized out>
        version = {<nsFixedCString> = {<nsCString> = {<nsCSubstring> = {<nsACString_internal> = {mVTable = 0x2b58920a3150, 
---Type <return> to continue, or q <return> to quit---
          mData = 0x7fff18ea57d8 "2.0.0.4_2007061721/1.8.1.4_2007061721", mLength = 37, 
          mFlags = 65553}, <No data fields>}, <No data fields>}, mFixedCapacity = 63, 
    mFixedBuf = 0x7fff18ea57d8 "2.0.0.4_2007061721/1.8.1.4_2007061721"}, 
  mStorage = "2.0.0.4_2007061721/1.8.1.4_2007061721\000\000\000�\036@\000\000\000\000\000\\\230�\221X+\000\000\001\000\000\000\000\000\000"}
        osABI = {<nsCString> = {<nsCSubstring> = {<nsACString_internal> = {mVTable = 0x2b58920a3150, mData = 0x40f978 "Linux_x86_64-gcc3", 
        mLength = 17, mFlags = 1}, <No data fields>}, <No data fields>}, <No data fields>}
        versionOK = <value optimized out>
        needsRestart = 0
        appInitiatedRestart = <value optimized out>
#18 0x00002b5892e7c533 in __libc_start_main () from /lib/libc.so.6
No symbol table info available.
#19 0x00000000004037d9 in _start ()
No symbol table info available.
(gdb) list
1034            {
1035              /* scan for: (\d+)\s+([A-Z][a-z][a-z])\s+
1036               *  (\d\d\d\d|\d\:\d\d|\d\d\:\d\d|\d\:\d\d\:\d\d|\d\d\:\d\d\:\d\d)
1037               *  \s+(.+)$
1038              */
1039              if (isdigit(*tokens[pos]) /* size */
1040                  /* (\w\w\w) */
1041               && toklen[pos+1] == 3 && isalpha(*tokens[pos+1]) &&
1042                  isalpha(tokens[pos+1][1]) && isalpha(tokens[pos+1][2])
1043                  /* (\d|\d\d) */
(gdb)

Comment 1

[Martijn Wargers (dead)]

Confirmed, also happens on trunk.

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

If the tests inside that big 'if'-statement fails ('lstyle' is set to zero)
then we should also reset 'pos' to its saved value (line 1071) so it
has the correct value when we continue the loop (line 1033).
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/netwerk/streamconv/converters/ParseFTPList.cpp&rev=1.9&root=/cvsroot&mark=1033,1071,1078,1093#1033

Also remove a couple of comments that are out of sync with the code.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 272677 [details] [diff] [review]
Patch rev. 1

I'd prefer if you updated the comments to the new loop/if-statement instead of removing it.

also please get sr=bz or darin.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Shouldn't that only happen if lstyle is 0?

Comment 5

[Mats Palmgren (inactive)]

Yes, we only need a correct 'pos' when 'lstyle' is zero, I just
didn't think it was worth checking to avoid an assignment.

Comment 6

[Boris Zbarsky [:bzbarsky]]

You mean that if lstyle is not zero we never look at |pos| again?  That's really quite non-obvious from this code, if so, and don't like correct functioning depending on non-obvious things.

Comment 7

[Mats Palmgren (inactive)]

(In reply to comment #6)
> You mean that if lstyle is not zero we never look at |pos| again?

Yes.  'pos' is used throughout this ~1600 line function as a temp loop
variable, it's generally not used beyond each loop (without being assigned
again first).  The value of 'tokmarker' *is* used beyond this loop though,
so we need to leave that as is.  We could introduce a local variable for
the inner loops instead if that makes the code clearer...

Comment 8

[Boris Zbarsky [:bzbarsky]]

I would prefer that, yes.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273066 [details] [diff] [review]
Patch rev. 2

You need to give those different names.  Or declare before the first for() loop only.  Otherwise the broken scoping on Windows will make this not compile (it'll see a redeclaration of 'i').

With that, sr=bzbarsky.  I like this a lot more.

Comment 11

[Mats Palmgren (inactive)]

(In reply to comment #10)
> ... Otherwise the broken scoping on Windows will make this not compile
> (it'll see a redeclaration of 'i').

But the 2nd for-loop is inside an if-block, do you mean there is still
some compiler having problems with that?  FWIW, I'm using MSVC++ 2005 Express
and it compiles it without warnings.  To be precise:
Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 14.00.50727.762 for 80x86

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ah, indeed.  Yeah, that probably works, though it'll trigger a redeclaration warning in some cases...

Comment 13

[Mats Palmgren (inactive)]

Fixed the potential warning and checked in to trunk.
mozilla/netwerk/streamconv/converters/ParseFTPList.cpp 	1.10

-> FIXED

Comment 14

[Daniel Veditz [:dveditz]]

Guessing at the severity ("sg:low"). Is the over-read from data the attacker controls, and could we fill it with anything "interesting" in this context (making the severity worse)?

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 273066 [details] [diff] [review]
Patch rev. 2

approved for 1.8.1.7 and 1.8.0.14, a=dveditz for release-drivers

Comment 16

[Mats Palmgren (inactive)]

MOZILLA_1_8_BRANCH
mozilla/netwerk/streamconv/converters/ParseFTPList.cpp 	1.8.18.1 
mozilla/netwerk/streamconv/converters/ParseFTPList.cpp 	1.8.18.2 

MOZILLA_1_8_0_BRANCH
mozilla/netwerk/streamconv/converters/ParseFTPList.cpp 	1.8.26.1 
mozilla/netwerk/streamconv/converters/ParseFTPList.cpp 	1.8.26.2 

(sorry, it took two commits to get it right)

Comment 17

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.7 with testing the url from comment #0

using: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.7pre) Gecko/2007090308 BonEcho/2.0.0.7pre on Fedora F7

and: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.7pre) Gecko/20070903 BonEcho/2.0.0.7pre ID:2007090304

no crash on the test url - adding verified keyword

Comment 19

[Stephen Donner [:stephend] Not actively reading bugmail]

No crash with neither Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.14pre) Gecko/20071210 Firefox/1.5.0.13pre nor Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.0.14pre) Gecko/20071210 Firefox/1.5.0.13pre when navigating to the URL in comment 0.

Replacing fixed1.8.0.14 keyword with verified1.8.0.14

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

And now, verified on trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007121107 Minefield/3.0b2pre

and

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007121108 Minefield/3.0b2pre

Comment 21

[Daniel Veditz [:dveditz]]

This appears to have been assigned CVE-2007-5691 based on Michal Bucko's advisory about his duplicate report of this issue (bug 400234)

http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2007-5691
http://www.securityfocus.com/bid/26159

Comment 22

[Priit Laes]

(In reply to comment #21)
> This appears to have been assigned CVE-2007-5691 based on Michal Bucko's
> advisory about his duplicate report of this issue (bug 400234)
> 
> http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2007-5691
> http://www.securityfocus.com/bid/26159
> 

Is the security bounty program still ongoing and does this mean that I'm eligible for 500 bucks? :)