515583 - (CVE-2009-3384) Memory fencepost / integer underflow in ParseFTPList.cpp

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect, P2)

Description

[Michal Zalewski]

Hi all,

The code in ParseFTPList.cpp (the parser for directory listing) is horrible and should be taken out and shot. I tried fuzzing it a bit today, and stumbled upon a memory fencepost pretty much right away:

input = "[RWCEM1 4 5-MAR-1993 18:09:01.12\r\n"

The crash occurs with SUPPORT_VMS code, namely at this obviously bad line:

436                 while (*p != ']')

This is a read fencepost, but the attacker may pad the memory to avoid crashing immediately by eventually providing ']' past the end of currently parsed string; in this case, if I understand correctly, this will cause unsigned int 'pos' to wrap up around 0 and start counting down from 0xffffffff before eventually exiting the aforementioned while loop; this nonsensical value would then be populated into result->fe_fnlen. 

Whether this is exploitable for anything other than a crash would depend on where that fe_fnlen is passed outside ParseFTPList, and whether all functions down the road properly account for integer overflows. No idea at this point.

Please note that Mozilla's original code is used in a couple of other browsers and FTP clients, so please keep this bug private regardless of the immediate outcome with Firefox.

Comment 1

[Michal Zalewski]

Another crash, SUPPORT_DOS:

$ perl -e '{print "\r"x12,"10-23-00  01:27PM       <x>       S  v"}' | ./parseftp -

#0  0x0804a8b2 in ParseFTPList (line=0xffffd32c '\r' <repeats 12 times>, "10-23-00  01:27PM       <x>       S  v", state=0xffffd52c, result=0xffffd248)
    at ParseFTPList.cpp:809
