1336699 - (CVE-2017-5405) Uninitialized value in nsFtpState::R_pasv

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Chamal De Silva]

Attachment: Server.cpp

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

1. Build Firefox with valgrind.
   I built with options and instructions  mentioned in
   https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Valgrind
2. Start firefox
   export VALGRIND_OPTIONS="--smc-check=all-non-file --vex-iropt-register-updates=allregs-at-mem-access --show-mismatched-frees=no --read-inline-info=yes"
   ./mach run --debugger="valgrind" --debugger-args="$VALGRIND_OPTIONS"
3. Download and compile Server.cpp on a Linux machine.
   g++ -w -o Server Server.cpp
4. Start the server on port 21.
   sudo ./Server
5. Visit ftp://127.0.0.1



Actual results:

Valgrind will display.

==28235== Conditional jump or move depends on uninitialised value(s)
==28235==    at 0xFECEB36: mozilla::net::nsSocketTransportService::CreateRoutedTransport(char const**, unsigned int, nsACString_internal const&, int, nsACString_internal const&, int, nsIProxyInfo*, nsISocketTransport**) (nsSocketTransportService2.cpp:741)
==28235==    by 0xFECEC85: mozilla::net::nsSocketTransportService::CreateTransport(char const**, unsigned int, nsACString_internal const&, int, nsIProxyInfo*, nsISocketTransport**) (nsSocketTransportService2.cpp:713)
==28235==    by 0x101455F7: nsFtpState::R_pasv() [clone .part.108] [clone .constprop.145] (nsFtpConnectionThread.cpp:1501)
==28235==    by 0x1014EDC7: R_pasv (nsFtpConnectionThread.cpp:526)
==28235==    by 0x1014EDC7: nsFtpState::Process() (nsFtpConnectionThread.cpp:632)
==28235==    by 0x1014F422: nsFtpState::OnControlDataAvailable(char const*, unsigned int) (nsFtpConnectionThread.cpp:221)
==28235==    by 0x10142E17: nsFtpControlConnection::OnInputStreamReady(nsIAsyncInputStream*) (nsFtpControlConnection.cpp:61)
==28235==    by 0xFDF2333: nsInputStreamReadyEvent::Run() (nsStreamUtils.cpp:96)
...



Expected results:

Uninitialized value should not be used.

Comment 1

[Chamal De Silva]

Cause of Bug
------------

Bug is present in R_pasv method of netwerk\protocol\ftp\nsFtpConnectionThread.cpp file.
FTP response of "200 (\r\n" to R_pasv method causes unitialized value in "int32_t p0". 
"p0" variable is later used to calculate "int32_t port".

Below mentioned line in R_pasv method is the cause of this bug.
fields = PR_sscanf(ptr,"%ld,%ld,%ld,%ld,%ld,%ld",
&h0, &h1, &h2, &h3, &p0, &p1);

fields is uint32_t type variable. PR_sscanf method returns -1 on error conditions.
So fields variable become 4294967295 when PR_sscanf method return -1.
This causes below mentioned if condition in R_pasv method to fail.

if (fields < 6)    
  return FTP_ERROR;

Comment 2

[:Gijs (he/him)]

Flagging :mcmanus for triage / prioritization. :-)

Comment 3

[Patrick McManus [:mcmanus]]

this is a great bug report chamal - thank you.

Comment 4

[Patrick McManus [:mcmanus]]

Attachment: scanf has signed return value

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8835023 [details] [diff] [review]
scanf has signed return value

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
easily.. though its not clear how serious of an exploit you could manage. it looks you can trigger a connection to some host defined by unitialized contents of the stack

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
its pretty clear

Which older supported branches are affected by this flaw?
all

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
they will be the same - this is static code effectively

How likely is this patch to cause regressions; how much testing does it need?
insignificant regression risk

Comment 6

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

sec-approval+ for trunk. 

We should get branch patches nominated for Aurora, Beta, and ESR45 as well.

Comment 7

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6f4704c8718ba919b794d3ebc84165839836f03
Bug 1336699 - scanf has signed return value r=dragana

Comment 8

[Patrick McManus [:mcmanus]]

Comment on attachment 8835023 [details] [diff] [review]
scanf has signed return value

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
na

User impact if declined: exploit would be to open a new connection via PASV to some location unintialized from stack (so stack leak, and stray connection).

Fix Landed on Version: 54

Risk to taking this patch (and alternatives if risky): very low risk located to ftp client code

String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: latent forever bug
[User impact if declined]: see above
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: hand tested
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: one line with local scope
[String changes made/needed]: none

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 10

[Chamal De Silva]

Hi Patrick, I think it is not possible to trigger a connection to another host.
Integer variables h0, h1, h2 and h3 in below line are not used later.

fields = PR_sscanf(ptr,"%ld,%ld,%ld,%ld,%ld,%ld",&h0, &h1, &h2, &h3, &p0, &p1);

Only p0 and p1 integers are later used to calculate port.

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 8835023 [details] [diff] [review]
scanf has signed return value

signed vs unsigned confusion in ftp pasv response parsing, for aurora53 and beta52

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f33a97693508
https://hg.mozilla.org/releases/mozilla-beta/rev/ae84530a2103

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8835023 [details] [diff] [review]
scanf has signed return value

Fix a sec-high. ESR45+.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/381552c888b4

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/ae84530a2103

Comment 16

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

(In reply to Chamal from comment #10)
> Hi Patrick, I think it is not possible to trigger a connection to another
> host.
> Integer variables h0, h1, h2 and h3 in below line are not used later.
> 
> fields = PR_sscanf(ptr,"%ld,%ld,%ld,%ld,%ld,%ld",&h0, &h1, &h2, &h3, &p0,
> &p1);
> 
> Only p0 and p1 integers are later used to calculate port.

Patrick, is this correct or is it possible to trigger a connection? I need to know for the advisory.

Comment 17

[Patrick McManus [:mcmanus]]

(In reply to Al Billings [:abillings] from comment #16)
> (In reply to Chamal from comment #10)
> > Hi Patrick, I think it is not possible to trigger a connection to another
> > host.
> > Integer variables h0, h1, h2 and h3 in below line are not used later.
> > 
> > fields = PR_sscanf(ptr,"%ld,%ld,%ld,%ld,%ld,%ld",&h0, &h1, &h2, &h3, &p0,
> > &p1);
> > 
> > Only p0 and p1 integers are later used to calculate port.
> 
> Patrick, is this correct or is it possible to trigger a connection? I need
> to know for the advisory.

reporter is correct - its just the port number.