1478950 - (FTPDirListConv) Assertion failure: CheckCapacity(aLength) (String is too large.), at xpcom/string/nsTSubstring.h:876

Status: RESOLVED FIXED

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

Description

[u473386]

Attachment: bug

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.139 Safari/537.36

Steps to reproduce:

This was found by the FTPDirListConv fuzzing target (WIP). Minimal test case attached.

Assertion failure: CheckCapacity(aLength) (String is too large.), at mozilla-central/xpcom/string/nsTSubstring.h:876

==4722==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f94df0fdc15 bp 0x7ffc6a7bd6b0 sp 0x7ffc6a7bd6a0 T0)
==4722==The signal is caused by a WRITE memory access.
==4722==Hint: address points to the zero page.
    #0 0x7f94df0fdc14 in nsTSubstring<char>::nsTSubstring(char*, unsigned int, mozilla::detail::StringDataFlags, mozilla::detail::StringClassFlags) xpcom/string/nsTSubstring.h:876:5
    #1 0x7f94df1127c8 in nsTDependentSubstring<char>::nsTDependentSubstring(char const*, char const*) xpcom/string/nsTDependentSubstring.cpp:57:5
    #2 0x7f94df115d75 in nsTDependentSubstring<char> const Substring<char>(char const*, char const*) xpcom/string/nsTDependentSubstring.cpp:92:10
    #3 0x7f94dfaad74c in nsFTPDirListingConv::DigestBufferLines(char*, nsTString<char>&) netwerk/streamconv/converters/nsFTPDirListingConv.cpp:270:37
    #4 0x7f94dfaac62c in nsFTPDirListingConv::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) netwerk/streamconv/converters/nsFTPDirListingConv.cpp:131:12
    #5 0x7f94e94c1c02 in FuzzingRunFTPConv(nsCOMPtr<nsIInputStream>) netwerk/streamconv/converters/fuzztest/FTPDirListConv_fuzzer.cpp:64:9

The problem appears to be that bogus result.fe_fnlen is set in ParseFTPList.

Comment 2

[Christian Holler (:decoder)]

Valentin, can you help looking into this or suggest someone who can? Thanks!

Comment 3

[Kershaw Chang [:kershaw]]

I can take a look at this.
But I don't know how to reproduce this due to the missing of FTPDirListConv_fuzzer.cpp.

@pdknsk, could you let me know where to find this file and how to run this test case?

Thanks.

Comment 4

[u473386]

It's still WIP. I have CC'd you to the other bug, which has it in patch FTPDirListConv-3.

Comment 5

[Kershaw Chang [:kershaw]]

Sorry that I don't have a linux enviornment right now.
It will take more time to investigate this bug for me.

Comment 6

[u473386]

I think it might be difficult to track down why fe_fnlen is set incorrectly in ParseFTPList. I propose a more general fix that checks fe_fnlen for bogus values, and skips it. It's the file name length of a single directory listing line. The bug is that it tries to pre-allocate a bogus value for the output string, I think. I suggest 1MB as a generous limit, or 32K as a resonable limit.

Comment 7

[Kershaw Chang [:kershaw]]

Attachment: Bug 1478950 - Add a max limit to FTP list line

As suggested by pdknsk, add 32k limit to list line.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8998771 [details]
Bug 1478950 - Add a max limit to FTP list line

Valentin Gosu [:valentin] has approved the revision.

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: Bug 1478950 - Add a max limit to FTP list line, r=valentin

Carry reviewer's name.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

There is no need to upload a new version of the patch if only the reviewer gets added. That's something the sheriffs can do.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3b637d1359222dffc41dd9ba4d5b56416e2cce1

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f3b637d13592

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please request Beta and ESR60 approval when you get a chance. Also, this needs a sec rating.

Comment 13

