Bug 1336699 (CVE-2017-5405)

Uninitialized value in nsFtpState::R_pasv

RESOLVED FIXED in Firefox -esr45

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: chamal.desilva, Assigned: mcmanus)

Tracking

({csectype-uninitialized, sec-low})

51 Branch
mozilla54
csectype-uninitialized, sec-low
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox-esr4552+ fixed, firefox51 wontfix, firefox52+ fixed, firefox-esr5252+ fixed, firefox53+ fixed, firefox54+ fixed)

Details

(Whiteboard: [post-critsmash-triage][necko-active][adv-main52+][adv-esr45.8+])

Attachments

(2 attachments)

(Reporter)

Description

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

Comment 1

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

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

Comment 3

2 years ago
this is a great bug report chamal - thank you.
Flags: needinfo?(mcmanus)
(Assignee)

Updated

2 years ago
Keywords: csectype-uninitialized, sec-high
(Assignee)

Comment 4

2 years ago
Attachment #8835023 - Flags: review?(dd.mozilla)
(Assignee)

Updated

2 years ago
Assignee: nobody → mcmanus
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8835023 - Flags: review?(dd.mozilla) → review+
(Assignee)

Comment 5

2 years ago
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.
status-firefox51: --- → wontfix
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox52: --- → +
tracking-firefox53: --- → +
tracking-firefox54: --- → +
tracking-firefox-esr52: --- → 52+
Attachment #8835023 - Flags: sec-approval? → sec-approval+
Whiteboard: [necko-active]
(Assignee)

Comment 8

2 years ago
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
Last Resolved: 2 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Reporter)

Comment 10

2 years ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/f33a97693508
https://hg.mozilla.org/releases/mozilla-beta/rev/ae84530a2103
status-firefox52: affected → fixed
status-firefox53: affected → fixed
status-firefox-esr52: --- → affected
tracking-firefox-esr45: --- → ?
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]
tracking-firefox-esr45: ? → 52+
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)
(Assignee)

Comment 17

2 years ago
(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-high → sec-low
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.