IMAP Response Injection when using STARTTLS
Categories
(Thunderbird :: Security, defect, P1)
Tracking
(thunderbird_esr78+ fixed, thunderbird85 fixed)
People
(Reporter: poddebniak, Assigned: gds)
References
Details
(Keywords: sec-moderate, Whiteboard: [TM:78.7.0])
Attachments
(3 files, 8 obsolete files)
|
52.49 KB,
image/jpeg
|
Details | |
|
179.60 KB,
image/png
|
Details | |
|
5.56 KB,
patch
|
benc
:
review+
wsmwk
:
approval-comm-beta+
wsmwk
:
approval-comm-esr78+
|
Details | Diff | Splinter Review |
starttls-sequence-bad.jpg
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:74.0) Gecko/20100101 Firefox/74.0
Steps to reproduce:
Thunderbird has connected to an IMAP server and issued the starttls command. The server responds with "okay, let's start TLS now". However, the server (or some malicious party) also appends extra data after the starttls server response.
Example trace:
S: * OK [CAPABILITY IMAP4REV1 STARTTLS LOGINDISABLED]\r\n
C: 1 STARTTLS\r\n
S: 1 OK start TLS now\r\n* OK [ALERT] Huch?\r\n2 OK ...\r\n4 OK ...\r\n
----- <TLS handshake> --- everything after this line is encrypted -----
C: 2 capability\r\n
// Thunderbird proceeds with login, as "2 OK" was already received/buffered
C: 4 login \"alice\" \"password\"\r\n
// Thunderbird proceeds with list, as "4 OK" was already received/buffered
C: 5 list \"\" \"*\"\r\n
...
Actual results:
Thunderbird shows the alert, and also interprets both results even though they were provided via plaintext (w/o TLS.)
1 OK start TLS now\r\n* OK [ALERT] Huch?\r\n2 OK ...\r\n4 OK ...\r\n
It seems that when the server answers with multiple results attached directly after "1 OK start TLS now\r\n" response, all following responses are written to an internal buffer first. Only after that, all sockets are switched to speak TLS. However, the old responses are kept and thus end up being interpreted as if they were read via TLS.
This issue was first described by Wietse Venema (www.postfix.org/CVE-2011-0411.html). Theoretically, any other protocols supporting STARTTLS (SMTP, LDAP, ...) could also be affected.
Expected results:
Thunderbird must not interpret the additional data. Arguably, everything after "1 OK start TLS now\r\n" should be interpreted as the SERVER_HELLO TLS message. An easier to implement, but potentially breaking alternative is to discard the data.
Using the additional data allows an attacker to inject malicious responses, which are interpreted as if they came via a secure TLS connection.
I guess that this issue could in one way or another be exploited to obtain sensitive data or be very useful to mount a successful attack on STARTTLS.
Please see the attached test program for a demonstration of the issue.
You will need to provide a a certificate chain and a private key for the leaf certificate. I used the mkcert tool [1] to create a local root CA and issued a certificate for 127.0.0.1.
Thunderbird is configured as follows:
IMAP: 127.0.0.1:10143 (STARTTLS)
SMTP: 127.0.0.1:10587 (STARTTLS)
To make testing easier, the provided example server also implements the minimum SMTP functionality to pass thunderbirds account wizard. Thus, you can setup a fresh thunderbird profile (-ProfileManager) for testing.
|
||
Comment 1•5 years ago
|
||
Summary:
Attacker injects protocol commands during the plaintext phase, which will get evaluated within the encrypted session.
|
|
||
Comment 2•5 years ago
|
||
(In reply to Damian Poddebniak from comment #0)
Thunderbird shows the alert
How is the alert shown? WIth an interactive dialog/prompt?
|
Reporter | |
Comment 3•5 years ago
|
||
Yes, I see a notification in my gnome shell. I only used the alert, because it is easier to see the issue. 2 OK and 4 OK are probably more important as they are used to answer the capability and login commands TB will send in the future. However, alerts do generally work pre-TLS and I think they maybe shouldn't. Shall I open another issue?
|
||
Comment 4•5 years ago
|
||
How did Postfix patch this? As you mentioned there are two options discussed, error out or ignore the extra data. Erroring would be clearer to everyone, but might cause compat problems. I guess either could cause compat problems, so at least the error approach would make the failure easier to diagnose.
Setting severity to "moderate" based on the CVSS I see on the old advisories, but that was 9 years ago and we might have higher standards now?
|
Assignee | |
Comment 5•5 years ago
•
|
||
I installed mkcert using the "linuxbrew" method and made the certificate and key file for localhost, 127.0.0.1 and ::1. Then built your server using "cargo build". I then run the server like this:
response-injection-release]$ target/debug/response-injection ~/.linuxbrew/localhost+2.pem ~/.linuxbrew/localhost+2-key.pem
In tb I create the account but don't know what username and password I should use. Otherwise it seems to work. I see the STARTTLS command and response and the alert popup occurs. It tries to use "login" authentication and asks for password and I give it my usual user password and it fails and nothing more happens. Should I be able to complete the login?
(I'm on fedora 31 linux.)
Edit: I see this in the server console:
IMAP: 127.0.0.1:57544 --> 127.0.0.1:10143
S: * OK [CAPABILITY IMAP4REV1 STARTTLS LOGINDISABLED]\r\n
C: 1 STARTTLS\r\n
S: 1 OK start TLS now\r\n* OK [ALERT] This is plaintext, which should not have been interpreted. We can inject more plaintext and e.g. already answer Thunderbird's upcoming capability command.\r\n2 OK [CAPABILITY IMAP4REV1 AUTH=X1337METHOD]\r\n4 OK login seems legit, I guess?\r\n
thread '<unnamed>' panicked at 'called Result::unwrap() on an Err value: Custom { kind: InvalidData, error: AlertReceived(UnknownCA) }', src/libcore/result.rs:1188:5
I suppose the panic means I don't have somthing set up right?
Edit2: Had to import ~/.local/share/mkcert/rootCA-key.pem. Now I see:
IMAP: 127.0.0.1:57636 --> 127.0.0.1:10143
S: * OK [CAPABILITY IMAP4REV1 STARTTLS LOGINDISABLED]\r\n
C: 1 STARTTLS\r\n
S: 1 OK start TLS now\r\n* OK [ALERT] This is plaintext, which should not have been interpreted. We can inject more plaintext and e.g. already answer Thunderbird's upcoming capability command.\r\n2 OK [CAPABILITY IMAP4REV1 AUTH=X1337METHOD]\r\n4 OK login seems legit, I guess?\r\n
C: 2 capability\r\n
C: 4 login "gene@127.0.0.1" "mypassword"\r\n
C: 5 capability\r\n
thread '<unnamed>' panicked at 'called Result::unwrap() on an Err value: Custom { kind: ConnectionAborted, error: "CloseNotify alert received" }', src/libcore/result.rs:1188:5
At least now tb tries to send password but not sure why "capability" is sent twice with no response and then the server still seems to give up and no response to login.
|
|
Reporter | |
Comment 6•5 years ago
|
||
Oh, sorry. Two things first:
- I should't have used 127.0.0.1... please use only "localhost" to avoid other TLS errors.
- you need to use "mkcert -install" to install the local CA in the system trust store.
If Thunderbird prompts for a password, just type anything. There is no authentication going on whatsoever.
Your trace looks like expected.
S: * OK [CAPABILITY IMAP4REV1 STARTTLS LOGINDISABLED]\r\n
C: 1 STARTTLS\r\n
until here everything is normal.
S: 1 OK start TLS now\r\n* OK [ALERT] This is plaintext, which should not have been interpreted. We can inject more plaintext and e.g. already answer Thunderbird's upcoming capability command.\r\n2 OK [CAPABILITY IMAP4REV1 AUTH=X1337METHOD]\r\n4 OK login seems legit, I guess?\r\n
The server responds with multiple responses now. Only "1 OK start TLS now" is properly defined in the spec. After this line, everything what the server send should be TLS. However, there is trailing plaintext:
- the Alert (which is what you see in the dialog)
- the "2 OK" response, which should only be send after TB issued the "2 capability" command and
- the "4 OK" response, which should also only be issued after TB sends the login command.
Note: I hardcoded the values after observing that TB uses deterministic tags.
Then TB sends the following line
C: 2 capability\r\n
Normally, TB will wait for the "2 OK" response. However, it was already received before. This is why TB directly proceeds with
C: 4 login "gene@127.0.0.1" "mypassword"\r\n
For some reason my setup does not send the "5 capability", but continues with the "list" command. I used "STARTTLS/Normal password".
S: * OK [CAPABILITY IMAP4REV1 STARTTLS LOGINDISABLED]\r\n
C: 1 STARTTLS\r\n
S: 1 OK start TLS now\r\n* OK [ALERT] ...\r\n2 OK [CAPABILITY IMAP4REV1 AUTH=X1337METHOD]\r\n4 OK ...?\r\n
C: 2 capability\r\n
C: 4 login \"alice\" \"alice\"\r\n
C: 5 list \"\" \"*\"\r\n
However, I think this is not super important. The important bit is, that TB has accepted plaintext answers during an encrypted session. You can test it by removing everything after "* OK [ALERT] ...\r\n" and see that TB will wait for the capability response via TLS.
Sorry, I hope that makes sense?
I also forgot to mention how I would propose to fix this. The easiest thing you could do is to just call buffer.clear() after the client parsed the starttls and before the client continues with TLS (e.g. CLIENT_HELLO.) This solution might break in case the buffer already contains e.g. the SERVER_HELLO. But the server will not send it until the client sends its CLIENT_HELLO. So this might be easy to deploy and do not cause interoperability issues.
|
|
Reporter | |
Comment 7•5 years ago
|
||
A better solution would be to pass any remaining data in the buffer to the TLS socket as server data. However, common TLS stream implementations likely do not have this functionaility.
Btw, I am not 100% certain, that the buffering is the core issue here. This is only an educated guess.
|
|
Reporter | |
Comment 8•5 years ago
|
||
Also a note on severity:
We believe that we can build a PoC to obtain user credentials as MitM with this bug. However, in order to work, the server needs to behave in a certain way. (We have already reported this to a larger email provider, but most providers are luckily not affected.)
The Postfix bug is nearly a decade old and I am also not sure how critical a response injection really is in practice. What is the worst an attacker can do when injecting responses in TB? The best idea I came up with is expunging the inbox with "a begin starttls\r\n* expunge\r\n* expunge\r\n* expunge\r\n* expunge\r\n...."
|
|
Assignee | |
Comment 9•5 years ago
|
||
Ok, I now understand that your server doesn't respond to anything after it enters the loop to receive encrypted commands (just prints the commands with "C:" prefix). I added responses in that loop to "capability", "login" and "logout" and now get all the way to the list command which is not supported in the loop.
For some reason my setup does not send the "5 capability", but continues with the "list" command. I used "STARTTLS/Normal password".
That's because of a patch for another problem that I have in my tb code right now that sends the extra capability. So without the patch I should see the same as you see, even without making a change to your server code.
I also made a change to your server to respond NO to the login command I added. This should prevent login from tb. But as you have pointed out, even with the real NO response now sent by the server, the fake "4 OK ...?" response inserted with the STARTTLS response causes the login to succeed with a completely bogus userid and password and the real NO response is ignored. This seems kind of bad.
(In reply to Damian Poddebniak from comment #8)
Also a note on severity:
We believe that we can build a PoC to obtain user credentials as MitM with this bug. However, in order to work, the server needs to behave in a certain way. (We have already reported this to a larger email provider, but most providers are luckily not affected
The Postfix bug is nearly a decade old and I am also not sure how critical a response injection really is in practice. What is the worst an attacker can do when injecting responses in TB? The best idea I came up with is expunging the inbox with "a begin starttls\r\n* expunge\r\n* expunge\r\n* expunge\r\n* expunge\r\n...."
Postfix is smtp, right? Or maybe POP3? I don't see many recent imap servers using starttls. The only one I see defaulting to it is maybe Courier.
Not sure if I understand how the expunge would work. Looks like you show it as an untagged response which would be ignored in tb I think. Oh, but maybe not if there is also a leading sequence number. This may cause the message to disappear from view in tb, but they would probably come back when the folder re-syncs with the server. Seems like it would have to appear as an imap command to actually expunge anything from the server.
|
|
Reporter | |
Comment 10•5 years ago
|
||
Yup, Postfix is SMTP. Dovecot is POP3 and IMAP. But generally, I think that this issue can pretty much exist in any protocol with STARTTLS support, e.g. ftp, xmpp, telnet, irc, mysql, postgres, lmtp, nntp, sieve, ldap, ... Tbh, I still have to test TBs SMTP implementation for the same problem, but I have no time currently. Does TB support one of the other protocols?
I think that it is not super important if a server understands STARTTLS, as long as some client tries to speak STARTTLS. In the MitM scenario this is trivial, because a MitM attacker can just add the STARTTLS capability and maybe even block port 993. AFAIK, TB will also never automatically choose implicit TLS (port 993) via probing, even if it is available. At least the tests I conducted showed this. Maybe via Autoconfiguration (https://developer.mozilla.org/en-US/docs/Mozilla/Thunderbird/Autoconfiguration)?
Anyway, as soon as the client starts to accept STARTTLS, a MitM can simply do the whole pre-TLS plaintext protocol dance and than just forward the TLS traffic (starting with CLIENT_HELLO) to imap.example.org:993. This is also something we tested and what works. (ALPN could have helped here.) So if a server supports STARTTLS is not that important.
|
|
Assignee | |
Comment 11•5 years ago
|
||
Here's a possible fix for the STARTTLS response insertions. It does two things after TLS mode entered and before the first imap command, CAPABILITY, is sent encrypted.
- It drains the socket input buffer of any application data that is has not yet been read out and not yet stored in the application buffer (where IMAP parsing occurs).
- It clears the application buffer of any data that was read and stored after the STARTTLS OK response but before TLS mode was entered.
An assumption is that no new plaintext data can be received into the socket buffer after TLS mode is entered. (I have minimal knowledge of how the transition to TLS actually occurs in tb/moz code.)
In my initial tests using the rust IMAP server, only step 2 was necessary to fix the problem. However, I did some temporary modifications to the TB code that reads IMAP lines from the socket so that not all the socket data is read out before the CAPABILITY in TLS mode is sent and, even with step 2 code in place, the "inserted" responses were received by tb and caused problems. So both steps 1 and 2 are needed to make sure all the plaintext data sent after the STARTTLS "OK" response is discarded and ignored.
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
||
Comment 12•5 years ago
•
|
||
Looking at RFC 3501 it seems that everything after the server response (<tag> OK Begin TLS negotiation now\r\n) would be considered part of the StartTLS negotiation:
A [TLS] negotiation begins immediately after the CRLF at the end
of the tagged OK response from the server. Once a client issues a
STARTTLS command, it MUST NOT issue further commands until a
server response is seen and the [TLS] negotiation is complete.
So I'd guess a server which sends extra plaintext should result in a failed negotiation.
In an ideal world the TB IMAP parser would stop after the "\r\n", call StartTLS() and then, once the negotiation was complete, continue reading on the newly-secured socket. In practice, the IMAP parser will be greedy and slurp in as much data as it can buffer (otherwise you're just reading single bytes, looking for \r\n, which would suck). Because the StartTLS upgrade is handled at a lower level, the IMAP parser needs a way to feed the remaining buffered data back into the stream for the negotiation.
Back In the real world:
After the server sends it's <tag> OK Begin TLS negotiation now\r\n response, it must wait upon the client for the next step (which is, I think, the client saying which versions of SSL/TLS and ciphers it talks).
So, with that in mind, it seems perfectly fine for the client to completely discard everything after the server's OK Begin... line.
Gene's approach in comment 11 seems to be the right approach... although technically I think it wants to drain the socket and clear the TB IMAP-parser-incoming-data buffer before StartTLS() is called, rather than afterwards. But the main thing is that any dodgy data buffered up in the IMAP parser is ditched rather than used.
|
|
||
Comment 13•4 years ago
|
||
We should decide how we want to proceed here.
|
|
Reporter | |
Comment 14•4 years ago
|
||
Just so you know: We have disclosed this issue to other email client developers now and most people decided to just clear an internal buffer after STARTTLS. Have you decided what to do about this? gene smiths patch looks slightly more complicated to what I expected (maybe due to how streaming is implemented in TB?), but I guess this should prohibit the injection.
|
|
Assignee | |
Comment 15•4 years ago
|
||
(In reply to Ben Campbell from comment #12)
Looking at RFC 3501 it seems that everything after the server response (
<tag> OK Begin TLS negotiation now\r\n) would be considered part of the StartTLS negotiation:A [TLS] negotiation begins immediately after the CRLF at the end
of the tagged OK response from the server. Once a client issues a
STARTTLS command, it MUST NOT issue further commands until a
server response is seen and the [TLS] negotiation is complete.So I'd guess a server which sends extra plaintext should result in a failed negotiation.
I think this mean that after the client (TB) has sent STARTTLS it should be quiet, IMAP wise, until negotation is complete. I don't think any extra injected data the server sends after the STARTTLS Ok response will affect the negotiations.
In an ideal world the TB IMAP parser would stop after the "\r\n", call StartTLS() and then, once the negotiation was complete, continue reading on the newly-secured socket. In practice, the IMAP parser will be greedy and slurp in as much data as it can buffer (otherwise you're just reading single bytes, looking for
\r\n, which would suck). Because the StartTLS upgrade is handled at a lower level, the IMAP parser needs a way to feed the remaining buffered data back into the stream for the negotiation.Back In the real world:
After the server sends it's<tag> OK Begin TLS negotiation now\r\nresponse, it must wait upon the client for the next step (which is, I think, the client saying which versions of SSL/TLS and ciphers it talks).
Yes, that's in the "Hello Client" packet. See my attached diagrams starttls-sequence-bad.jpg.
So, with that in mind, it seems perfectly fine for the client to completely discard everything after the server's
OK Begin...line.
Yes, that's shown in my starttls-sequence-bad diagram. However, a malicious server or MitM could ignore the spec and send "More injections" after sending the "OK STARTTLS + Injection" and before starting negotiations. Depending on timing, this 2nd injection would never get cleared from the application buffers.
Gene's approach in comment 11 seems to be the right approach... although technically I think it wants to drain the socket and clear the TB IMAP-parser-incoming-data buffer before StartTLS() is called, rather than afterwards. But the main thing is that any dodgy data buffered up in the IMAP parser is ditched rather than used.
I think you mean the function called sslControl->StartTLS(). This starts up the TLS handshakes. The other function, which is called first and also named just StartTLS() sends the imap STARTTLS to the server. By itself this imap transaction doesn't cause any TLS transactions to occur from what I can tell. The call to sslControl->StartTLS() triggers the low level code to send the "Hello Client" to the server to begin the negotiation. So if bufferers are cleared before sslControl->StartTLS() is called, there is a small window where more injections that are never cleared can be sent by the server. I show this in the "bad" diagram.
My diagram starttls-sequence-good.jpg show my proposed approach. To avoid the problem of delayed injections, I wait until TLS mode is entered before clearing the application buffers. My assumptions are
- Once the client sends "Hello Client" any injections will will break the negotiations.
- A server that sucessfully negotiates and enters TLS mode with the client is not malicious.
- Clearing the application buffers of any injection after entry to TLS mode is effective.
This is why in the original diff above in comment 11 I do the clear after the sslControl->StartTLS() call.
However, looking at the sslControl->StartTLS() code, it is not clear to me exactly how to know when TLS mode is reached. Stepping with the debugger I don't see anything occur on the network when stepping over sslControl->StartTLS. I only see it when run at full speed. What I observed is that after ssl->StartTLS is called the subsequent IMAP transactions are encryped unless negotiations failed. If nego fails, nothing is sent on the network. (I forced nego fails by tweaking the tls version prefs.) But clearing the buffers right after ssl->StartTLS does not guarantee that TLS mode is yet entered since it sends nothing on the network. So in my next proposed code I do an IMAP noop before clearing buffers to ensure that TLS is entered.
I have also added a loop that will wait up to 5 seconds after the NOOP is sent (actually, just the tag is initailly send) that polls the connection security status. I'm not sure if this is really needed but it is semi-interesting. I did notice that with nothing sent before this loop that often secure state was never detected after 5 seconds or longer. With something send, it is always detected.
Just the tag "a " for the noop is initially sent so that a response from the server is not solicited. Then after the buffers are cleared the remaining noop command is sent "noop\r\n" and the response received in the normal manner and the usual imap commands continue on in TLS mode.
This works with the rust server, both Damian's original version and my modified version with more injections. I also tested this with one real imap server that uses STARTTLS connection security. I thinks it's located in Germany but not sure; it's a Dovecot server. I'm trying to locate some other test servers supporting STARTTLS but they are kind of rare.
|
|
Assignee | |
Comment 16•4 years ago
|
||
|
|
Assignee | |
Comment 17•4 years ago
|
||
There are a couple small N/A changes in this diff. The main change is in nsImapProtocol.cpp and is described in my previous long comment.
|
|
Assignee | |
Comment 18•4 years ago
|
||
Comment on attachment 9178905 [details]
starttls-sequence-good.jpg
I left out from the picture the "a " that is sent right before the ClearBuffer() client activity. This is need to ensure that TLS mode (secure mode) is entered before clearing the possible injected stuff. I also left out the send of "noop\r\n" right before Imap Capability is sent to complete the noop transaction.
|
|
||
Comment 19•4 years ago
|
||
|
|
Reporter | |
Comment 20•4 years ago
|
||
Can we register a CVE for this to have it documented? I can do that in case you do not prefer to do it yourself.
|
||
Updated•4 years ago
|
|
|
||
Comment 22•4 years ago
|
||
|
|
Assignee | |
Comment 23•4 years ago
|
||
Ben, I think I understand your points. You must be right that TLS negotiation doesn't start after sslControl->StartTLS() until a send is requested. That would explain why I never saw the bit that indicated secure state unless I attempted a send of something, e.g., the "a" tag.
I think you are also saying that once sslControl->StartTLS() returns that no additional malicious injected data can arrive in the socket buffer, even though SSL negotiation may not yet have started. So we just clear the buffers and do the next normal send and I don't need to send the dummy tag and NOOP.
If that's what you mean, that's basically what I proposed in my original (now marked obsolete) diff: attachment 9136652 [details] [diff] [review].
I think if something bad did arrive between the return of sslControl->StartTLS() and the start of the negotiation handshakes (a very small window), it would cause the negotiation to fail, which is probably OK. So just clearing the application buffer (m_inputStreamBuffer->ClearBuffer()) is probably sufficient and don't need the "kungFuGrip" loop? But maybe keep it just to be safe, don't know.
|
|
Assignee | |
Comment 24•4 years ago
|
||
I did some more testing with reporter Damian's server code. I added a sleep with a small delay after the write of the original insertion and did another insertion. Depending on the delay, this causes the additional insertion to occur either before, during or after the call to to sslControl->StartTLS(). If it occurs before or during, the inserted data looks just like the original insertion data and doesn't affect the negotiation. If it occurs after sslControl->StartTLS() call, it causes the negotiation to fail.
So it is OK to just clear the socket/stream buffer and the application buffer after sslControl->StartTLS() call as Ben recommended and it will get rid of any pre-TLS inserted data that is probably harmful.
I also noticed that when the negotiation failed due to socket data arriving during negotiation that there is a crash. It was caused by a null m_transport when calling m_transport->Close() here: https://searchfox.org/comm-central/rev/de8ef99bb6d07bdf76201be94c02f2b64cd8fbca/mailnews/imap/src/nsImapProtocol.cpp#1752. So I'll change this to check for m_transport as is done here: https://searchfox.org/comm-central/rev/de8ef99bb6d07bdf76201be94c02f2b64cd8fbca/mailnews/imap/src/nsImapProtocol.cpp#1758.
|
|
||
Comment 25•4 years ago
|
||
(In reply to gene smith from comment #23)
I think if something bad did arrive between the return of sslControl->StartTLS() and the start of the negotiation handshakes (a very small window), it would cause the negotiation to fail, which is probably OK.
I've been pondering this for the last hour or so and arguing myself into loops :-)
I did get a bit worried, but after trying to think through it, I'm happy again. Here's my train of thought:
The sslControl->StartTLS() is deferred - negotiation doesn't actually happen until later.
Writing is easy to reason about - if we've called StartTLS() then the new data must be sent securely.
For reading, I originally worried that the server (or MITM) could still be sending out plaintext data which our transport layer would cheerfully receive and buffer up, with no way to tell if it was secure or not. This would suck.
But now I'm less worried:
https://tools.ietf.org/html/rfc2595#section-3.1
"A TLS negotiation begins immediately after the CRLF at the end of the tagged OK response from the server."
This makes total sense, and implies that the negotiation starts for both ends at a really well defined time. And the only way for that to be sensible is if the client can treat everything after the sslControl->StartTLS() as encrypted - both reading and writing. Even though the negotiation is deferred, from a logical point of view, all read/write calls after the sslControl->StartTLS() can safely be assumed encrypted.
So, the security issue is when the server (or the MITM) sends more data directly after it's "OK" response, before the client calls sslControl->StartTLS(). Technically, this should probably fail the negotiation, but I think the way our transport layer is implemented means that it'll be likely already be sitting in our input buffer. So calling ClearBuffer() after sslControl->StartTLS() should do the trick nicely.
So just clearing the application buffer (m_inputStreamBuffer->ClearBuffer()) is probably sufficient and don't need the "kungFuGrip" loop? But maybe keep it just to be safe, don't know.
So all that about was a roundabout way to say yes, I agree with you :-) I think the ClearBuffer() on it's own will be enough. Anything new that arrives can (I think!) safely be assumed secure, because we've already called sslControl->StartTLS().
|
|
||
Comment 26•4 years ago
|
||
(In reply to gene smith from comment #24)
I also noticed that when the negotiation failed due to socket data arriving during negotiation that there is a crash. It was caused by a null m_transport when calling m_transport->Close() here: https://searchfox.org/comm-central/rev/de8ef99bb6d07bdf76201be94c02f2b64cd8fbca/mailnews/imap/src/nsImapProtocol.cpp#1752. So I'll change this to check for m_transport as is done here: https://searchfox.org/comm-central/rev/de8ef99bb6d07bdf76201be94c02f2b64cd8fbca/mailnews/imap/src/nsImapProtocol.cpp#1758.
Oh, beware that the m_transport in IMAP hides an m_transport in the base class! (Bug 1660714). If you've got any renaming suggestions, I'm all ears!
(I'm dithering about renaming because I don't want to change the base class one, and I don't want to use a longer name for the IMAP one!)
|
|
Assignee | |
Comment 27•4 years ago
|
||
I only saw the error due to null m_transport when I was setting an error value on "rv" when I still was testing with my proposed code that checked the bit indicating "secure state" and it never got set due to later injected data. I'm not seeing the error now since the original code and my new proposed code only sets "rv" to an error if rv = sslControl->StartTLS() fails. I've never seen StartTLS() fail .
But I'll add the check for null m_transport in my proposed diff. (Not sure I understand the name override issues.)
Just to be clear, the point where rv is checked is here: https://searchfox.org/comm-central/rev/de8ef99bb6d07bdf76201be94c02f2b64cd8fbca/mailnews/imap/src/nsImapProtocol.cpp#1746. "rv" could also be set bad if GetSecurityInfo() fails but that has nothing to do with TLS handshakes it appears.
So all that about was a roundabout way to say yes, I agree with you :-) I think the ClearBuffer() on it's own will be enough. Anything new that arrives can (I think!) safely be assumed secure, because we've already called sslControl->StartTLS().
To see what's really going on I had to look with wireshark. When I send my additional injection after sslControl->StartTLS() returns, I'm seeing tb still send the "Client Hello" packet. The server then sends, in cleartext, my additional injection string "Inject stuff\r\n". This causes tb to respond with "Alert (Level: Fatal, Description: Record overflow)". This is handled in moz code it seems. Tb imap then goes ahead and tries to send the CAPABILITY but sees an error, per IMAP:5 log, that the connection is dropped with error code 0x805a2fe7 (SSL_ERROR_RX_RECORD_TOO_LONG according to https://james-ross.co.uk/mozilla/misc/nserror?0x805A2FE7).
I'm not sure where the cleartext "Inject stuff" string is stored when received by tb and it probably doesn't matter since it breaks the TLS negotiation. Even a single injected character kills the negotiation and drops the connection (but with a different error code).
I never see anything left in the stream/socket buffer when I attempt to "drain" it. This is because when the imap STARTTLS OK response line is parsed, nsMsgLineStreamBuffer::ReadNextLine() pulls everything out of the socket buffer and copies it to the application buffer. The only way I ever see anything to drain from the socket buffer is if I limit the number of bytes Read() to a low number like 22, the number of bytes in the server's STARTTLS OK response. So, just to be sure, I'd feel better going ahead and draining this too since there may still exist a small window for receiving additional injected data that I haven't been able to hit that stores to the normal socket buffer and doesn't break negotiations.
|
|
Assignee | |
Comment 28•4 years ago
|
||
This still includes the drain of the stream/socket buffer. Also, I still have some printf to show what/where stuff gets drained. Also, includes the check for null m_transport. I'll submit a formal patch if you think this is OK.
|
|
Assignee | |
Comment 29•4 years ago
|
||
Here's an updated sequence diagram corresponding to starttls-resp-insertion-v3.diff.
The dashed lines show the case where the inserted data arrives right after sslControl->StartTLS() takes effect. This causes the negotiation to fail and is indicated by failure due to dropped transport connection while attempting to do the imap capability request. This will, of course, inhibit entry to TLS mode and subsequent imap activity on the account.
The solid lines show the successful path where possible stored injections are deleted and subsequent encrypted imap activity occurs.
|
|
Assignee | |
Comment 30•4 years ago
|
||
In ...v3.pgn, all lines appear "dashed". Maybe this looks some better. Also, corrected an error and added a few more details.
|
|
Assignee | |
Comment 31•4 years ago
|
||
I made one more change to the v3 diff inspired by Ben's comment:
SendData("a "); // Tag + Space for noop, continued belowI think this is the point where the negotiation is likely to actually occur. The send will block until the negotiation is performed, then the data will be sent over the new encrypted transport.
I'm pretty sure we'd need to check the error code here. If the negotiation fails (eg bad certificate, self-signed, whatever), this is where we'll first hear about it.
Since I'm no longer sending the tag, the negotiation fail will now be detected on the Capability(). So to detected it I had to add a return value to Capability() instead of returning void. So if Capability() fails it will now set rv which will log a negotiation failed message below. However, a normal user will still not be notified and tb will just fail to fetch anything. So maybe some kind of alert message should pop up too if STARTTLS nego failure detected?
|
|
||
Comment 32•4 years ago
|
||
(In reply to gene smith from comment #31)
Created attachment 9189053 [details] [diff] [review]
starttls-resp-insertion-v3.1.diff
That all looks pretty sane to me!
Since I'm no longer sending the tag, the negotiation fail will now be detected on the Capability(). So to detected it I had to add a return value to Capability() instead of returning void. So if Capability() fails it will now set rv which will log a negotiation failed message below. However, a normal user will still not be notified and tb will just fail to fetch anything. So maybe some kind of alert message should pop up too if STARTTLS nego failure detected?
The existing StartTLS() error handling is in: nsImapProtocol::CreateNewLineFromSocket(), which the imap state parser uses to feed itself.
The CreateNewLineFromSocket() checks for read failures, and if one occurs it:
- pops up an alert box, for some kinds of errors (It really shouldn't!)
- grabs the securityInfo from the transport and stashes it on the running Url (ugh, but required), so that the onStopRunningURL() callback can access it.
- calls
SetConnectionStatus()to set the error code on the connection (again, to pass out to onStopRunningURL()).
The onStopRunningURL() callback is where the outside code can detect and handle IMAP errors. So, for example, it can check to see if the status code is a recoverable NSS cert error (eg a server using a self-signed cert, say) and show an add-a-certificate-exception dialog box to the user. It's also where the GUI _should be popping up the alert boxes in part 1) above, but hey. One day :-)
So, to get back on topic... it's likely that correct error handling will already be performed by virtue of the Capabilty() using the standard read->parse mechanisms, which end up in nsImapProtocol::CreateNewLineFromSocket(). Off the top of my head I can't remember if the GUI-side onStopRunningURL handler pops up an alert for other unhandled errors... I suspect it might not because of 1). But that's probably a different bug.
|
|
Assignee | |
Comment 33•4 years ago
|
||
eg a server using a self-signed cert, say)
Maybe a bit off-topic but I still have my changes for this bug present and I'm not seeing an override dialog for a self-signed cert. See bug 1678792 comment 2.
|
|
Assignee | |
Comment 34•4 years ago
|
||
(In reply to gene smith from comment #33)
eg a server using a self-signed cert, say)
Maybe a bit off-topic but I still have my changes for this bug present and I'm not seeing an override dialog for a self-signed cert. See bug 1678792 comment 2.
Yes, that was the problem. The new return value I put on Capability() used down below caused the error to be logged and a m_transport->Close(). This must have inhibited the self-signed cert override dialog from popping up, I assume because I closed the connection.
I'm also attaching starttls-resp-insertion-v4.diff. This removes the previously added return on Capability() to avoid the problem of comment 33 above.
One thing I've noticed is that for imap when there is a non-overridable TLS error there is just complete silence. So I added an new "alert" to handle TLS errors that don't trigger an override dialog. It does work ok and pops up a general error about the problem but is not specific. I noticed that for SMTP the non-overridable error messages are very specific, e.g., "Your server has an old TLS version so fix it" etc. I guess this is part of the problem with "alerts" you talk about in comment 32. So, if what I did is not OK, I need a bit of help to fix it (if a fix is possible). Also, I'm leaving this in this diff for now but probably this alert stuff should go into a new bug.
|
|
Assignee | |
Comment 35•4 years ago
|
||
|
|
||
Comment 36•4 years ago
|
||
|
|
Assignee | |
Comment 37•4 years ago
|
||
https://searchfox.org/mozilla-central/source/security/manager/ssl/NSSErrorsService.h#46
Excellent! For some reason I assumed it was private. So I'll just call it directly.
|
|
Assignee | |
Comment 38•4 years ago
|
||
Note: The fix for TLS error notification (not security issue) is handled in Bug 1676141
This patch only addresses the security related issue identified for STARTTLS and last discussed in comment 32 and earlier in this bug report.
|
|
||
Comment 39•4 years ago
|
||
|
|
Assignee | |
Comment 40•4 years ago
|
||
Looks like
discardedis never used.
I'll fix that. In the previous diff attachment I passed it to a printf and should have removed it too when I removed the printf.
|
|
Assignee | |
Comment 41•4 years ago
|
||
Removed unused "discarded" variable.
|
|
||
Comment 42•4 years ago
|
||
|
|
Assignee | |
Updated•4 years ago
|
|
|
||
Comment 43•4 years ago
|
||
|
|
||
Updated•4 years ago
|
|
|
||
Comment 44•4 years ago
|
||
Comment on attachment 9193628 [details] [diff] [review]
Bug1622640-precludes-starttls-response-injection-v2.patch
[Approval Request Comment]
Sec issue. Could need beta baking.
|
||
Comment 45•4 years ago
|
||
Comment on attachment 9193628 [details] [diff] [review]
Bug1622640-precludes-starttls-response-injection-v2.patch
[Triage Comment]
Approved for beta
|
|
||
Comment 46•4 years ago
|
||
I suspect we will want more than 1 week on beta, so TM:78.7.0
|
||
Comment 47•4 years ago
|
||
|
|
||
Updated•4 years ago
|
|
|
||
Comment 48•4 years ago
|
||
reconfirmed, deferring to 78.7.0
|
|
||
Comment 49•4 years ago
•
|
||
Comment on attachment 9193628 [details] [diff] [review]
Bug1622640-precludes-starttls-response-injection-v2.patch
[Triage Comment]
Approved for esr78
Kaie, Needs text for CVE ?
|
|
||
Comment 50•4 years ago
|
||
For text we could use:
During the plaintext phase of the STARTTLS connection setup, protocol commands could be injected and get evaluated within the encrypted session.
Sorry I forget what the procedure is. Do we just put it in the bug like this?
Kai's away until the end of the month.
|
||
Comment 51•4 years ago
|
||
| uplift | ||
|
|
||
Updated•4 years ago
|
|
|
||
Comment 52•3 years ago
|
||
Hanno Böck send me the following information, which I'm relaying FYI:
This bug could become unrestricted soon. A bigger disclosure will be made in the next few days.
Damian, if you have worked with Hanno on this bug, would it make sense to add Hanno to CC?
|
|
Reporter | |
Comment 53•3 years ago
|
||
Hey, could you ask Mitre to
|
|
Reporter | |
Comment 54•3 years ago
|
||
(In reply to Damian Poddebniak from comment #53)
Hey, could you ask Mitre to
Sorry, I added Hanno to this bug. Seems like Bugzilla published the message draft I was about to post when clicking on the "Add" button in the CC field? Nevermind for now :-)
|
||
Comment 55•3 years ago
|
||
The bug has been fixed for over half a year and the paper has been released to the public.
Any objections to opening this bug up for public consumption?
|
|
||
Updated•3 years ago
|
Description
•