807555 - FTP crash if server reply is missing directory

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[sczimmer]

In response to 'PWD', normally FTP servers send something with some text between the quotes like:
257 "/" is current directory.

The ftp server on RICOH Aficio printers sends:
257 "" is current option.

in nsFtpConnectionThread,
   nsFtpState::R_pwd()

cuts out the part of the response between quotes and then calls respStr.Last()
calling Last() on an empty string crashes the browser

Comment 1

[Jason Duell]

"crashes the browser": sounds like this could be invoked simply by redirecting users to an FTP server that omits the "/".

Deterministic crashes get sec-critical, right?

Comment 2

[Jason Duell]

Note I haven't confirmed this.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jason Duell (:jduell) from comment #1)
> Deterministic crashes get sec-critical, right?

Only if it's an exploitable crash - no evidence that this is at the moment, looks to me like just a null deref?

Comment 4

[sczimmer]

It is a deterministic crash, but it is not exploitable.
It seg faults at: 'return mData[mLength - 1];' since mLength is 0 so mLength - 1 = 0xffffffff
so it's not quite a null deref, but it crashes trying to deref mData + 0xffffffff
Even if the attacker could somehow map and control the value at that address, there's still no chance of an exploit as it just reads the value and compares it with '/' to decide if it needs to add a '/' to the end of the string

If you want to reproduce it, any of these ftp servers should crash firefox http://www.shodanhq.com/search?q=RICOH+aficio+port%3A21

Comment 5

[Christoph Diehl [:posidron]]

Attachment: callstack

Still nice find.

Adding the callstack of the crash.

Comment 6

[Jason Duell]

Does this bug need to still be hidden?  I assume no.

Comment 7

[Jason Duell]

Attachment: v1

I've verified this fixes the crash on

  ftp://math-copier.acadiau.ca

Comment 8

[Jason Duell]

Comment on attachment 677893 [details] [diff] [review]
v1

Bugzilla decides to post a little, easily-missed message if the requestee's :nickname is ambiguous.  Not a feature...

Comment 9

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49258354b9d0

Making public based on comment 4.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/49258354b9d0