[CONC-648] Do not trust error packets received from the server prior to TLS handshake completion #223
Conversation
|
This commit message contains a |
dlenski
force-pushed
the
fix_CONC-648_vulnerability
branch
2 times, most recently
from
7400409 to
abad64b
Compare
dlenski
changed the title
dlenski
force-pushed
the
fix_CONC-648_vulnerability
branch
from
abad64b to
777e956
Compare
|
I think this is ok. Rewriting the error to @9E0R9, what do you think? |
|
I was just going to merge, but wanted to check the code in the context first. Few lines below there's /* Check if server sends an error */
if (mysql->protocol_version == 0XFF)
{
net_get_error(end, pkt_length - 1, net->last_error, sizeof(net->last_error),
&net->last_errno, net->sqlstate);
/* fix for bug #26426 */
if (net->last_errno == 1040)
memcpy(net->sqlstate, "08004", SQLSTATE_LENGTH);
goto error;
}does it need to be fixed similarly? |
|
@vuvova wrote:
You're referring specifically to these lines, right? mariadb-connector-c/libmariadb/mariadb_lib.c Lines 1811 to 1820 in 777e956 That is completely redundant dead code, and has been forever (not as a result of this PR):
In general, there is a great deal of redundant code and confusing naming* in * For example… |
Based on Sergei Golubchik's question in mariadb-corporation#223 (comment)
|
updated missing possible Error packet in documentation https://mariadb.com/kb/en/connection/ |
|
Right, I agree, it's dead code. Instead of a comment, I'd rather remove it and add an assert instead. Also, what if the connection is actually dropped by the server? Not an error message, but network error or something. It'd be less confusing to get CR_SERVER_LOST instead of your new "The authenticity of the following error cannot be verified". Because it actually can, can it not? Meaning, I can apply this PR, but I'd prefer to put your Would that be ok for you? I'll of course keep the attribution. Or would you prefer to do these changes yourself? Or do you disagree with this approach completely? |
|
@vuvova wrote:
As you can see from 8529511, I did both. I added a
I had very intentionally chosen this order. Two (related) reasons:
|
yes, I know you did both. I'm not a great fan of having dead code laying around, so I would prefer only the assert.
I checked the code again. May be I'm missing something, but I don't see how CR_SERVER_LOST can be a result of any data sent by the server. It looks like CR_SERVER_LOST can only be caused by a network error. |
What does "only the assert" mean?
The fact that a MITM can inject a pre-TLS error packet, which appears to have been sent by the server, is precisely the point of CONC-648. You can test this yourself by building a trivially-modified server (dlenski/mariadb-server@demonstration_of_CONC-648_vulnerability), and you'll see that you can inject If I modify the order of the |
|
By "only the assert" I meant something like @@ -1523,20 +1523,10 @@ MYSQL *mthd_my_real_connect(MYSQL *mysql, const char
/* Check if version of protocol matches current one */
mysql->protocol_version= end[0];
end++;
+ assert(mysql->protocol_version != 0xFF);
- /* Check if server sends an error */
- if (mysql->protocol_version == 0XFF)
- {
- net_get_error(end, pkt_length - 1, net->last_error, sizeof(net->last_error>
- &net->last_errno, net->sqlstate);
- /* fix for bug #26426 */
- if (net->last_errno == 1040)
- memcpy(net->sqlstate, "08004", SQLSTATE_LENGTH);
- goto error;
- }
-
if (mysql->protocol_version < PROTOCOL_VERSION)
{
net->last_errno= CR_VERSION_ERROR;but when I was writing it I reconsidered. So, now I'd say — let's just remove that
Hmm, indeed. I don't think the client should accept client-side errors from the server at all, pre-TLS or not. Something like @@ -235,6 +235,13 @@ ma_net_safe_read(MYSQL *mysql)
}
goto restart;
}
+ if (last_errno >= 2000 && last_errno < 3000 ||
+ last_errno >= 5000 && last_errno < 6000)
+ {
+ my_set_error(mysql, CR_MALFORMED_PACKET, SQLSTATE_UNKNOWN, 0);
+ }
+ else
+ {
net->last_errno= last_errno;
if (pos[0]== '#')
{
@@ -248,6 +255,7 @@ ma_net_safe_read(MYSQL *mysql)
ma_strmake(net->last_error,(char*) pos,
min(len,sizeof(net->last_error)-1));
+ }
}
else
{
my_set_error(mysql, CR_UNKNOWN_ERROR, SQLSTATE_UNKNOWN, 0); |
Per @vuvova in mariadb-corporation#223 (comment): > I don't think the client should accept client-side errors from the server > at all. If the server sends an error packet with error codes in the ranges `CR_{MIN,MAX}_ERROR` (codes [2000, 2999]) or `CER_{MIN,MAX}_ERROR` (codes [5000, 5999]), we will replace these with `CR_MALFORMED_PACKET`, rather than propagating them to the client user.
… completion MariaDB Connector/C does not distinguish [application-layer error packets](https://mariadb.com/kb/en/err_packet) that it receives prior to TLS handshake completion from those that it receives immediately after. (A trivially modified server built from dlenski/mariadb-server@demonstration_of_CONC-648_vulnerability can easily be used to demonstrate this.) Pre-TLS error packet received from this trivially modified server. This packet should NOT be trusted to actually originate from the server: $ mariadb --ssl --ssl-verify-server-cert -uUsername -pVerySecretPassword -h CONC-648.vuln.demo.server.com ERROR 1815 (HY000): Internal error: Client will accept this error as genuine even if running with --ssl --ssl-verify-server-cert, and even though this error is sent in plaintext PRIOR TO TLS HANDSHAKE. Post-(TLS handshake) error packet received from a normal MariaDB server upon an attempt to connect with incorrect credentials. This error packet CAN be trusted to actually originate from the server, assuming transitive trust in the TLS protocol implementation and PKI-based certificate validation: $ mariadb --ssl --ssl-verify-server-cert -uUsername -pWrongPassword -h $NORMAL_MARIADB10.6.14_SERVER ERROR 1045 (28000): Access denied for user 'Username'@'A.B.C.D' (using password: YES) This client behavior opens up MariaDB Connector/C clients to an extremely straightforward [downgrade attack](https://en.wikipedia.org/wiki/Downgrade_attack). An on-path or pervasive attacker can inject errors into MariaDB client→server connections that are intended to be protected by TLS, and the client has no clear mechanism to distinguish such errors from errors that actually come from the server. An attacker could easily use this to DOS a client, or even influence its behavior. For example, consider a client application which is configured… 1. To use TLS with server certificate validation (`--ssl --ssl-verify-server-cert`), and 2. To wait for a back-off period and then *retry* connection attempts if the server responds with `ER_CON_COUNT_ERROR` ("Too many connections") from the server, and 3. To give up and shut down if its connection attempts fail with `ER_ACCESS_DENIED_ERROR` ("Access denied for user"), on the assumption that this is due to an incorrect or expired password, and cannot be resolved without human intervention. An attacker could completely disable the retry mechanism of this application by intercepting connection attempts and replying with `ER_ACCESS_DENIED_ERROR` packets. This patch modifies MariaDB Connector/C so that if the client is configured to use TLS, error packets received prior to the completion of the TLS handshake are untrusted, and are changed to a generic `CR_CONNECTION_ERROR`. $ mariadb --ssl --ssl-verify-server-cert -uUsername -pVerySecretPassword -h CONC-648.vuln.demo.server.com ERROR 2002 (HY000): Received error packet before completion of TLS handshake. The authenticity of the following error cannot be verified: 1815 - Internal error: Client will accept this error as genuine even if running with --ssl --ssl-verify-server-cert, and even though this error is sent in plaintext PRIOR TO TLS HANDSHAKE. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
Based on Sergei Golubchik's question about this code section in mariadb-corporation#223 (comment), eventually culminating in the conclusion that it's literally impossible to reach this code section based on the types and signedess of the variables involved: mariadb-corporation#223 (comment) All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
dlenski
force-pushed
the
fix_CONC-648_vulnerability
branch
from
8529511 to
ce7fc39
Compare
|
@vuvova wrote:
Great catch that it's literally impossible by the variable types alone. 🙌 Done, removed entirely in ce7fc39.
Great, that makes things much simpler!
|
| @@ -241,18 +241,30 @@ ma_net_safe_read(MYSQL *mysql) | |||
| } | |||
| goto restart; | |||
| } | |||
| net->last_errno= last_errno; | |||
| if (pos[0]== '#') | |||
| if (last_errno >= CR_MIN_ERROR && last_errno <= CR_MAX_ERROR || | |||
There was a problem hiding this comment.
How about:
if (IS_MYSQL_ERROR(last_errno) || IS_MARIADB_ERROR(last_errno)) ?
There was a problem hiding this comment.
This is needed to fix travis builds, otherwise the code doesn't compile with -Werror. But I don't want to drag this any longer, I'll merge and fix afterwards with a separate commit
There was a problem hiding this comment.
How about:
if (IS_MYSQL_ERROR(last_errno) || IS_MARIADB_ERROR(last_errno)) ?
Makes sense to simplify the code, but per the definition of the IS_MYSQL_ERROR macro, this would not be 100% equivalent.
- Current version rejects error codes [2000, 2999] + [5000, 5999].
- @9EOR9's proposed replacement would reject only error codes [2000, 2061] + [5000, 5023].
Because these error codes are client-side-only, they should be equivalent as long as no one forgets to update CR_MYSQL_LAST_ERROR and CR_MARIADB_LAST_ERROR.
There was a problem hiding this comment.
Consider modifying the IS_MYSQL_ERROR and IS_MARIADB_ERROR macros so that they include the entire client-side error ranges, rather than depending on CR_MYSQL_LAST_ERROR and CR_MARIADB_LAST_ERROR being updated.
There was a problem hiding this comment.
That wouldn't hurt, but I'll leave it to @9EOR9. It doesn't matter much whether we test the full possible range or the actual range only, All error codes within the range we test can be considered safe, not injected by MitM, but generated here in the client library. All codes outside of that range are unsafe (before SSL). So as long as the code never trusts error message numbers outside of the tested range, it should be good.
Per @vuvova in #223 (comment): > I don't think the client should accept client-side errors from the server > at all. If the server sends an error packet with error codes in the ranges `CR_{MIN,MAX}_ERROR` (codes [2000, 2999]) or `CER_{MIN,MAX}_ERROR` (codes [5000, 5999]), we will replace these with `CR_MALFORMED_PACKET`, rather than propagating them to the client user.
MariaDB Connector/C does not distinguish application-layer error
packets that it receives prior to TLS
handshake completion from those that it receives immediately after.
(A trivially modified server built from
dlenski/mariadb-server@demonstration_of_CONC-648_vulnerability
can easily be used to demonstrate this.)
Pre-TLS error packet received from this trivially modified server. This packet
should NOT be trusted to actually originate from the server:
Post-(TLS handshake) error packet received from a normal MariaDB server upon
an attempt to connect with incorrect credentials. This error packet CAN be
trusted to actually originate from the server, assuming transitive trust in
the TLS protocol implementation and PKI-based certificate validation:
This client behavior opens up MariaDB Connector/C clients to an extremely
straightforward downgrade attack.
An on-path or pervasive attacker can inject errors into MariaDB
client→server connections that are intended to be protected by TLS, and the
client has no clear mechanism to distinguish such errors from errors that
actually come from the server.
An attacker could easily use this to DOS a client, or even influence its
behavior. For example, consider a client application which is configured…
(
--ssl --ssl-verify-server-cert), andresponds with
ER_CON_COUNT_ERROR("Too many connections") from theserver, and
ER_ACCESS_DENIED_ERROR("Access denied for user"), on the assumptionthat this is due to an incorrect or expired password, and cannot be
resolved without human intervention.
An attacker could completely disable the retry mechanism of this application
by intercepting connection attempts and replying with
ER_ACCESS_DENIED_ERRORpackets.This patch modifies MariaDB Connector/C so that if the client is configured
to use TLS, error packets received prior to the completion of the TLS
handshake are untrusted, and are changed to a generic
CR_CONNECTION_ERROR.