Bug 515583 (CVE-2009-3384)

Memory fencepost / integer underflow in ParseFTPList.cpp

RESOLVED FIXED in mozilla1.9.2

Status

()

Core
Networking: FTP
P2
normal
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Michal Zalewski, Assigned: michal)

Tracking

({verified1.9.0.15, verified1.9.1})

unspecified
mozilla1.9.2
verified1.9.0.15, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.2 +
blocking1.9.0.15 +
wanted1.9.0.x +
blocking1.8.1.next +
wanted1.8.1.x +
blocking1.8.0.next ?
wanted1.8.0.x ?

Firefox Tracking Flags

(status1.9.2 beta1-fixed, blocking1.9.1 .4+, status1.9.1 .4-fixed)

Details

(Whiteboard: [sg:investigate] Coordinate advisory with webkit/apple/chrome/gvfs/libsoup)

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

8 years ago
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.
Whiteboard: [sg:investigate]
(Reporter)

Comment 1

8 years ago
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
(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. ;)
Component: Security → Networking: FTP
Product: Firefox → Core
QA Contact: firefox → networking.ftp
(Reporter)

Comment 3

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

Updated

8 years ago
Assignee: nobody → michal.novotny
I would fully support taking out and shooting ParseFTPList (or maybe all of FTP support)... :(
(Assignee)

Comment 5

8 years ago
Created attachment 400465 [details] [diff] [review]
patch 1 - fix for CMU/VMS-IP FTP style listing
Attachment #400465 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

8 years ago
Created attachment 400469 [details] [diff] [review]
patch 1 v2 - added unittest

I forgot to add the test.
Attachment #400465 - Attachment is obsolete: true
Attachment #400469 - Flags: review?(bzbarsky)
Attachment #400465 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

8 years ago
(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.
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?
(Reporter)

Comment 9

8 years ago
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.
(Reporter)

Comment 10

8 years ago
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.
(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
(Reporter)

Comment 12

8 years ago
D'oh, I somehow missed the "raw file" link on the right.
(Reporter)

Comment 13

8 years ago
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++)
(Reporter)

Comment 14

8 years ago
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.
(Reporter)

Comment 15

8 years ago
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.
(Reporter)

Comment 16

8 years ago
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.
(Assignee)

Comment 17

8 years ago
Created attachment 400768 [details] [diff] [review]
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.
Attachment #400469 - Attachment is obsolete: true
Attachment #400768 - Flags: review?(doug.turner)
Attachment #400469 - Flags: review?(bzbarsky)
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 );
Attachment #400768 - Flags: review?(doug.turner) → review-
(Reporter)

Comment 19

8 years ago
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 on attachment 400768 [details] [diff] [review]
patch 1 v3

and the assertion?
Attachment #400768 - Flags: review- → review+
(Assignee)

Comment 21

8 years ago
Maybe instead of assertion we should skip processing the input like in case when linelen == 0.
(Assignee)

Comment 22

8 years ago
Created attachment 401002 [details] [diff] [review]
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.
Attachment #400768 - Attachment is obsolete: true
Attachment #401002 - Flags: review?(doug.turner)
blocking1.9.1: --- → ?
Flags: blocking1.9.2?
Flags: blocking1.9.0.16?
Flags: blocking1.9.0.15?
status1.9.1: --- → wanted
Flags: blocking1.9.0.15? → wanted1.9.0.x+
(Reporter)

Comment 23

8 years ago
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

8 years ago
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.
Attachment #401002 - Flags: review+
(Assignee)

Comment 25

8 years ago
Created attachment 401958 [details] [diff] [review]
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
Attachment #401002 - Attachment is obsolete: true
Attachment #401958 - Flags: superreview?(cbiesinger)
Attachment #401002 - Flags: review?(doug.turner)

Comment 26

8 years ago
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.
blocking1.9.1: ? → .5+
Flags: blocking1.9.0.16? → blocking1.9.0.16+
Since we have two r=s and only lack sr=, let's see if we can make this train of releases...
blocking1.9.1: .5+ → ?
Flags: blocking1.9.0.15?
(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. :(
(Assignee)

Comment 29

8 years ago
(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?
IMHO this doesn't need sr.  This is not an architectural change.
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

8 years ago
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).
Attachment #401958 - Flags: superreview?(cbiesinger) → superreview+
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 == ']')?
This SpiderMonkey hacker urges others to get over outsize fears of forward goto when used for unified error/return handling.  :-)
gotos for error handling are used all over the place in the linux kernel, too--it's like a thrifty version of C++ exceptions.
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.
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?
blocking1.9.1: ? → .4+
Flags: blocking1.9.0.15? → blocking1.9.0.15+
Attachment #401958 - Flags: approval1.9.2?
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?
Status: NEW → ASSIGNED
(Assignee)

Comment 39

8 years ago
Created attachment 402232 [details] [diff] [review]
patch 1 v6

I've added the assertion and created a new function for the common code.
Attachment #401958 - Attachment is obsolete: true
Attachment #401958 - Flags: approval1.9.2?
(Assignee)

Comment 40

8 years ago
(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 on attachment 402232 [details] [diff] [review]
patch 1 v6

Michal, can you land this on trunk, or do you need assistance?
Attachment #402232 - Flags: approval1.9.2?
Attachment #402232 - Flags: approval1.9.1.4?
Attachment #402232 - Flags: approval1.9.0.15?
(Assignee)

Comment 42

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

Updated

8 years ago
Flags: blocking1.9.2? → blocking1.9.2+
Comment on attachment 402232 [details] [diff] [review]
patch 1 v6

Now that this is a blocker, please feel free to land asap.
Attachment #402232 - Flags: approval1.9.2?

Updated

8 years ago
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2
Flags: blocking1.9.0.16+
Comment on attachment 402232 [details] [diff] [review]
patch 1 v6

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz
Attachment #402232 - Flags: approval1.9.1.4?
Attachment #402232 - Flags: approval1.9.1.4+
Attachment #402232 - Flags: approval1.9.0.15?
Attachment #402232 - Flags: approval1.9.0.15+
http://hg.mozilla.org/mozilla-central/rev/98330c8132a9
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 46

8 years ago
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.
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
status1.9.1: wanted → .4-fixed
status1.9.2: --- → beta1-fixed
Keywords: fixed1.9.0.15
(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
Created attachment 402272 [details] [diff] [review]
what was landed
Attachment #402232 - Attachment is obsolete: true
(Assignee)

Comment 50

8 years ago
(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,,)
Whiteboard: [sg:investigate] → [sg:investigate] Coordinate advisory with webkit/apple/chrome

Comment 51

8 years ago
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?
cc'ing a few Red Hat people to help with coordination.
Whiteboard: [sg:investigate] Coordinate advisory with webkit/apple/chrome → [sg:investigate] Coordinate advisory with webkit/apple/chrome/gvfs/libsoup
Is it useful to use vendor-sec or oCERT or some other group for this type of coordination?
Verified for 1.9.0 and 1.9.1 using passing unit test since there is no manual repro steps or method.
Keywords: fixed1.9.0.15 → verified1.9.0.15, verified1.9.1
(Reporter)

Comment 55

8 years ago
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

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

Comment 57

8 years ago
I'm not releasing any advisory (that I am aware of :-).
I'll send email to oCERT to help us with multi-vendor coordination.

Comment 59

8 years ago
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
Alias: CVE-2009-3384
Flags: wanted1.8.1.x?
Flags: wanted1.8.0.x?
Flags: blocking1.8.1.next?
Flags: blocking1.8.0.next?
Has somebody contacted the maintainers of gvfs and libsoup about this issue?

Comment 61

8 years ago
I'm adding the maintainer of the gvfs code to the CC list. libsoup doesn't yet contain this code.
(Reporter)

Comment 62

8 years ago
FWIW, I think this is now public on Apple end.
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next?
Flags: blocking1.8.1.next+

Comment 63

7 years ago
This was never made public. Are there plans to do so?
(Reporter)

Comment 64

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

Comment 65

6 years ago
Since this is already fixed, would it make sense to open this bug?
Group: core-security
You need to log in before you can comment on or make changes to this bug.