1474492 - FTPListParser fuzzing target

Status: RESOLVED WORKSFORME

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

Description

[u473386]

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:

What do you think? It's just a single file that is fuzzed, but please take a look. It's a branch maze.

https://dxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/ParseFTPList.cpp

I've let it run for a few hours, with no bugs found so far, but coverage is still increasing. Considering that some branches are very difficult for libFuzzer to reach, it may stay busy for weeks to come.

Much of the code doesn't seem particularly prone to security bugs, but there are also some memcpys in there, and constructs like this.

          for (pos = 0; pos < toklen[1]; pos++)
            *dot++ = *p++;

I think it might also be a good target for UBSAN, should support be added.

Comment 1

[u473386]

Attachment: FTPListParser

Comment 2

[u473386]

Attachment: FTPListParser

I think the previous patch was malformed.

Comment 3

[u473386]

(I misread the above code snippet.)

Comment 4

[u473386]

Attachment: FTPListParser-2

Comment 5

[u473386]

Attachment: FTPListParser-3

An unrelated file got into FTPListParser-2.

Comment 6

[Christian Holler (:decoder)]

Comment on attachment 8991216 [details] [diff] [review]
FTPListParser-3

I did some brief fuzzing of this code before (local only) and I think it is worth adding a target. However, the functions in nsFTPDirListingConv itself also look complicated enough to target them. Hence, I suggest fuzzing nsFTPDirListingConv itself, rather than just the ParseFTPList function. I did this once before briefly and wrote some code to do it (but I never got around to land it). I can attach my sample code, maybe you can test it and ensure it does what it is supposed to do?

Comment 7

[Christian Holler (:decoder)]

Attachment: Example for targeting nsFTPDirListingConv

Not sure if this still compiles, some headers might not be required and you might have to check that it really hits the function you originally wanted to target. Let me know if there is any trouble. With this code, also some other converters could be targeted, although they are all pretty simple I think.

Thanks for working on this!

Comment 8

[u473386]

Thanks, works well. I verified it reaches ParseFTPList. I only made a few minor adjustments. Most of the headers weren't needed.

Comment 9

[u473386]

Attachment: FTPDirListConv

Comment 10

[David Olah]

Hi Christian,

Should this issue be set on Core: Platform Fuzzing Tea component?

If this is not the right component, please update it to the right one.

Comment 11

[Christian Holler (:decoder)]

We generally file these bugs in the component that they test, adjusting based on that :)

Comment 12

[Christian Holler (:decoder)]

Comment on attachment 8991801 [details] [diff] [review]
FTPDirListConv

Review of attachment 8991801 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this! From the fuzzing perspective, this looks all correct. Forwarding for a final review to Valentin, so a Necko peer can check on the code itself as well :)

Comment 13

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

Comment on attachment 8991801 [details] [diff] [review]
FTPDirListConv

Review of attachment 8991801 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Thanks!

::: netwerk/streamconv/converters/fuzztest/FTPDirListConv_fuzzer.cpp
@@ +15,5 @@
> +
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIREQUESTOBSERVER
> +
> +  NS_IMETHODIMP OnDataAvailable(nsIRequest* aRequest,

I think this ought to be NS_IMETHOD, without the imp

Comment 14

[Christian Holler (:decoder)]

Marking s-s until this has received more fuzzing, so we don't easily expose potential security problems.

Comment 15

[u473386]

Do you want to author the patch, or should I do it? I mean you wrote the code basically.

Comment 16

[Christian Holler (:decoder)]

Feel free to author the patch, I am currently too busy with the ASan Nightly Project to handle additional code landing :)

Comment 17

[u473386]

Attachment: FTPDirListConv-2

> I think this ought to be NS_IMETHOD, without the imp

Done.

Comment 18

[u473386]

Attachment: FTPDirListConv-3

Minor formatting fix.

Comment 19

[u473386]

First blocking bug from local fuzzing.

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

==3176==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f23cc0fdc15 bp 0x7ffdad88edf0 sp 0x7ffdad88ede0 T0)
==3176==The signal is caused by a WRITE memory access.
==3176==Hint: address points to the zero page.
    #0 0x7f23cc0fdc14 in nsTSubstring<char>::nsTSubstring(char*, unsigned int, mozilla::detail::StringDataFlags, mozilla::detail::StringClassFlags) mozilla-central/xpcom/string/nsTSubstring.h:876:5
    #1 0x7f23cc1127c8 in nsTDependentSubstring<char>::nsTDependentSubstring(char const*, char const*) mozilla-central/xpcom/string/nsTDependentSubstring.cpp:57:5
    #2 0x7f23cc115d75 in nsTDependentSubstring<char> const Substring<char>(char const*, char const*) mozilla-central/xpcom/string/nsTDependentSubstring.cpp:92:10
    #3 0x7f23ccaad74c in nsFTPDirListingConv::DigestBufferLines(char*, nsTString<char>&) mozilla-central/netwerk/streamconv/converters/nsFTPDirListingConv.cpp:270:37

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

Comment 20

[Christian Holler (:decoder)]

Can you open a bug blocking this one? Mark it s-s so we don't reveal what we are doing in this bug (not because the other bug itself would be s-s). My guess is we need to apply a useful limit to this length.

Comment 21

[u473386]

Attachment: FTPDirListingConv

(No changes in this patch other than renaming the target to FTPDirListingConv).

Comment 22

[u473386]

When the blocking bug is fixed, I think this should be good to go. I've been fuzzing locally with its patch, and there were no reports yet. By the way, it's good you suggested fuzzing nsFTPDirListingConv, as just fuzzing FTPListParser wouldn't have found the bug.