809                   if (p[0] == ' ' && p[3] == ' ' && p[2] == '>' &&

Not exactly sure what went wrong here, but parser state looks pretty messed up:

(gdb) printf "%d\n%d\n", pos, result->fe_fnlen
-7346
-8

/mz

Comment 2

[Reed Loden [:reed]]

(In reply to comment #0)
> Please note that Mozilla's original code is used in a couple of other browsers
> and FTP clients, so please keep this bug private regardless of the immediate
> outcome with Firefox.

Such as Chromium, as per http://blog.chromium.org/2009/09/new-ftp-implementation-goes-live.html. ;)

Comment 3

[Michal Zalewski]

Yeah, I think it's also in WebKit as such (FTPDirectoryParser), and quite a few other projects, so....

Comment 4

[Boris Zbarsky [:bzbarsky]]

I would fully support taking out and shooting ParseFTPList (or maybe all of FTP support)... :(

Comment 5

[Michal Novotny [:michal]]

Attachment: patch 1 - fix for CMU/VMS-IP FTP style listing

Comment 6

[Michal Novotny [:michal]]

Attachment: patch 1 v2 - added unittest

I forgot to add the test.

Comment 7

[Michal Novotny [:michal]]

(In reply to comment #1)
> #0  0x0804a8b2 in ParseFTPList (line=0xffffd32c '\r' <repeats 12 times>,
> "10-23-00  01:27PM       <x>       S  v", state=0xffffd52c, result=0xffffd248)
>     at ParseFTPList.cpp:809
> 809                   if (p[0] == ' ' && p[3] == ' ' && p[2] == '>' &&

I can't reproduce this crash and I can't see anything wrong while debugging the code. What revision of the file do you use? I have different line numbers in the hg trunk.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So... given that I don't know this code at all (would dougt be a better reviewer?), why is it ok to check for ']' before checking that we're still within toklength?

Comment 9

[Michal Zalewski]

You might try perl -e '{print "\r"x128,"10-23-00  01:27PM       <x>       S  v"}' instead... if that does not work, I'll investigate later today.

Comment 10

[Michal Zalewski]

I just grabbed ParseFTPList.cpp from http://ftp.mozilla.org/pub/mozilla.org/mozilla/nightly/latest-trunk/mozilla-source.tar.gz, there indeed were some minor tweaks, but the PoC still crashes for me with x128. 

I have some tweaks to restart state to dump listing so far and restart parser on !(random() % 10), but that shouldn't be it.

Comment 11

[Reed Loden [:reed]]

(In reply to comment #10)
> I just grabbed ParseFTPList.cpp from
> http://ftp.mozilla.org/pub/mozilla.org/mozilla/nightly/latest-trunk/mozilla-source.tar.gz

In the future, you can use mxr to look at source code directly.

http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/ParseFTPList.cpp

Comment 12

[Michal Zalewski]

D'oh, I somehow missed the "raw file" link on the right.

Comment 13

[Michal Zalewski]

Continuing fuzzing with top of the trunk:

3) Memory fencepost with bad month name:

perl -e '{print "----------   1 owner    group         1803128 Zul 10 10:18 X\r\n"}' | ./parseftp -

This particular gem is because this outer loop:

        for (pos = (numtoks-5); !lstyle && pos > 1; pos--)

...uses the same 'pos' as an inner loop inside used to check for month numbers:

              for (pos = 0;pos < (12*3); pos+=3)

...except that the month loop uses it to index a wholly different string, and results in a grossly out of bounds index to tokens[pos] later on. This looks pretty bad.

4) Endless loop denial of service in SUPPORT_LSL with non-numeric file sizes:

perl -e '{print "----------   X X    X         0000X0 Jul 10 10:00 X\r\n"}' | ./parseftp -

This is again becuase inner loops reuse the same index as the aforementioned outer loop; in this case, inner loop fast-forwards 'pos' to the end of a date, then the outer loop rewinds it back, until forever:

            for (pos = 0; lstyle && pos < toklen[tokmarker]; pos++)

Comment 14

[Michal Zalewski]

Err, never mind that last two, seems to be fixed on mxr, looks like  http://ftp.mozilla.org has a really old copy... my bad.

Comment 15

[Michal Zalewski]

Continuing on the real trunk... this is in SUPPORT_VMS again, somewhat similar to issue #2, maybe the same thing:

Issue 3) $ perl -e '{print "\r\r\r\n"}' | ./parseftp -
Segmentation fault (core dumped)
290           else if ((tokens[0][toklen[0]-1]) != ';')

toklen[0] seems to be nonsensical. This crashes reliably for me with the same binary on two AMD Opteron machines, but not on a third Intel Core2 machine with an older Linux distro. Which is odd, but maybe it depends on memory alignment or somesuch. Sorry for not investigating a bit more - maybe on Monday.

Comment 16

[Michal Zalewski]

Added some printf()s. Even on the machine where it does not crash, tokens[0] seems to point to garbage, and toklen[0] is 0, which would wrap to -1 in that line. So looks like this could go very wrong.

Comment 17

[Michal Novotny [:michal]]

Attachment: patch 1 v3

(In reply to comment #8)
> So... given that I don't know this code at all (would dougt be a better
> reviewer?), why is it ok to check for ']' before checking that we're still
> within toklength?

I've changed this to be more clean.
Patch also contains: 

- fix for crash in comment #15
- check for empty filename in VMS listing
- more tests

Boris, I've asked Doug for review but feel free to do the review if you want.

Comment 18

[Doug Turner (:dougt)]

Comment on attachment 400768 [details] [diff] [review]
patch 1 v3

Do you want to add an assertion if numtoks is zero?

How does this work?

-  rc = ParseFTPLIST( line, state, &result );
+  rc = ParseFTPList( line, state, &result );

Comment 19

[Michal Zalewski]

I think the original code that calls ParseFTPLIST never worked, but it's #if-ed out unless you want to do standalone testing, so nobody noticed?

Comment 20

[Doug Turner (:dougt)]

Comment on attachment 400768 [details] [diff] [review]
patch 1 v3

and the assertion?

Comment 21

[Michal Novotny [:michal]]

Maybe instead of assertion we should skip processing the input like in case when linelen == 0.

Comment 22

[Michal Novotny [:michal]]

Attachment: patch 1 v4

I've changed it so that the line isn't processed at all when no token is found. For example computing of linelen_sans_wsp was wrong in this case. I'm just not sure if we shouldn't strip leading '\r' at http://hg.mozilla.org/mozilla-central/annotate/1904598c4caf/netwerk/streamconv/converters/ParseFTPList.cpp#l73 since we treat '\r' as white space when searching for tokens. That would solve the problem too.

Comment 23

[Michal Zalewski]

Just FYI, I'm not seeing further crashes with patch v1.4. I opened a bug with WebKit to incorporate your fixes:

https://bugs.webkit.org/show_bug.cgi?id=29294

...and asked them if they want to coordinate, but no response yet. Will keep you posted.

Comment 24

[Wan-Teh Chang]

Comment on attachment 401002 [details] [diff] [review]
patch 1 v4

r=wtc.

>@@ -351,21 +354,25 @@ int ParseFTPList(const char *line, struc
>             while (lstyle && pos < toklen[0] && *p != ']')
>             {
>               if (*p != '$' && *p != '.' && *p != '_' && *p != '-' &&
>                   *p != '~' && !isdigit(*p) && !isalpha(*p))              
>                 lstyle = 0;
>               pos++;
>               p++;
>             }
>-            if (lstyle && pos < (toklen[0]-1) && *p == ']')
>+            if (lstyle && pos < (toklen[0]-1))
>             {
>+              /* ']' was found and there is at least one character after it */
>               pos++;
>               p++;
>               tokmarker = pos; /* length of leading "[DIR1.DIR2.etc]" */
>+            } else {

Just wanted to make sure I understand this change.  We can
remove the *p == ']' test because the fact that we exited
the while loop and that *p == ']' is true, correct?

>-          if (!lstyle || pos > 80) /* VMS filenames can't be longer than that */
>+          if (!lstyle || pos > 80 || !pos)
>           {
>+            /* VMS filenames can't be longer than that */
>             lstyle = 0;
>           }

Style nit: please write this if conditional like this:

  if (!lstyle || pos == 0 || pos > 80) /* VMS filenames can't be longer than that */

pos == 0 rather than !pos is the prevalent style used in
this file, and we should keep the comment right next to
the pos > 80 test.

Comment 25

[Michal Novotny [:michal]]

Attachment: patch 1 v5

(In reply to comment #24)
> Just wanted to make sure I understand this change.  We can
> remove the *p == ']' test because the fact that we exited
> the while loop and that *p == ']' is true, correct?

Yes, exactly.

> Style nit: please write this if conditional like this:
> 
>   if (!lstyle || pos == 0 || pos > 80) /* VMS filenames can't be longer than
> that */
> 

fixed

Comment 26

[Wan-Teh Chang]

Comment on attachment 401958 [details] [diff] [review]
patch 1 v5

>+    if (!numtoks)
>+      return (state->parsed_one || state->lstyle) ? '?' : '"';

The return statement duplicates the following code at the end
of the ParseFTPList function:

  if (state->parsed_one || state->lstyle) /* junk if we fail to parse */
    return '?';      /* this time but had previously parsed successfully */
  return '"';        /* its part of a comment or error message */

I wonder if we can avoid the code duplication.  A quick and dirty
fix is to use a goto statement to jump to the end of the function.
But I'm sure some people will find goto distasteful.

We can restructure the beginning of the function so that it looks
like this:

  if (linelen > 0)
  {
    /* parse line into tokens */
    ...
  }
  if (numtoks)
  {
    linelen_sans_wsp = &(tokens[numtoks-1][toklen[numtoks-1]]) - tokens[0];
    if (numtoks == (sizeof(tokens)/sizeof(tokens[0])) )
    ...
 
But this requires a lot of changes.

Perhaps we can just create a small function for the common code.

Comment 27

[Reed Loden [:reed]]

Since we have two r=s and only lack sr=, let's see if we can make this train of releases...

Comment 28

[Reed Loden [:reed]]

(In reply to comment #27)
> Since we have two r=s and only lack sr=, let's see if we can make this train of
> releases...

... else we're going to be the last ones to fix it rather than the first. :(

Comment 29

[Michal Novotny [:michal]]

(In reply to comment #26)

I considered all mentioned options and I've chosen code duplication as the least evil.

What about define instead of function?

Comment 30

[Jason Duell]

IMHO this doesn't need sr.  This is not an architectural change.

Comment 31

[Mats Palmgren (inactive)]

As a security bug, per our new policy, the patch needs explicit super-review
from a second person. http://www.mozilla.org/hacking/reviewers.html

Comment 32

[Wan-Teh Chang]

Comment on attachment 401958 [details] [diff] [review]
patch 1 v5

>+    if (!numtoks)
>+      return (state->parsed_one || state->lstyle) ? '?' : '"';

The reason I pointed out this duplicate code is that I had a
hard time understanding the return statement and vouching for
its correctness.  Fortunately I found that it is the same as
the last three lines at the end of the function.  From there,
I concluded that the code you added merely treats !numtoks
the same way as linelen == 0, which makes sense.

We should help a future maintainer link these two pieces of
code.  Using a goto statement is one option (I'd do that).
Adding a comment, or even duplicating the comment at the
end of the function is another option.  Addig a macro is
fine, too.

I found an unrelated problem: if linelen is 0, the current
code will skip VMS long filename carryover buffer
(state->carry_buf).

Comment 33

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

Comment on attachment 401958 [details] [diff] [review]
patch 1 v5

+              /* ']' was found and there is at least one character after it */

can you add an NS_ASSERTION(*p == ']')?

Comment 34

[Jeff Walden [:Waldo]]

This SpiderMonkey hacker urges others to get over outsize fears of forward goto when used for unified error/return handling.  :-)

Comment 35

[Jason Duell]

gotos for error handling are used all over the place in the linux kernel, too--it's like a thrifty version of C++ exceptions.

Comment 36

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

I tend to think that a better way for unified error handling is moving code into its own function, but I guess that kind of refactoring for this code would kind of suck.

Comment 37

[Daniel Veditz [:dveditz]]

If this fix could make 1.9.1.4 that'd be great. Can we land this on mozilla-central ASAP and get approval for 1.9.2?

Comment 38

[Reed Loden [:reed]]

So, I see three issues remaining:
1. Fixing the skipping issue found at the bottom of comment #32.
2. Adding the assertion mentioned in comment #33.
3. Adding a goto/macro/function/comment/whatever to deal with duplicated code.

Is that correct?

Comment 39

[Michal Novotny [:michal]]

Attachment: patch 1 v6

I've added the assertion and created a new function for the common code.

Comment 40

[Michal Novotny [:michal]]

(In reply to comment #32)
> I found an unrelated problem: if linelen is 0, the current
> code will skip VMS long filename carryover buffer
> (state->carry_buf).

But this is correct behaviour, isn't it? If linelen is 0 then the previous line wasn't part of a multiline listing and was correctly treated as junk.

Comment 41

[Reed Loden [:reed]]

Comment on attachment 402232 [details] [diff] [review]
patch 1 v6

Michal, can you land this on trunk, or do you need assistance?

Comment 42

[Michal Novotny [:michal]]

I need assistance. I've intended to add the checkin-needed keyword if there are no further comments on the last patch.

Comment 43

[Johnny Stenback (:jst)]

Comment on attachment 402232 [details] [diff] [review]
patch 1 v6

Now that this is a blocker, please feel free to land asap.

Comment 44

[Daniel Veditz [:dveditz]]

Comment on attachment 402232 [details] [diff] [review]
patch 1 v6

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz

Comment 45

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/98330c8132a9

Comment 46

[Wan-Teh Chang]

Comment on attachment 402232 [details] [diff] [review]
patch 1 v6

>+static inline int ParseFTPListDetermineRetval(struct list_state *state)

Would be nice to name this function ParsingFailed,
which describes when we call this function.

Re: comment 40: Michal, I think you're right.  You're
saying that an incomplete VMS long filename should be
treated as junk, right?  In any case, that issue is
unrelated to this security bug.

Comment 47

[Reed Loden [:reed]]

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fd9b730fe69d

Checking in netwerk/streamconv/converters/ParseFTPList.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/ParseFTPList.cpp,v  <--  ParseFTPList.cpp
new revision: 1.11; previous revision: 1.10
done
RCS file: /cvsroot/mozilla/netwerk/test/unit/test_bug515583.js,v
done
Checking in netwerk/test/unit/test_bug515583.js;
/cvsroot/mozilla/netwerk/test/unit/test_bug515583.js,v  <--  test_bug515583.js
initial revision: 1.1
done

Comment 48

[Reed Loden [:reed]]

(In reply to comment #46)
> (From update of attachment 402232 [details] [diff] [review])
> >+static inline int ParseFTPListDetermineRetval(struct list_state *state)
> 
> Would be nice to name this function ParsingFailed,
> which describes when we call this function.

Fixed in http://hg.mozilla.org/mozilla-central/rev/cade5b705114

Comment 49

[Reed Loden [:reed]]

Attachment: what was landed

Comment 50

[Michal Novotny [:michal]]

(In reply to comment #46)
> You're saying that an incomplete VMS long filename 
> should be treated as junk, right?

Yes, exactly. Current code will parse following malformed input as tree lines of junk, which is IMHO correct.

ANOTHER-LONG-VMS-FILENAME.WITH-LF-TO-NEXT-LINE

                   213  29-JAN-1996 03:33 [ANONYMOU,ANONYMOUS] (RWED,RWED,,)

Comment 51

[Wan-Teh Chang]

dveditz: ParseFTPList is also used in gvfs and libsoup.
We should send them the patch.  Perhaps we can contact
them through Fedora/Red Hat's security response team?

Comment 52

[Reed Loden [:reed]]

cc'ing a few Red Hat people to help with coordination.

Comment 53

[Reed Loden [:reed]]

Is it useful to use vendor-sec or oCERT or some other group for this type of coordination?

Comment 54

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0 and 1.9.1 using passing unit test since there is no manual repro steps or method.

Comment 55

[Michal Zalewski]

FYI, I checked on webkit-security (which includes Apple, Palm, etc). Looks like they have the fixes landed on trunk already, and there seems to be no specific desire to coordinate releases or advisories past this point. 

I think this covers most of the high-profile users, so not sure if there's a significant value in bringing vendor-sec or oCERT to the table.

Comment 56

[aaron]

Apple has specifically asked that the details of this issue (the advisory) be embargoed until November.

Comment 57

[Michal Zalewski]

I'm not releasing any advisory (that I am aware of :-).

Comment 58

[Brandon Sterne (:bsterne)]

I'll send email to oCERT to help us with multi-vendor coordination.

Comment 59

[lcars]

webkit-security should cover all vendors which include webkit code in their own product. I checked our vendor database and I don't see additional vendors that should be warned at this time as they are all covered by either vendor-sec or webkit-security.

If vendor-sec has been already contacted (which I don't know as I have no visibility there) then I don't think oCERT can provide additional help assuming you are coordinating with vendor-sec embargoes and updates.

Though if you need any additional help with specific vendors let me know, I'm glad to help if I can.

Cheers

Comment 60

[Reed Loden [:reed]]

Has somebody contacted the maintainers of gvfs and libsoup about this issue?

Comment 61

[Josh Bressers]

I'm adding the maintainer of the gvfs code to the CC list. libsoup doesn't yet contain this code.

Comment 62

[Michal Zalewski]

FWIW, I think this is now public on Apple end.

Comment 63

[Josh Bressers]

This was never made public. Are there plans to do so?

Comment 64

[Michal Zalewski]

I think there is no harm in doing so at this point. WebKit fixes shipped as well.

Comment 65

[Huzaifa Sidhpurwala]

Since this is already fixed, would it make sense to open this bug?