1751476 - Optimistic assumptions about STARTTLS upgrade and passwords [security]

Status: ASSIGNED

(MailNews Core :: Networking: SMTP, defect, P1)

Description

[Ben Bucksch (:BenB)]

Situation: We're connecting to a malicious server (due to MITM, DNS poisoning, IP level attacks etc.), and the server is trying a downgrade attack, forcing Thunderbird to not use TLS, then listening on the connection, including the emails, and possibly including the password.

1. SmtpClient.jsm function _actionSTARTTLS() has:

    this._secureTransport = true;
    this.socket.upgradeToSecure();

The order should be reversed, because the upgrade can fail.

Let's say the server deliberately lets the STARTTLS upgrade fail. this._secureTransport would be set to true, and other code later on then depends on that value to decide whether the connection is fine, e.g.
function _actionEHLO()

if (!this._secureTransport && this.options.alwaysSTARTTLS) {
  ... fail
... else continue ...

2. I assume here that upgradeToSecure() would throw an error in case the upgrade failed, but that also needs to be checked.

3. There are likely other conditions that are problematic. The code also in other parts of SmtpClient.jsm seems to make sunny assumptions and needs to better consider a malicious server that deliberately makes the code fail. A more dedicated and detailed security analysis of this code for error scenarios in the SSL and encrypted passwords options is recommended.

Comment 1

[Ping Chen [:rnons] (away)]

(In reply to Ben Bucksch (:BenB) from comment #0)

1. SmtpClient.jsm function _actionSTARTTLS() has:

    this._secureTransport = true;
    this.socket.upgradeToSecure();

The order should be reversed, because the upgrade can fail.

Let's say the server deliberately lets the STARTTLS upgrade fail. this._secureTransport would be set to true, and other code later on then depends on that value to decide whether the connection is fine, e.g.

I think the order doesn't matter a lot as long as upgradeToSecure do the right thing. I will reverse it later anyway.

2. I assume here that upgradeToSecure() would throw an error in case the upgrade failed, but that also needs to be checked.

This indeed seems to be a problem. Seems to me we should check the rv of https://searchfox.org/mozilla-central/rev/5ad2b9480ab6809c28d64ff56918ceb394642c72/dom/network/TCPSocket.cpp#316, and throw if failed. :valentin: what do you think? I will send a patch later today.

Comment 2

[Ben Bucksch (:BenB)]

Hi Ping, thanks for handling this!

as long as upgradeToSecure() do the right thing

That's the problem: What if a malicious server makes it fail deliberately? Then secureConnection is already true, and later code makes assumptions that the link is secure. Such failure cases must be handled, otherwise there's the risk for security holes.

Comment 3

[Ping Chen [:rnons] (away)]

Attachment: Bug 1751476 - Throw in TCPSocket::UpgradeToSecure if failed to activate tls. r=valentin

Comment 4

[Ping Chen [:rnons] (away)]

(In reply to Ben Bucksch (:BenB) from comment #2)

as long as upgradeToSecure() do the right thing

That's the problem: What if a malicious server makes it fail deliberately? Then secureConnection is already true, and later code makes assumptions that the link is secure. Such failure cases must be handled, otherwise there's the risk for security holes.

If upgradeToSecure fails and throws, the socket is disconnected, the value of _secureConnection doesn't matter.
If upgradeToSecure fails silently, changing the order doesn't help. So I don't need to reverse it.

Comment 5

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

👍‍‍

Comment 6

[Ben Bucksch (:BenB)]

Re Problem 2:

Thanks for finding and fixing the bug in TCPSocket!

Re Problem 1:

I don't need to reverse it.

I'm glad that the reversed order (Problem 1) is not an actual bug currently.
However, the current logic is making assumptions about the behavior of another component. The API of upgradeToSecure() does not promise to abort the connection in case the upgrade fails. If could also just throw and leave the connection open, and would still adhere to the API. Thus, it would be safer to reverse the order of the 2 statements, and is trivial to do.

Re Problem 3:
It would be good to security-reviewed the entire file with an attitude of being fail-safe, particularly in light of unforseen circumstances and deliberate attemps to exploit it.

Comment 7

[Ping Chen [:rnons] (away)]

Attachment: Bug 1751476 - Send QUIT or close TCPSocket immediately in onerror handler. r=mkmelin

Comment 8

[Ping Chen [:rnons] (away)]

The API of upgradeToSecure() does not promise to abort the connection in case the upgrade fails.

We already have code to close socket in the onerror handler. My patch just now moves the quit/close to the beginning of onerror.

Comment 9

[Ping Chen [:rnons] (away)]

Only D137261 is the patch for c-c and needs checkin, thanks

Comment 10

[Ben Bucksch (:BenB)]

Patch: Send QUIT or close TCPSocket immediately in onerror handler

@rnons: Thank you!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cb73a6c8eb85
Send QUIT or close TCPSocket immediately in onerror handler. r=mkmelin

Comment 12

[Wayne Mery (:wsmwk)]

Gene, Magnus,
Any ideas why the test doesn't work?

Comment 13

[Magnus Melin [:mkmelin]]

Commented on phab.

Comment 14

[Wayne Mery (:wsmwk)]

Hey, how are we proceeding with this from here?
Also, the severity is set at S1 - how is this a release blocker or a work blocker?