Closed Bug 1336699 (CVE-2017-5405) Opened 4 years ago Closed 4 years ago

Uninitialized value in nsFtpState::R_pasv

Categories

(Core :: Networking: FTP, defect)

51 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 52+ fixed
firefox51 --- wontfix
firefox52 + fixed
firefox-esr52 52+ fixed
firefox53 + fixed
firefox54 + fixed

People

(Reporter: chamal.desilva, Assigned: mcmanus)

Details

(Keywords: csectype-uninitialized, sec-low, Whiteboard: [post-critsmash-triage][necko-active][adv-main52+][adv-esr45.8+])

Attachments

(2 files)

Attached file 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.
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;
Flagging :mcmanus for triage / prioritization. :-)
Group: firefox-core-security → core-security
Component: Untriaged → Networking: FTP
Flags: needinfo?(mcmanus)
Product: Firefox → Core
Group: core-security → network-core-security
this is a great bug report chamal - thank you.
Flags: needinfo?(mcmanus)
Attachment #8835023 - Flags: review?(dd.mozilla)
Assignee: nobody → mcmanus
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8835023 - Flags: review?(dd.mozilla) → review+
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
Attachment #8835023 - Flags: sec-approval?
sec-approval+ for trunk. 

We should get branch patches nominated for Aurora, Beta, and ESR45 as well.
Attachment #8835023 - Flags: sec-approval? → sec-approval+
Whiteboard: [necko-active]
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
Attachment #8835023 - Flags: approval-mozilla-esr45?
Attachment #8835023 - Flags: approval-mozilla-beta?
Attachment #8835023 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d6f4704c8718
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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 on attachment 8835023 [details] [diff] [review]
scanf has signed return value

signed vs unsigned confusion in ftp pasv response parsing, for aurora53 and beta52
Attachment #8835023 - Flags: approval-mozilla-beta?
Attachment #8835023 - Flags: approval-mozilla-beta+
Attachment #8835023 - Flags: approval-mozilla-aurora?
Attachment #8835023 - Flags: approval-mozilla-aurora+
Group: network-core-security → core-security-release
Comment on attachment 8835023 [details] [diff] [review]
scanf has signed return value

Fix a sec-high. ESR45+.
Attachment #8835023 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Flags: qe-verify+
Whiteboard: [necko-active] → [post-critsmash-triage][necko-active]
Whiteboard: [post-critsmash-triage][necko-active] → [post-critsmash-triage][necko-active][adv-main52+][adv-esr48.8+]
Whiteboard: [post-critsmash-triage][necko-active][adv-main52+][adv-esr48.8+] → [post-critsmash-triage][necko-active][adv-main52+][adv-esr45.8+]
Alias: CVE-2017-5405
(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.
Flags: needinfo?(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.
Flags: needinfo?(mcmanus)
Keywords: sec-highsec-low
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.