[Kershaw Chang [:kershaw]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Please request Beta and ESR60 approval when you get a chance. Also, this
> needs a sec rating.

I'd like to decide the sec rating first. After that I'll ask for sec-approval and uplift.

Comment 14

[Kershaw Chang [:kershaw]]

@decoder, please correct me if I am wrong. I think this bug's sec rating is low, right?

Thanks.

Comment 15

[Christian Holler (:decoder)]

Essentially this bug is sec-none, the crash cannot be exploited in any way. The reason why we have marked this as s-s is to not point to ongoing fuzzing operations in the linked bug. Marked it as sec-other for now.

Comment 16

[Kershaw Chang [:kershaw]]

(In reply to Christian Holler (:decoder) from comment #15)
> Essentially this bug is sec-none, the crash cannot be exploited in any way.
> The reason why we have marked this as s-s is to not point to ongoing fuzzing
> operations in the linked bug. Marked it as sec-other for now.

Thanks.

Ryan, if you agree, I think we don't have to uplift this one.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Agreed, sounds reasonable.

Comment 18

[u473386]

I'm afraid the bug has been fixed incorrectly. The problem is not the LIST line length exceeding a certain value (the test case is only 37 bytes). It's rather that ParseFTPList parses the LIST line somehow incorrectly and gets a bogus file name length (for a file name entry in that LIST line).

https://dxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/nsFTPDirListingConv.cpp#271

It then tries to allocate that bogus length, which fails. I added some debug code and the value of result.fe_fnlen in this case is 4294967294. This makes me suspect an unsigned underflow bug in ParseFTPList.

Note there may be an actual security bug here. Substring takes start/end pointers. It's just that CheckCapacity is called before it tries to read 4GB past the pointer. I think a good fix might be to check if the end pointer is still within the line.

Comment 19

[u473386]

I'm thinking code like this.

if (result.fe_fname + result.fe_fnlen > eol) {
  // fail parsing or just skip the single bogus line
}

Comment 20

[u473386]

(Skipping the line probably confuses the aString mechanism, which I don't know how it's supposed to work.)

Comment 21

[Kershaw Chang [:kershaw]]

@pdknsk, it looks like you are the perfect person to fix this, since you already know where the problem is and understand how to verify it.

Do you mind if I re-assign this bug to you? If you don't have enough cycles, I can still work on this.

Comment 22

[u473386]

Sure, but it might take a while.

Comment 23

[Kershaw Chang [:kershaw]]

(In reply to pdknsk from comment #22)
> Sure, but it might take a while.

Thanks.

Comment 24

[:Gijs (he/him)]

Should the original fix here be backed out?

Comment 25

[Kershaw Chang [:kershaw]]

(In reply to :Gijs (he/him) from comment #24)
> Should the original fix here be backed out?

I think we don't have to back it out, since it's reasonable to add a limit check.

Comment 26

[u473386]

Attachment: ftp-patch

I think this should definitely be classified as a security bug, even though the attached earlier test case isn't (but only because it gets lucky). I'll explain.

The function parses an FTP listing, and converts it into a different format for display. Each line is fed to ParseFTPLine, which then returns a pointer to the start of the file name (fe_fname) within that line and the length of the file name (fe_fnlen). If fe_fnlen is bogus (4294967294 with the test case) it over-reads past the line into the output string. With the attached test case CheckCapacity asserts that the output string cannot exceed 2GB, so it fails before. For a security bug the fuzzer needs to produce a test case that causes bogus fe_fnlen of less than 2GB. I think that chance is (or was) good.

Comment 27

[:Gijs (he/him)]

Comment on attachment 9003704 [details] [diff] [review]
ftp-patch

Setting a reviewer so this doesn't get lost. 

:decoder, do you want to adjust the severity given comment 26?

Comment 28

[Christian Holler (:decoder)]

I think we need a better root cause analysis here.

I've investigated this a bit and the bug seems to be related to OS/2 server support. The buggy fe_fnlen value is set here:

https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/netwerk/streamconv/converters/ParseFTPList.cpp#914


It is the result of a pointer subtraction with -2 as the result. In this case, the filename (fe_fname) has string length 0.

> DEBUG: fe_fname is 0x604000051734 with length 0
> DEBUG: &(line[linelen_sans_wsp])) is 0x604000051732

If the difference here was controllable (e.g. not fixed to -2) and could be forced to wrap around so much that we don't hit the 2GB limit check, then this would cause an over-read later when constructing the substring.

In order to determine if this is really exploitable, we would have to understand this code better and figure out if the difference here is a fixed value. I have some fuzzing running that tries to find other examples of this bug with a different difference right now, but so far no results.

Moving this needinfo to a Necko peer. Valentin: This code needs some investigating and I don't feel like fixing symptoms around it if I don't really understand the code. The question is, do we have anyone that really understands this? Should we send someone over to Google to drag dougt back here and fix it? ;P Alternatively, we could remove this code entirely if it isn't required anymore (at least the OS/2 support). It certainly looks scary enough.

Comment 29

[u473386]

Another way how this could become a security bug is if at some point the string limit is raised to 4GB. I think a general catch-all check is perhaps not so bad, considering the maze that is ParseFTPList. On the user side, that (most likely not in a valid format) FTP line is missing in the output.

Comment 30

[Valentin Gosu [:valentin] (he/him)]

(In reply to Christian Holler (:decoder) from comment #28)
> Moving this needinfo to a Necko peer. Valentin: This code needs some
> investigating and I don't feel like fixing symptoms around it if I don't
> really understand the code. The question is, do we have anyone that really
> understands this?

I don't think we have anyone who already knows how that code is working. In the past we fixed issues on a case-by-case basis, and that seemed to work. I really hope we get to a place where we can remove FTP support entirely.

> Should we send someone over to Google to drag dougt back here and fix it? ;P

I didn't know that was an option. YES! =)

> Alternatively, we could remove this code entirely if it
> isn't required anymore (at least the OS/2 support). It certainly looks scary
> enough.

I am OK with removing OS/2 support.

Comment 31

[Andrew McCreight [:mccr8]]

Keep in mind we do have gtest support for testing FTP parsing stuff, that I added in bug 1402151.

Comment 32

[Christian Holler (:decoder)]

I think what we need to move forward here is someone (or data) that can tell us which of these FTP implementations we need and which we don't.


I am strongly against disabling the entire FTP functionality because it is important to the open source community. But we support certain odd formats in there that *should* have no relevancy anymore nowdays, e.g.:


* Virtual Machine/Conversational Monitor System (IBM Mainframe)
* OS/2
* 16-bit Windows (e.g. Windows 3.1)


I am not sure what a good way would be to move forward. Here are some ideas:

* We could add Telemetry to figure out if anyone actually uses any of these formats (and I would doubt it).
* We could disable some of these at runtime and hide them behind a pref (mitigates security risks but still allows people that really need it to use it).
* An entirely different approach would be to look at Chrome's rewritten FTP code and import that instead.
* If we had the time and resources, rewrite this in Rust \o/


Forwarding this to Dragana to get some input on this.

Comment 33

[u473386]

I think the format names may not necessarily reflect the system, but perhaps only the origin. Chrome also supports VMS.

https://cs.chromium.org/chromium/src/net/ftp/ftp_directory_listing_parser_vms.cc

Comment 34

[Christian Holler (:decoder)]

(In reply to pdknsk from comment #33)
> I think the format names may not necessarily reflect the system, but perhaps
> only the origin. Chrome also supports VMS.
> 
> https://cs.chromium.org/chromium/src/net/ftp/
> ftp_directory_listing_parser_vms.cc

Hm, if this is used somewhere else as well, then we might need to keep the code around. Maybe we could import Google's code, it looks a lot saner than ours.

Comment 35

[Daniel Stenberg [:bagder]]

Let me add my 2 cents on how I think we can move this issue forward:

1. We figure out *exactly* which assign of fe_fnlen that gets the length wrong, and exactly what the input line is that triggers this. I would feel much safer if we would act at once when we figure out the format is wrong rather than check the end results and see if the length is bananas.

2. Then we tighten the parser for that line format so that we can't get the name length wrong. And we land that fix.

If we find further problems once this is landed, we iterate on this. I don't believe in importing someone else's code for this, and I fear that disabling parts of the parser with a pref only postpones this work since someone will eventually enable that section and get hit by this bug down the line.

pdknsk, can we get a recipe to reproduce this or can you work on making ParseFTPList() instead return an error when this happens?

Comment 36

[u473386]

I'm not familiar with the error reporting machinery, but you can just modify patch ftp-patch I attached previously.

+        if (result.fe_fname + result.fe_fnlen > eol) {
+          // report error
+        }

The exact line to trigger this is attached as 37-bytes attachment bug.

Comment 37

[Honza Bambas (:mayhemer)]

Please raise the priority again if this is actually exploitable.

Comment 38

[Christian Holler (:decoder)]

This needs to be fixed because it is blocking any kind of additional fuzz testing on the FTP code (which we consider important to perform as this code is really old and not well-tested).

Comment 39

[Kershaw Chang [:kershaw]]

(In reply to Christian Holler (:decoder) from comment #38)

This needs to be fixed because it is blocking any kind of additional fuzz testing on the FTP code (which we consider important to perform as this code is really old and not well-tested).

Move this to P2 for now.
I'll also bring this to our round table of necko meeting.

Comment 40

[Kershaw Chang [:kershaw]]

Michal will try to fix this bug.
Thanks!

Comment 41

[Michal Novotny [:michal]]

Attachment: Bug 1478950 - Fix parsing filename in OS/2 FTP listing, r=valentin

The parsing code uses filename without checking its presence. This patch ensures that the filename contains at least one non white space character.

Comment 42

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/7e57bb8c2fdc1d1764f945fcc242993ad279fe04
https://hg.mozilla.org/mozilla-central/rev/7e57bb8c2fdc

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Has the security diagnosis from comment 15/16 changed or is this still a wontfix for uplifts?

Comment 44

[Michal Novotny [:michal]]

I think the severity hasn't changed. result.fe_fnlen will always be slightly less than UINT32_MAX and assertion in
nsTSubstring::nsTSubstring is a release assertion, so it should always assert and we won't access memory out of the buffer.