You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Laurent Goujon <la...@dremio.com> on 2017/11/01 00:42:44 UTC
Re: Drill SASL Forward Compatibility
Regarding DRILL-5582 patch which broke compatibility with 1.9 version
(which is less than a year old):
I'm still not clear what we are trying to prevent here: stealing user
credentials and/or data? connecting to a rogue server which could return
corrupted data to the user? The patch gives a false sense of security
because it prevents none of it:
- if using password-based authentication, client is sending it in clear in
the initial handshake so I guess it's already game over!
- You could configure a client to not sent password over wire by choosing
another authentication algorithm, BUT if there's no encryption of the
channel, any data can be intercepted and rewritten. And of course, the
rogue server could actually run queries on behalf of the user...
- if using Kerberos, things are a bit different: even if a MITM intercepts
the token, only the server can decode it properly and set up encryption so
that only client can use it (a proxy server would not be able to decode the
traffic). So what you need to ensure is that you actually use Kerberos as
the only authentication mechanism, and that the channel is encrypted (if
channel is not encryted, see above). This is things you should do by
configuring the client by not sending the password (no need to), only
authorize Kerberos authentication, and verify that encryption is enabled
(which is already done I believe).
For comparison, HTTP protocol (using it since it is one of the most used
public protocol) has no issue with client sending an authentication header
to a remote server, without knowing based on the server response if
authentication happened.
As for the other issue at hand (compatibility between 1.11 client and 1.10
server), I am not sure to understand the proposed fix: is the logic you
proposed to be added to 1.10 server? why not simply add the missing enum
value (and that's it! no more values after that!)?
Laurent
On Tue, Oct 31, 2017 at 3:12 PM, Sorabh Hamirwasia <sh...@mapr.com>
wrote:
> Hi Laurent,
>
> We are preventing 2 cases here:
>
> * False positive for successful authentication. Even though MITM
> attack can happen after successful authentication, but client and server
> involved here has ensure successful authentication handshake has taken
> place. With the security flaw there can always be false positive on client
> side thinking authentication was successful even though that might not be
> the case.
> * False positive for successful handshake with encryption capability:
> In cases when server is properly configured to support encryption, MITM
> attack tweaking handshake response and making client to believe that after
> successful handshake all communications will be encrypted is again another
> bad false positive.
>
> IMHO Drill Client should be able to validate when server says that
> authentication is completed then it's actually completed, at least until
> the point SASL Handshake is initiated, rather than blindly trusting it.
> This is because if Drill client doesn't guarantees that, then it's making
> the support for protocol like Kerberos weaker which prevent's from any MITM
> attack at handshake level. Whereas mechanisms like PLAIN are still prone to
> MITM even during SASL handshake.
>
> As far as forward compatibility is concerned there are few things:
>
> * AFAIK DrillClient & Drillbit doesn't have any concept of supporting
> different RPC versions across releases.They are forced to talk on same RPC
> versions else connection will fail. Once we have that I think then we will
> be able to clearly justify or provide the matrix of backward and forward
> compatibility across releases.
> * We can put the check based on supported_methods to detect if server
> side supports SASL or not. But this is again just a work around not proper
> solution. With workaround there can still be similar security holes as
> PLAIN mechanism itself is prone to MITM. Given 1.9 is 3 releases behind
> now, not sure if we still want to support that combination.
> * At least for compatibility between Drill 1.11 client and Drill 1.10
> server, I think the fix should be made which is mentioned in first email of
> this thread.
>
> Thanks,
> Sorabh
>
> ________________________________
> From: Laurent Goujon <la...@dremio.com>
> Sent: Tuesday, October 31, 2017 9:38:13 AM
> To: dev
> Cc: Arina Lelchieva; sudheesh@apache.org
> Subject: Re: Drill SASL Forward Compatibility
>
> See my answers inline.
>
> On Tue, Oct 31, 2017 at 1:40 AM, Sorabh Hamirwasia <sh...@mapr.com>
> wrote:
>
> > Hi Laurent,
> >
> > Thanks for pointing issue with <= 1.9 version Drillbit, I looked into
> > supported_methods field and it doesn't advertise SASL support using that
> > [1][2]. Do you mean that checking if supported_methods list is non-empty
> > should suffice since it was introduced in 1.10 ?
> >
>
> My bad, I thought SASL_MESSAGE was added to the list of supported methods,
> but it's not. Alternatively you could check for authenticationMechanisms
> which should be not empty if version >= 1.10 and authentication is turned
> on.
>
>
> >
> >
> > For security flaw let's consider an example. Let say client is connecting
> > to a Drillbit with authentication (and or encryption) enabled for
> Kerberos
> > mechanism. The message flow will happen something like below:
> >
> >
> > Good Case:
> >
> > * DrillClient sends Handshake Request to Drillbit
> > * Drillbit sends the response back to DrillClient with AUTH_REQD as
> > status
> > * DrillClient exchange SASL handshake messages with Drillbit.
> > * Once handshake is successful DrillClient is connected to secure
> > Drillbit.
> > * App using DrillClient has actually established a connection to
> > secure Drillbit with authentication (and or encryption) and can submit
> it's
> > query.
> >
> > Bad Case:
> >
> > * Step 1 as above
> > * Step 2 as above. This message was intercepted by MITM and status
> was
> > changed to SUCCESS.
> > * Without the recent change DrillClient will not initiate SASL
> > handshake and will return connection successful.
> > * App using DrillClient will think it has successfully connected to
> > secure Drillbit which is NOT the case.
> >
>
> What are we preventing in the bad case? if this is credentials or data
> interception, a MITM could simply act as proxy to intercept all of it since
> traffic is not encrypted. If we were to prevent the loss of credentials,
> the solution to avoid transmitting the credentials in clear in the first
> place. For that, we don't need a protocol change but:
> - disable plain authentication, and use something like Kerberos or
> CRAM-MD5/SCRAM
> - make sure the password is not sent in the initial handshake (if using
> Kerberos, there should no credentials to send over in the first place)
>
>
> >
> > I think what you are pointing out is even in good case and in
> > authentication only scenario, even if connection is successful, the
> > messages between DrillClient and Drillbit can still be intercepted since
> > they will be in plain text. The only way to avoid that is using
> encryption.
> > But the fix was more of to avoid wrong behavior in that case too where
> > connection should fail, instead of client just relying on server
> response.
> >
>
> The "wrong" behavior was what allowed for compatibility with older servers
> in the original design...
>
>
> >
> > [1]: https://github.com/apache/drill/blob/1.11.0/exec/java-
> > exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java#L254
> > [2]: https://github.com/apache/drill/blob/1.11.0/exec/java-
> > exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java#L89
> >
> >
> > Thanks,
> > Sorabh
> >
> > ________________________________
> > From: Laurent Goujon <la...@dremio.com>
> > Sent: Monday, October 30, 2017 5:47 PM
> > To: dev
> > Cc: Arina Lelchieva; sudheesh@apache.org
> > Subject: Re: Drill SASL Forward Compatibility
> >
> > Regarding DRILL-5582, I see that fix as a breakage of the work to
> maintain
> > compatibility for an newer client to connect to a older version of the
> > server. Or put it differently: current (master) client does not connect
> > anymore to a server not supporting SASL (<=1.9). Note that the client
> could
> > detect if the server supports SASL as it is advertised in the
> > supported_methods field of the BitToUserHandshake, and it would restore
> > compatibility, but it seems the fix was done in response to a potential
> > security flaw (although I have to admin not sure what issue it does
> prevent
> > since it is still possible for a MITM to intercept all traffic between a
> > client and a server).
> >
> > Laurent
> >
> > On Mon, Oct 30, 2017 at 5:18 PM, Sorabh Hamirwasia <shamirwasia@mapr.com
> >
> > wrote:
> >
> > > Hi All,
> > >
> > > We recently added a check (as part of DRILL-5582<https://issues.
> > > apache.org/jira/browse/DRILL-5582>) on DrillClient side to enforce
> that
> > > if client showed intent for authentication and Drillbit say's it
> doesn't
> > > require authentication then connection will fail with proper error
> > message.
> > >
> > >
> > > With this change we found a hidden issue w.r.t forward compatibility of
> > > Drill. New clients running on 1.11+ authenticating to older Drillbit
> > > running on 1.10 are treated as client running without any SASL support
> or
> > > (<=1.9 version). The root cause for this issue is as follows:
> > >
> > >
> > > In 1.10 a new field SASL_SUPPORT was introduced in Handshake message
> > > between DrillCilent and Drillbit. The supported values for that field
> > are:
> > >
> > >
> > > Drill 1.10: [1]
> > >
> > >
> > > enum SASL_SUPPORT {
> > > UNKNOWN_SASL_SUPPORT = 0;
> > > SASL_AUTH = 1;
> > > }
> > >
> > >
> > > Drill 1.11/1.12: [2]
> > >
> > >
> > > enum SASL_SUPPORT {
> > > UNKNOWN_SASL_SUPPORT = 0;
> > > SASL_AUTH = 1;
> > > SASL_PRIVACY = 2;
> > > }
> > >
> > > A 1.11 client always has SASL_PRIVACY set in handshake. A 1.10 Drillbit
> > > getting the message interprets the value as UNKNOWN_SASL_SUPPORT [3].
> In
> > > 1.10 Drillbit treats that value as an indication of older client < 1.10
> > > [4], and it will try to authenticate using the 1.9 mechanism and send
> the
> > > SUCCESS/FAILURE state in Handshake Response. The 1.12 DrillClient will
> > fail
> > > the connection as it will expect AUTH_REQUIRED from Drillbit instead of
> > > SUCCESS/FAILURE.
> > >
> > >
> > > The fix for this issue can be to use only absence of field as
> indication
> > > of client < 1.10 and if the field is present but it evaluates to
> > > UNKNOWN_SASL_SUPPORT value then Drillbit should consider corresponding
> > > client to be of future version at least for authentication purpose and
> > > speak SASL protocol.
> > >
> > > We have to either back port above forward compatibility fix into 1.10
> and
> > > 1.11 or just fix in current release and support forward compatibility
> > post
> > > 1.12+.
> > >
> > >
> > > Arina/Sudheesh - Please suggest if the analysis and fix sounds good and
> > > what are the releases we should consider the fix for. Considering 1.12
> > > release is planned soon can we take the fix in 1.12 release ?
> > >
> > >
> > >
> > > [1]: https://github.com/apache/drill/blob/1.10.0/protocol/
> > > src/main/protobuf/User.proto#L70
> > >
> > > [2]: https://github.com/apache/drill/blob/1.11.0/protocol/
> > > src/main/protobuf/User.proto#L70
> > >
> > > [3]: http://androiddevblog.com/protocol-buffers-pitfall-
> > > adding-enum-values/
> > >
> > > [4]: https://github.com/apache/drill/blob/1.10.0/exec/java-
> > > exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java#L297
> > >
> > >
> > > Thanks,
> > > Sorabh
> > >
> >
>
Re: Drill SASL Forward Compatibility
Posted by Laurent Goujon <la...@dremio.com>.
If encryption is not enabled, any MITM can intercept and inject traffic in
the session (meaning it could also do requests on behalf of the users
without the user noticing), even with Kerberos.
On Wed, Nov 1, 2017 at 2:13 PM, Sorabh Hamirwasia <sh...@mapr.com>
wrote:
> Parth,
>
> Your understanding is correct.
>
>
> I am also debating for 1(A) specially in context of security mechanisms
> like Kerberos which guarantees prevention from MITM during handshake.
>
>
> But with 1( B ) we are saying in case of Drill it's possible and fine
> since no data is compromised.
>
>
> Thanks,
> Sorabh
>
> ________________________________
> From: Parth Chandra <pa...@apache.org>
> Sent: Wednesday, November 1, 2017 1:42:14 PM
> To: dev
> Subject: Re: Drill SASL Forward Compatibility
>
> I sort of lost track of the arguments in the thread. Is my understanding
> below correct ?
>
>
> 1) A handshake from a (1.12) client expecting authentication and encryption
> is intercepted by a rogue server. The server then responds with a success
> message and bypasses the auth and encryption for the session.
>
> 2) The client is now connected, but not to the server it wanted to connect
> to.
>
> 3) The rogue server can now feed any bogus response to the client.
>
>
> Question 1 - Is #3 a security issue?
>
>
> Answer 1 (A) - Yes. The handshake has been compromised. The client is no
> longer connected to an authentic server.
>
>
> Answer 1 (B) - No. There is no data that has been compromised. Just a
> client that has been misled.
>
>
>
> I believe this is a security issue. A rogue server can now feed invalid
> results to the client and that is not safe. Perhaps others with more
> experience on industrial grade security can chime in.
>
> Question 2 - If this is a security issue, is it severe enough to break
> forward compatibility?
>
> In general, I'm -1 on breaking backward compatibility and -0 on breaking
> forward compatibility. I believe it is a very desirable goal to maintain
> both backward and forward compatibility. However, forward compatibility
> cannot be guaranteed unless we bake it into the RPC protocol and design
> clients to be version and feature aware. This itself would be a breaking
> change and should be one of the goals for V2.
>
> In this case, I'm inclined to go with what Arina is suggesting.
>
Re: Drill SASL Forward Compatibility
Posted by Sorabh Hamirwasia <sh...@mapr.com>.
Parth,
Your understanding is correct.
I am also debating for 1(A) specially in context of security mechanisms like Kerberos which guarantees prevention from MITM during handshake.
But with 1( B ) we are saying in case of Drill it's possible and fine since no data is compromised.
Thanks,
Sorabh
________________________________
From: Parth Chandra <pa...@apache.org>
Sent: Wednesday, November 1, 2017 1:42:14 PM
To: dev
Subject: Re: Drill SASL Forward Compatibility
I sort of lost track of the arguments in the thread. Is my understanding
below correct ?
1) A handshake from a (1.12) client expecting authentication and encryption
is intercepted by a rogue server. The server then responds with a success
message and bypasses the auth and encryption for the session.
2) The client is now connected, but not to the server it wanted to connect
to.
3) The rogue server can now feed any bogus response to the client.
Question 1 - Is #3 a security issue?
Answer 1 (A) - Yes. The handshake has been compromised. The client is no
longer connected to an authentic server.
Answer 1 (B) - No. There is no data that has been compromised. Just a
client that has been misled.
I believe this is a security issue. A rogue server can now feed invalid
results to the client and that is not safe. Perhaps others with more
experience on industrial grade security can chime in.
Question 2 - If this is a security issue, is it severe enough to break
forward compatibility?
In general, I'm -1 on breaking backward compatibility and -0 on breaking
forward compatibility. I believe it is a very desirable goal to maintain
both backward and forward compatibility. However, forward compatibility
cannot be guaranteed unless we bake it into the RPC protocol and design
clients to be version and feature aware. This itself would be a breaking
change and should be one of the goals for V2.
In this case, I'm inclined to go with what Arina is suggesting.
Re: Drill SASL Forward Compatibility
Posted by Sorabh Hamirwasia <sh...@mapr.com>.
Hi Laurent,
Please see the responses inline.
Thanks,
Sorabh
________________________________
From: Laurent Goujon <la...@dremio.com>
Sent: Thursday, November 2, 2017 11:52 AM
To: dev
Subject: Re: Drill SASL Forward Compatibility
I have a parallel scenario:
- Scenario 1:
1) A handshake from a (1.12) client expecting authentication and encryption
is intercepted by a rogue server. The rogue server then responds first with
AUTH_REQUIRED, but authenticationMechanisms doesn't provide gssapi/kerberos
as a sasl mechanism. The client accepts another authentication mechanism,
and authenticate with the rogue server, the server send a SASL_SUCCESS
message and bypasses the auth and encryption for the session.
[Sorabh] - Should be mechanism dependent. When client/server get's SASL_SUCCESS message then they evaluate the bits sent by
other side to verify if the underlying mechanism implementation determine the handshake as complete or not. If yes then only handshake is
treated as complete otherwise handshake fails.
https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/AuthenticationOutcomeListener.java#L203
2) The client is now connected, but not to the server it wanted to connect
to.
3) The rogue server can now feed any bogus response to the client.
Question:
- is it the same situation as the scenario proposed by Parth where the
rogue server was directly sending SUCCESS?
I personally consider it is the same, and that is a potential security
issue, the same way phishing is a security issue, and can be used to misled
users
- is it possible for both these scenario to happen if client actually sets
DrillProperties.SASL_ENCRYPT to true, meaning it would only allow to
complete handshake if encryption is actually not set up properly?
Answer for this question is likely to be no since both C++ and Java clients
check if encryption has been set up, and will fail if not
[Sorabh] - Correct. Its not possible in this case.
*
https://github.com/apache/drill/blob/b0c4e0486d6d4620b04a1bb8198e959d433b4840/exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java#L238
*
https://github.com/apache/drill/blob/b0c4e0486d6d4620b04a1bb8198e959d433b4840/contrib/native/client/src/clientlib/drillClientImpl.cpp#L614
My suggestions would be:
- client should be able to select which SASL mechanisms to allow or not.
Client could choose mechanisms which only allow for mutual authentication.
[Sorabh] - Client's can do that by auth parameter in connection string.
If client requires encryption, client could only allow for mechanisms
allowing encryption. C++ client seems to support any mechanism registered
in SASL library. Java client has PLAIN and KERBEROS always enabled, and can
add other custom mechanims, but it is not possible to disable PLAIN!
[Sorabh] - The internal SASL library (both Java side and Cyrus SASL plugin) take's care of this while creating SASL client instance.
They check if the requested mechanism meets all the security properties passed by client or not.
Example: On Java: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/com/sun/security/sasl/ClientFactoryImpl.java#92
- modify the check introduced with DRILL-5582 to check if client wants to
use PLAIN authentication. If client wants so, allow to connect it to older
servers. If not, disable PLAIN mechanism in SASL, and yes, make sure you go
through a SASL authentication.
[Sorabh] - I will modify the check to allow for PLAIN mechanism since in that case we are not weakening it any more. But keep the checks for
other mechanisms.
Laurent
On Wed, Nov 1, 2017 at 1:42 PM, Parth Chandra <pa...@apache.org> wrote:
> I sort of lost track of the arguments in the thread. Is my understanding
> below correct ?
>
>
> 1) A handshake from a (1.12) client expecting authentication and encryption
> is intercepted by a rogue server. The server then responds with a success
> message and bypasses the auth and encryption for the session.
>
> 2) The client is now connected, but not to the server it wanted to connect
> to.
>
> 3) The rogue server can now feed any bogus response to the client.
>
>
> Question 1 - Is #3 a security issue?
>
>
> Answer 1 (A) - Yes. The handshake has been compromised. The client is no
> longer connected to an authentic server.
>
>
> Answer 1 (B) - No. There is no data that has been compromised. Just a
> client that has been misled.
>
>
>
> I believe this is a security issue. A rogue server can now feed invalid
> results to the client and that is not safe. Perhaps others with more
> experience on industrial grade security can chime in.
>
> Question 2 - If this is a security issue, is it severe enough to break
> forward compatibility?
>
> In general, I'm -1 on breaking backward compatibility and -0 on breaking
> forward compatibility. I believe it is a very desirable goal to maintain
> both backward and forward compatibility. However, forward compatibility
> cannot be guaranteed unless we bake it into the RPC protocol and design
> clients to be version and feature aware. This itself would be a breaking
> change and should be one of the goals for V2.
>
> In this case, I'm inclined to go with what Arina is suggesting.
>
Re: Drill SASL Forward Compatibility
Posted by Laurent Goujon <la...@dremio.com>.
I have a parallel scenario:
- Scenario 1:
1) A handshake from a (1.12) client expecting authentication and encryption
is intercepted by a rogue server. The rogue server then responds first with
AUTH_REQUIRED, but authenticationMechanisms doesn't provide gssapi/kerberos
as a sasl mechanism. The client accepts another authentication mechanism,
and authenticate with the rogue server, the server send a SASL_SUCCESS
message and bypasses the auth and encryption for the session.
2) The client is now connected, but not to the server it wanted to connect
to.
3) The rogue server can now feed any bogus response to the client.
Question:
- is it the same situation as the scenario proposed by Parth where the
rogue server was directly sending SUCCESS?
I personally consider it is the same, and that is a potential security
issue, the same way phishing is a security issue, and can be used to misled
users
- is it possible for both these scenario to happen if client actually sets
DrillProperties.SASL_ENCRYPT to true, meaning it would only allow to
complete handshake if encryption is actually not set up properly?
Answer for this question is likely to be no since both C++ and Java clients
check if encryption has been set up, and will fail if not
*
https://github.com/apache/drill/blob/b0c4e0486d6d4620b04a1bb8198e959d433b4840/exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java#L238
*
https://github.com/apache/drill/blob/b0c4e0486d6d4620b04a1bb8198e959d433b4840/contrib/native/client/src/clientlib/drillClientImpl.cpp#L614
My suggestions would be:
- client should be able to select which SASL mechanisms to allow or not.
Client could choose mechanisms which only allow for mutual authentication.
If client requires encryption, client could only allow for mechanisms
allowing encryption. C++ client seems to support any mechanism registered
in SASL library. Java client has PLAIN and KERBEROS always enabled, and can
add other custom mechanims, but it is not possible to disable PLAIN!
- modify the check introduced with DRILL-5582 to check if client wants to
use PLAIN authentication. If client wants so, allow to connect it to older
servers. If not, disable PLAIN mechanism in SASL, and yes, make sure you go
through a SASL authentication.
Laurent
On Wed, Nov 1, 2017 at 1:42 PM, Parth Chandra <pa...@apache.org> wrote:
> I sort of lost track of the arguments in the thread. Is my understanding
> below correct ?
>
>
> 1) A handshake from a (1.12) client expecting authentication and encryption
> is intercepted by a rogue server. The server then responds with a success
> message and bypasses the auth and encryption for the session.
>
> 2) The client is now connected, but not to the server it wanted to connect
> to.
>
> 3) The rogue server can now feed any bogus response to the client.
>
>
> Question 1 - Is #3 a security issue?
>
>
> Answer 1 (A) - Yes. The handshake has been compromised. The client is no
> longer connected to an authentic server.
>
>
> Answer 1 (B) - No. There is no data that has been compromised. Just a
> client that has been misled.
>
>
>
> I believe this is a security issue. A rogue server can now feed invalid
> results to the client and that is not safe. Perhaps others with more
> experience on industrial grade security can chime in.
>
> Question 2 - If this is a security issue, is it severe enough to break
> forward compatibility?
>
> In general, I'm -1 on breaking backward compatibility and -0 on breaking
> forward compatibility. I believe it is a very desirable goal to maintain
> both backward and forward compatibility. However, forward compatibility
> cannot be guaranteed unless we bake it into the RPC protocol and design
> clients to be version and feature aware. This itself would be a breaking
> change and should be one of the goals for V2.
>
> In this case, I'm inclined to go with what Arina is suggesting.
>
Re: Drill SASL Forward Compatibility
Posted by Parth Chandra <pa...@apache.org>.
I sort of lost track of the arguments in the thread. Is my understanding
below correct ?
1) A handshake from a (1.12) client expecting authentication and encryption
is intercepted by a rogue server. The server then responds with a success
message and bypasses the auth and encryption for the session.
2) The client is now connected, but not to the server it wanted to connect
to.
3) The rogue server can now feed any bogus response to the client.
Question 1 - Is #3 a security issue?
Answer 1 (A) - Yes. The handshake has been compromised. The client is no
longer connected to an authentic server.
Answer 1 (B) - No. There is no data that has been compromised. Just a
client that has been misled.
I believe this is a security issue. A rogue server can now feed invalid
results to the client and that is not safe. Perhaps others with more
experience on industrial grade security can chime in.
Question 2 - If this is a security issue, is it severe enough to break
forward compatibility?
In general, I'm -1 on breaking backward compatibility and -0 on breaking
forward compatibility. I believe it is a very desirable goal to maintain
both backward and forward compatibility. However, forward compatibility
cannot be guaranteed unless we bake it into the RPC protocol and design
clients to be version and feature aware. This itself would be a breaking
change and should be one of the goals for V2.
In this case, I'm inclined to go with what Arina is suggesting.
Re: Drill SASL Forward Compatibility
Posted by Arina Yelchiyeva <ar...@gmail.com>.
Hi Sorabh,
regarding your question:
>
> We have to either back port above forward compatibility fix into 1.10 and
> 1.11 or just fix in current release and support forward compatibility post
> 1.12+.
> Arina/Sudheesh - Please suggest if the analysis and fix sounds good and
> what are the releases we should consider the fix for. Considering 1.12
> release is planned soon can we take the fix in 1.12 release ?
>
I think we add the fix (if needed) in 1.12 and support forward
compatibility post 1.12+. As far as I know Drill never claimed it is
backward compatible and explicitly discourages users to use different Drill
versions in a cluster as well as for client and server.
Kind regards
Arina
On Wed, Nov 1, 2017 at 5:48 PM, Laurent Goujon <la...@dremio.com> wrote:
> My comments inline:
>
> On Tue, Oct 31, 2017 at 6:11 PM, Sorabh Hamirwasia <sh...@mapr.com>
> wrote:
>
> > - if using Kerberos, things are a bit different: even if a MITM
> intercepts
> >
> > the token, only the server can decode it properly and set up encryption
> so
> > that only client can use it (a proxy server would not be able to decode
> the
> > traffic). So what you need to ensure is that you actually use Kerberos as
> > the only authentication mechanism, and that the channel is encrypted (if
> > channel is not encryted, see above). This is things you should do by
> > configuring the client by not sending the password (no need to), only
> > authorize Kerberos authentication, and verify that encryption is enabled
> > (which is already done I believe).
> >
> >
> > [Sorabh] - You are correct about the Kerberos preventing MITM which is
> > what I mentioned in the last response. But this is guaranteed if client
> and
> > server reach to the point of SASL handshake in their communication, since
> > SASL handshake exchanges all the bits related to Kerberos protocol.
> Before
> > that point is reached there are still few messages which is exchanged
> > between DrillClient and Drillbit to detect whether server side needs
> > authentication or not and what are supported mechanisms. This is where
> the
> > security flaw can cause client to believe Authentication is successfully
> > completed (even when Drillbit/DrillClient are authenticating using
> Kerberos
> > with or without encryption). This is what patch for DRILL-5582 is trying
> to
> > address.
> >
> >
> You still haven't answered what is the issue/security risk for the client
> here: sure the client didn't authenticate with the server, but at the same
> time it didn't get access to the server either...
>
> Also, it doesn't take very long to modify the rogue server to fake a SASL
> authentication. So, now you are "authenticated", but still not to the right
> server...
>
>
>
> >
> > As for the other issue at hand (compatibility between 1.11 client and
> 1.10
> > server), I am not sure to understand the proposed fix: is the logic you
> > proposed to be added to 1.10 server? why not simply add the missing enum
> > value (and that's it! no more values after that!)?
> >
> >
> > [Sorabh] - Just adding the missing enum value in 1.10 will not help since
> > in future if any other enum value is introduced then the same issue will
> be
> > seen. Moreover 1.10 doesn't support Encryption so that enum value should
> > not be added in that release. Instead the fix is to treat the return
> value
> > of UNKNOWN_SASL_SERVER while retrieving messages from future client as
> > valid default value and take decision based on that.
> >
>
> But 1.10.1 could consider that a client supporting encryption also support
> authentication? The code is already here in 1.10 btw:
> https://github.com/apache/drill/blob/1.10.0/exec/java-
> exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java#L297
>
> Alternatively, you could just check that the field sasl_support is set and
> not check the value alltogether. I'm not convinced you need to do some
> extra logic around UNKNOWN_SASL_SERVER which would just keep people
> confused (although it doesn't seem something you need to apply to 1.11 or
> higher)
>
>
>
> >
> >
> > Thanks,
> > Sorabh
> >
> > ________________________________
> > From: Laurent Goujon <la...@dremio.com>
> > Sent: Tuesday, October 31, 2017 5:42 PM
> > To: dev
> > Cc: Arina Lelchieva; sudheesh@apache.org
> > Subject: Re: Drill SASL Forward Compatibility
> >
> > Regarding DRILL-5582 patch which broke compatibility with 1.9 version
> > (which is less than a year old):
> > I'm still not clear what we are trying to prevent here: stealing user
> > credentials and/or data? connecting to a rogue server which could return
> > corrupted data to the user? The patch gives a false sense of security
> > because it prevents none of it:
> > - if using password-based authentication, client is sending it in clear
> in
> > the initial handshake so I guess it's already game over!
> > - You could configure a client to not sent password over wire by choosing
> > another authentication algorithm, BUT if there's no encryption of the
> > channel, any data can be intercepted and rewritten. And of course, the
> > rogue server could actually run queries on behalf of the user...
> > - if using Kerberos, things are a bit different: even if a MITM
> intercepts
> > the token, only the server can decode it properly and set up encryption
> so
> > that only client can use it (a proxy server would not be able to decode
> the
> > traffic). So what you need to ensure is that you actually use Kerberos as
> > the only authentication mechanism, and that the channel is encrypted (if
> > channel is not encryted, see above). This is things you should do by
> > configuring the client by not sending the password (no need to), only
> > authorize Kerberos authentication, and verify that encryption is enabled
> > (which is already done I believe).
> >
> > For comparison, HTTP protocol (using it since it is one of the most used
> > public protocol) has no issue with client sending an authentication
> header
> > to a remote server, without knowing based on the server response if
> > authentication happened.
> >
> > As for the other issue at hand (compatibility between 1.11 client and
> 1.10
> > server), I am not sure to understand the proposed fix: is the logic you
> > proposed to be added to 1.10 server? why not simply add the missing enum
> > value (and that's it! no more values after that!)?
> >
> > Laurent
> >
> > On Tue, Oct 31, 2017 at 3:12 PM, Sorabh Hamirwasia <shamirwasia@mapr.com
> >
> > wrote:
> >
> > > Hi Laurent,
> > >
> > > We are preventing 2 cases here:
> > >
> > > * False positive for successful authentication. Even though MITM
> > > attack can happen after successful authentication, but client and
> server
> > > involved here has ensure successful authentication handshake has taken
> > > place. With the security flaw there can always be false positive on
> > client
> > > side thinking authentication was successful even though that might not
> be
> > > the case.
> > > * False positive for successful handshake with encryption
> capability:
> > > In cases when server is properly configured to support encryption, MITM
> > > attack tweaking handshake response and making client to believe that
> > after
> > > successful handshake all communications will be encrypted is again
> > another
> > > bad false positive.
> > >
> > > IMHO Drill Client should be able to validate when server says that
> > > authentication is completed then it's actually completed, at least
> until
> > > the point SASL Handshake is initiated, rather than blindly trusting it.
> > > This is because if Drill client doesn't guarantees that, then it's
> making
> > > the support for protocol like Kerberos weaker which prevent's from any
> > MITM
> > > attack at handshake level. Whereas mechanisms like PLAIN are still
> prone
> > to
> > > MITM even during SASL handshake.
> > >
> > > As far as forward compatibility is concerned there are few things:
> > >
> > > * AFAIK DrillClient & Drillbit doesn't have any concept of
> supporting
> > > different RPC versions across releases.They are forced to talk on same
> > RPC
> > > versions else connection will fail. Once we have that I think then we
> > will
> > > be able to clearly justify or provide the matrix of backward and
> forward
> > > compatibility across releases.
> > > * We can put the check based on supported_methods to detect if
> server
> > > side supports SASL or not. But this is again just a work around not
> > proper
> > > solution. With workaround there can still be similar security holes as
> > > PLAIN mechanism itself is prone to MITM. Given 1.9 is 3 releases behind
> > > now, not sure if we still want to support that combination.
> > > * At least for compatibility between Drill 1.11 client and Drill
> 1.10
> > > server, I think the fix should be made which is mentioned in first
> email
> > of
> > > this thread.
> > >
> > > Thanks,
> > > Sorabh
> > >
> > > ________________________________
> > > From: Laurent Goujon <la...@dremio.com>
> > > Sent: Tuesday, October 31, 2017 9:38:13 AM
> > > To: dev
> > > Cc: Arina Lelchieva; sudheesh@apache.org
> > > Subject: Re: Drill SASL Forward Compatibility
> > >
> > > See my answers inline.
> > >
> > > On Tue, Oct 31, 2017 at 1:40 AM, Sorabh Hamirwasia <
> shamirwasia@mapr.com
> > >
> > > wrote:
> > >
> > > > Hi Laurent,
> > > >
> > > > Thanks for pointing issue with <= 1.9 version Drillbit, I looked into
> > > > supported_methods field and it doesn't advertise SASL support using
> > that
> > > > [1][2]. Do you mean that checking if supported_methods list is
> > non-empty
> > > > should suffice since it was introduced in 1.10 ?
> > > >
> > >
> > > My bad, I thought SASL_MESSAGE was added to the list of supported
> > methods,
> > > but it's not. Alternatively you could check for
> authenticationMechanisms
> > > which should be not empty if version >= 1.10 and authentication is
> turned
> > > on.
> > >
> > >
> > > >
> > > >
> > > > For security flaw let's consider an example. Let say client is
> > connecting
> > > > to a Drillbit with authentication (and or encryption) enabled for
> > > Kerberos
> > > > mechanism. The message flow will happen something like below:
> > > >
> > > >
> > > > Good Case:
> > > >
> > > > * DrillClient sends Handshake Request to Drillbit
> > > > * Drillbit sends the response back to DrillClient with AUTH_REQD
> as
> > > > status
> > > > * DrillClient exchange SASL handshake messages with Drillbit.
> > > > * Once handshake is successful DrillClient is connected to secure
> > > > Drillbit.
> > > > * App using DrillClient has actually established a connection to
> > > > secure Drillbit with authentication (and or encryption) and can
> submit
> > > it's
> > > > query.
> > > >
> > > > Bad Case:
> > > >
> > > > * Step 1 as above
> > > > * Step 2 as above. This message was intercepted by MITM and
> status
> > > was
> > > > changed to SUCCESS.
> > > > * Without the recent change DrillClient will not initiate SASL
> > > > handshake and will return connection successful.
> > > > * App using DrillClient will think it has successfully connected
> to
> > > > secure Drillbit which is NOT the case.
> > > >
> > >
> > > What are we preventing in the bad case? if this is credentials or data
> > > interception, a MITM could simply act as proxy to intercept all of it
> > since
> > > traffic is not encrypted. If we were to prevent the loss of
> credentials,
> > > the solution to avoid transmitting the credentials in clear in the
> first
> > > place. For that, we don't need a protocol change but:
> > > - disable plain authentication, and use something like Kerberos or
> > > CRAM-MD5/SCRAM
> > > - make sure the password is not sent in the initial handshake (if using
> > > Kerberos, there should no credentials to send over in the first place)
> > >
> > >
> > > >
> > > > I think what you are pointing out is even in good case and in
> > > > authentication only scenario, even if connection is successful, the
> > > > messages between DrillClient and Drillbit can still be intercepted
> > since
> > > > they will be in plain text. The only way to avoid that is using
> > > encryption.
> > > > But the fix was more of to avoid wrong behavior in that case too
> where
> > > > connection should fail, instead of client just relying on server
> > > response.
> > > >
> > >
> > > The "wrong" behavior was what allowed for compatibility with older
> > servers
> > > in the original design...
> > >
> > >
> > > >
> > > > [1]: https://github.com/apache/drill/blob/1.11.0/exec/java-
> > > > exec/src/main/java/org/apache/drill/exec/rpc/user/
> UserServer.java#L254
> > > > [2]: https://github.com/apache/drill/blob/1.11.0/exec/java-
> > > > exec/src/main/java/org/apache/drill/exec/rpc/user/
> > UserRpcConfig.java#L89
> > > >
> > > >
> > > > Thanks,
> > > > Sorabh
> > > >
> > > > ________________________________
> > > > From: Laurent Goujon <la...@dremio.com>
> > > > Sent: Monday, October 30, 2017 5:47 PM
> > > > To: dev
> > > > Cc: Arina Lelchieva; sudheesh@apache.org
> > > > Subject: Re: Drill SASL Forward Compatibility
> > > >
> > > > Regarding DRILL-5582, I see that fix as a breakage of the work to
> > > maintain
> > > > compatibility for an newer client to connect to a older version of
> the
> > > > server. Or put it differently: current (master) client does not
> connect
> > > > anymore to a server not supporting SASL (<=1.9). Note that the client
> > > could
> > > > detect if the server supports SASL as it is advertised in the
> > > > supported_methods field of the BitToUserHandshake, and it would
> restore
> > > > compatibility, but it seems the fix was done in response to a
> potential
> > > > security flaw (although I have to admin not sure what issue it does
> > > prevent
> > > > since it is still possible for a MITM to intercept all traffic
> between
> > a
> > > > client and a server).
> > > >
> > > > Laurent
> > > >
> > > > On Mon, Oct 30, 2017 at 5:18 PM, Sorabh Hamirwasia <
> > shamirwasia@mapr.com
> > > >
> > > > wrote:
> > > >
> > > > > Hi All,
> > > > >
> > > > > We recently added a check (as part of DRILL-5582<https://issues.
> > > > > apache.org/jira/browse/DRILL-5582>) on DrillClient side to enforce
> > > that
> > > > > if client showed intent for authentication and Drillbit say's it
> > > doesn't
> > > > > require authentication then connection will fail with proper error
> > > > message.
> > > > >
> > > > >
> > > > > With this change we found a hidden issue w.r.t forward
> compatibility
> > of
> > > > > Drill. New clients running on 1.11+ authenticating to older
> Drillbit
> > > > > running on 1.10 are treated as client running without any SASL
> > support
> > > or
> > > > > (<=1.9 version). The root cause for this issue is as follows:
> > > > >
> > > > >
> > > > > In 1.10 a new field SASL_SUPPORT was introduced in Handshake
> message
> > > > > between DrillCilent and Drillbit. The supported values for that
> field
> > > > are:
> > > > >
> > > > >
> > > > > Drill 1.10: [1]
> > > > >
> > > > >
> > > > > enum SASL_SUPPORT {
> > > > > UNKNOWN_SASL_SUPPORT = 0;
> > > > > SASL_AUTH = 1;
> > > > > }
> > > > >
> > > > >
> > > > > Drill 1.11/1.12: [2]
> > > > >
> > > > >
> > > > > enum SASL_SUPPORT {
> > > > > UNKNOWN_SASL_SUPPORT = 0;
> > > > > SASL_AUTH = 1;
> > > > > SASL_PRIVACY = 2;
> > > > > }
> > > > >
> > > > > A 1.11 client always has SASL_PRIVACY set in handshake. A 1.10
> > Drillbit
> > > > > getting the message interprets the value as UNKNOWN_SASL_SUPPORT
> [3].
> > > In
> > > > > 1.10 Drillbit treats that value as an indication of older client <
> > 1.10
> > > > > [4], and it will try to authenticate using the 1.9 mechanism and
> send
> > > the
> > > > > SUCCESS/FAILURE state in Handshake Response. The 1.12 DrillClient
> > will
> > > > fail
> > > > > the connection as it will expect AUTH_REQUIRED from Drillbit
> instead
> > of
> > > > > SUCCESS/FAILURE.
> > > > >
> > > > >
> > > > > The fix for this issue can be to use only absence of field as
> > > indication
> > > > > of client < 1.10 and if the field is present but it evaluates to
> > > > > UNKNOWN_SASL_SUPPORT value then Drillbit should consider
> > corresponding
> > > > > client to be of future version at least for authentication purpose
> > and
> > > > > speak SASL protocol.
> > > > >
> > > > > We have to either back port above forward compatibility fix into
> 1.10
> > > and
> > > > > 1.11 or just fix in current release and support forward
> compatibility
> > > > post
> > > > > 1.12+.
> > > > >
> > > > >
> > > > > Arina/Sudheesh - Please suggest if the analysis and fix sounds good
> > and
> > > > > what are the releases we should consider the fix for. Considering
> > 1.12
> > > > > release is planned soon can we take the fix in 1.12 release ?
> > > > >
> > > > >
> > > > >
> > > > > [1]: https://github.com/apache/drill/blob/1.10.0/protocol/
> > > > > src/main/protobuf/User.proto#L70
> > > > >
> > > > > [2]: https://github.com/apache/drill/blob/1.11.0/protocol/
> > > > > src/main/protobuf/User.proto#L70
> > > > >
> > > > > [3]: http://androiddevblog.com/protocol-buffers-pitfall-
> > > > > adding-enum-values/
> > > > >
> > > > > [4]: https://github.com/apache/drill/blob/1.10.0/exec/java-
> > > > > exec/src/main/java/org/apache/drill/exec/rpc/user/
> > UserServer.java#L297
> > > > >
> > > > >
> > > > > Thanks,
> > > > > Sorabh
> > > > >
> > > >
> > >
> >
>
Re: Drill SASL Forward Compatibility
Posted by Laurent Goujon <la...@dremio.com>.
My comments inline:
On Tue, Oct 31, 2017 at 6:11 PM, Sorabh Hamirwasia <sh...@mapr.com>
wrote:
> - if using Kerberos, things are a bit different: even if a MITM intercepts
>
> the token, only the server can decode it properly and set up encryption so
> that only client can use it (a proxy server would not be able to decode the
> traffic). So what you need to ensure is that you actually use Kerberos as
> the only authentication mechanism, and that the channel is encrypted (if
> channel is not encryted, see above). This is things you should do by
> configuring the client by not sending the password (no need to), only
> authorize Kerberos authentication, and verify that encryption is enabled
> (which is already done I believe).
>
>
> [Sorabh] - You are correct about the Kerberos preventing MITM which is
> what I mentioned in the last response. But this is guaranteed if client and
> server reach to the point of SASL handshake in their communication, since
> SASL handshake exchanges all the bits related to Kerberos protocol. Before
> that point is reached there are still few messages which is exchanged
> between DrillClient and Drillbit to detect whether server side needs
> authentication or not and what are supported mechanisms. This is where the
> security flaw can cause client to believe Authentication is successfully
> completed (even when Drillbit/DrillClient are authenticating using Kerberos
> with or without encryption). This is what patch for DRILL-5582 is trying to
> address.
>
>
You still haven't answered what is the issue/security risk for the client
here: sure the client didn't authenticate with the server, but at the same
time it didn't get access to the server either...
Also, it doesn't take very long to modify the rogue server to fake a SASL
authentication. So, now you are "authenticated", but still not to the right
server...
>
> As for the other issue at hand (compatibility between 1.11 client and 1.10
> server), I am not sure to understand the proposed fix: is the logic you
> proposed to be added to 1.10 server? why not simply add the missing enum
> value (and that's it! no more values after that!)?
>
>
> [Sorabh] - Just adding the missing enum value in 1.10 will not help since
> in future if any other enum value is introduced then the same issue will be
> seen. Moreover 1.10 doesn't support Encryption so that enum value should
> not be added in that release. Instead the fix is to treat the return value
> of UNKNOWN_SASL_SERVER while retrieving messages from future client as
> valid default value and take decision based on that.
>
But 1.10.1 could consider that a client supporting encryption also support
authentication? The code is already here in 1.10 btw:
https://github.com/apache/drill/blob/1.10.0/exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java#L297
Alternatively, you could just check that the field sasl_support is set and
not check the value alltogether. I'm not convinced you need to do some
extra logic around UNKNOWN_SASL_SERVER which would just keep people
confused (although it doesn't seem something you need to apply to 1.11 or
higher)
>
>
> Thanks,
> Sorabh
>
> ________________________________
> From: Laurent Goujon <la...@dremio.com>
> Sent: Tuesday, October 31, 2017 5:42 PM
> To: dev
> Cc: Arina Lelchieva; sudheesh@apache.org
> Subject: Re: Drill SASL Forward Compatibility
>
> Regarding DRILL-5582 patch which broke compatibility with 1.9 version
> (which is less than a year old):
> I'm still not clear what we are trying to prevent here: stealing user
> credentials and/or data? connecting to a rogue server which could return
> corrupted data to the user? The patch gives a false sense of security
> because it prevents none of it:
> - if using password-based authentication, client is sending it in clear in
> the initial handshake so I guess it's already game over!
> - You could configure a client to not sent password over wire by choosing
> another authentication algorithm, BUT if there's no encryption of the
> channel, any data can be intercepted and rewritten. And of course, the
> rogue server could actually run queries on behalf of the user...
> - if using Kerberos, things are a bit different: even if a MITM intercepts
> the token, only the server can decode it properly and set up encryption so
> that only client can use it (a proxy server would not be able to decode the
> traffic). So what you need to ensure is that you actually use Kerberos as
> the only authentication mechanism, and that the channel is encrypted (if
> channel is not encryted, see above). This is things you should do by
> configuring the client by not sending the password (no need to), only
> authorize Kerberos authentication, and verify that encryption is enabled
> (which is already done I believe).
>
> For comparison, HTTP protocol (using it since it is one of the most used
> public protocol) has no issue with client sending an authentication header
> to a remote server, without knowing based on the server response if
> authentication happened.
>
> As for the other issue at hand (compatibility between 1.11 client and 1.10
> server), I am not sure to understand the proposed fix: is the logic you
> proposed to be added to 1.10 server? why not simply add the missing enum
> value (and that's it! no more values after that!)?
>
> Laurent
>
> On Tue, Oct 31, 2017 at 3:12 PM, Sorabh Hamirwasia <sh...@mapr.com>
> wrote:
>
> > Hi Laurent,
> >
> > We are preventing 2 cases here:
> >
> > * False positive for successful authentication. Even though MITM
> > attack can happen after successful authentication, but client and server
> > involved here has ensure successful authentication handshake has taken
> > place. With the security flaw there can always be false positive on
> client
> > side thinking authentication was successful even though that might not be
> > the case.
> > * False positive for successful handshake with encryption capability:
> > In cases when server is properly configured to support encryption, MITM
> > attack tweaking handshake response and making client to believe that
> after
> > successful handshake all communications will be encrypted is again
> another
> > bad false positive.
> >
> > IMHO Drill Client should be able to validate when server says that
> > authentication is completed then it's actually completed, at least until
> > the point SASL Handshake is initiated, rather than blindly trusting it.
> > This is because if Drill client doesn't guarantees that, then it's making
> > the support for protocol like Kerberos weaker which prevent's from any
> MITM
> > attack at handshake level. Whereas mechanisms like PLAIN are still prone
> to
> > MITM even during SASL handshake.
> >
> > As far as forward compatibility is concerned there are few things:
> >
> > * AFAIK DrillClient & Drillbit doesn't have any concept of supporting
> > different RPC versions across releases.They are forced to talk on same
> RPC
> > versions else connection will fail. Once we have that I think then we
> will
> > be able to clearly justify or provide the matrix of backward and forward
> > compatibility across releases.
> > * We can put the check based on supported_methods to detect if server
> > side supports SASL or not. But this is again just a work around not
> proper
> > solution. With workaround there can still be similar security holes as
> > PLAIN mechanism itself is prone to MITM. Given 1.9 is 3 releases behind
> > now, not sure if we still want to support that combination.
> > * At least for compatibility between Drill 1.11 client and Drill 1.10
> > server, I think the fix should be made which is mentioned in first email
> of
> > this thread.
> >
> > Thanks,
> > Sorabh
> >
> > ________________________________
> > From: Laurent Goujon <la...@dremio.com>
> > Sent: Tuesday, October 31, 2017 9:38:13 AM
> > To: dev
> > Cc: Arina Lelchieva; sudheesh@apache.org
> > Subject: Re: Drill SASL Forward Compatibility
> >
> > See my answers inline.
> >
> > On Tue, Oct 31, 2017 at 1:40 AM, Sorabh Hamirwasia <shamirwasia@mapr.com
> >
> > wrote:
> >
> > > Hi Laurent,
> > >
> > > Thanks for pointing issue with <= 1.9 version Drillbit, I looked into
> > > supported_methods field and it doesn't advertise SASL support using
> that
> > > [1][2]. Do you mean that checking if supported_methods list is
> non-empty
> > > should suffice since it was introduced in 1.10 ?
> > >
> >
> > My bad, I thought SASL_MESSAGE was added to the list of supported
> methods,
> > but it's not. Alternatively you could check for authenticationMechanisms
> > which should be not empty if version >= 1.10 and authentication is turned
> > on.
> >
> >
> > >
> > >
> > > For security flaw let's consider an example. Let say client is
> connecting
> > > to a Drillbit with authentication (and or encryption) enabled for
> > Kerberos
> > > mechanism. The message flow will happen something like below:
> > >
> > >
> > > Good Case:
> > >
> > > * DrillClient sends Handshake Request to Drillbit
> > > * Drillbit sends the response back to DrillClient with AUTH_REQD as
> > > status
> > > * DrillClient exchange SASL handshake messages with Drillbit.
> > > * Once handshake is successful DrillClient is connected to secure
> > > Drillbit.
> > > * App using DrillClient has actually established a connection to
> > > secure Drillbit with authentication (and or encryption) and can submit
> > it's
> > > query.
> > >
> > > Bad Case:
> > >
> > > * Step 1 as above
> > > * Step 2 as above. This message was intercepted by MITM and status
> > was
> > > changed to SUCCESS.
> > > * Without the recent change DrillClient will not initiate SASL
> > > handshake and will return connection successful.
> > > * App using DrillClient will think it has successfully connected to
> > > secure Drillbit which is NOT the case.
> > >
> >
> > What are we preventing in the bad case? if this is credentials or data
> > interception, a MITM could simply act as proxy to intercept all of it
> since
> > traffic is not encrypted. If we were to prevent the loss of credentials,
> > the solution to avoid transmitting the credentials in clear in the first
> > place. For that, we don't need a protocol change but:
> > - disable plain authentication, and use something like Kerberos or
> > CRAM-MD5/SCRAM
> > - make sure the password is not sent in the initial handshake (if using
> > Kerberos, there should no credentials to send over in the first place)
> >
> >
> > >
> > > I think what you are pointing out is even in good case and in
> > > authentication only scenario, even if connection is successful, the
> > > messages between DrillClient and Drillbit can still be intercepted
> since
> > > they will be in plain text. The only way to avoid that is using
> > encryption.
> > > But the fix was more of to avoid wrong behavior in that case too where
> > > connection should fail, instead of client just relying on server
> > response.
> > >
> >
> > The "wrong" behavior was what allowed for compatibility with older
> servers
> > in the original design...
> >
> >
> > >
> > > [1]: https://github.com/apache/drill/blob/1.11.0/exec/java-
> > > exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java#L254
> > > [2]: https://github.com/apache/drill/blob/1.11.0/exec/java-
> > > exec/src/main/java/org/apache/drill/exec/rpc/user/
> UserRpcConfig.java#L89
> > >
> > >
> > > Thanks,
> > > Sorabh
> > >
> > > ________________________________
> > > From: Laurent Goujon <la...@dremio.com>
> > > Sent: Monday, October 30, 2017 5:47 PM
> > > To: dev
> > > Cc: Arina Lelchieva; sudheesh@apache.org
> > > Subject: Re: Drill SASL Forward Compatibility
> > >
> > > Regarding DRILL-5582, I see that fix as a breakage of the work to
> > maintain
> > > compatibility for an newer client to connect to a older version of the
> > > server. Or put it differently: current (master) client does not connect
> > > anymore to a server not supporting SASL (<=1.9). Note that the client
> > could
> > > detect if the server supports SASL as it is advertised in the
> > > supported_methods field of the BitToUserHandshake, and it would restore
> > > compatibility, but it seems the fix was done in response to a potential
> > > security flaw (although I have to admin not sure what issue it does
> > prevent
> > > since it is still possible for a MITM to intercept all traffic between
> a
> > > client and a server).
> > >
> > > Laurent
> > >
> > > On Mon, Oct 30, 2017 at 5:18 PM, Sorabh Hamirwasia <
> shamirwasia@mapr.com
> > >
> > > wrote:
> > >
> > > > Hi All,
> > > >
> > > > We recently added a check (as part of DRILL-5582<https://issues.
> > > > apache.org/jira/browse/DRILL-5582>) on DrillClient side to enforce
> > that
> > > > if client showed intent for authentication and Drillbit say's it
> > doesn't
> > > > require authentication then connection will fail with proper error
> > > message.
> > > >
> > > >
> > > > With this change we found a hidden issue w.r.t forward compatibility
> of
> > > > Drill. New clients running on 1.11+ authenticating to older Drillbit
> > > > running on 1.10 are treated as client running without any SASL
> support
> > or
> > > > (<=1.9 version). The root cause for this issue is as follows:
> > > >
> > > >
> > > > In 1.10 a new field SASL_SUPPORT was introduced in Handshake message
> > > > between DrillCilent and Drillbit. The supported values for that field
> > > are:
> > > >
> > > >
> > > > Drill 1.10: [1]
> > > >
> > > >
> > > > enum SASL_SUPPORT {
> > > > UNKNOWN_SASL_SUPPORT = 0;
> > > > SASL_AUTH = 1;
> > > > }
> > > >
> > > >
> > > > Drill 1.11/1.12: [2]
> > > >
> > > >
> > > > enum SASL_SUPPORT {
> > > > UNKNOWN_SASL_SUPPORT = 0;
> > > > SASL_AUTH = 1;
> > > > SASL_PRIVACY = 2;
> > > > }
> > > >
> > > > A 1.11 client always has SASL_PRIVACY set in handshake. A 1.10
> Drillbit
> > > > getting the message interprets the value as UNKNOWN_SASL_SUPPORT [3].
> > In
> > > > 1.10 Drillbit treats that value as an indication of older client <
> 1.10
> > > > [4], and it will try to authenticate using the 1.9 mechanism and send
> > the
> > > > SUCCESS/FAILURE state in Handshake Response. The 1.12 DrillClient
> will
> > > fail
> > > > the connection as it will expect AUTH_REQUIRED from Drillbit instead
> of
> > > > SUCCESS/FAILURE.
> > > >
> > > >
> > > > The fix for this issue can be to use only absence of field as
> > indication
> > > > of client < 1.10 and if the field is present but it evaluates to
> > > > UNKNOWN_SASL_SUPPORT value then Drillbit should consider
> corresponding
> > > > client to be of future version at least for authentication purpose
> and
> > > > speak SASL protocol.
> > > >
> > > > We have to either back port above forward compatibility fix into 1.10
> > and
> > > > 1.11 or just fix in current release and support forward compatibility
> > > post
> > > > 1.12+.
> > > >
> > > >
> > > > Arina/Sudheesh - Please suggest if the analysis and fix sounds good
> and
> > > > what are the releases we should consider the fix for. Considering
> 1.12
> > > > release is planned soon can we take the fix in 1.12 release ?
> > > >
> > > >
> > > >
> > > > [1]: https://github.com/apache/drill/blob/1.10.0/protocol/
> > > > src/main/protobuf/User.proto#L70
> > > >
> > > > [2]: https://github.com/apache/drill/blob/1.11.0/protocol/
> > > > src/main/protobuf/User.proto#L70
> > > >
> > > > [3]: http://androiddevblog.com/protocol-buffers-pitfall-
> > > > adding-enum-values/
> > > >
> > > > [4]: https://github.com/apache/drill/blob/1.10.0/exec/java-
> > > > exec/src/main/java/org/apache/drill/exec/rpc/user/
> UserServer.java#L297
> > > >
> > > >
> > > > Thanks,
> > > > Sorabh
> > > >
> > >
> >
>
Re: Drill SASL Forward Compatibility
Posted by Sorabh Hamirwasia <sh...@mapr.com>.
- if using Kerberos, things are a bit different: even if a MITM intercepts
the token, only the server can decode it properly and set up encryption so
that only client can use it (a proxy server would not be able to decode the
traffic). So what you need to ensure is that you actually use Kerberos as
the only authentication mechanism, and that the channel is encrypted (if
channel is not encryted, see above). This is things you should do by
configuring the client by not sending the password (no need to), only
authorize Kerberos authentication, and verify that encryption is enabled
(which is already done I believe).
[Sorabh] - You are correct about the Kerberos preventing MITM which is what I mentioned in the last response. But this is guaranteed if client and server reach to the point of SASL handshake in their communication, since SASL handshake exchanges all the bits related to Kerberos protocol. Before that point is reached there are still few messages which is exchanged between DrillClient and Drillbit to detect whether server side needs authentication or not and what are supported mechanisms. This is where the security flaw can cause client to believe Authentication is successfully completed (even when Drillbit/DrillClient are authenticating using Kerberos with or without encryption). This is what patch for DRILL-5582 is trying to address.
As for the other issue at hand (compatibility between 1.11 client and 1.10
server), I am not sure to understand the proposed fix: is the logic you
proposed to be added to 1.10 server? why not simply add the missing enum
value (and that's it! no more values after that!)?
[Sorabh] - Just adding the missing enum value in 1.10 will not help since in future if any other enum value is introduced then the same issue will be seen. Moreover 1.10 doesn't support Encryption so that enum value should not be added in that release. Instead the fix is to treat the return value of UNKNOWN_SASL_SERVER while retrieving messages from future client as valid default value and take decision based on that.
Thanks,
Sorabh
________________________________
From: Laurent Goujon <la...@dremio.com>
Sent: Tuesday, October 31, 2017 5:42 PM
To: dev
Cc: Arina Lelchieva; sudheesh@apache.org
Subject: Re: Drill SASL Forward Compatibility
Regarding DRILL-5582 patch which broke compatibility with 1.9 version
(which is less than a year old):
I'm still not clear what we are trying to prevent here: stealing user
credentials and/or data? connecting to a rogue server which could return
corrupted data to the user? The patch gives a false sense of security
because it prevents none of it:
- if using password-based authentication, client is sending it in clear in
the initial handshake so I guess it's already game over!
- You could configure a client to not sent password over wire by choosing
another authentication algorithm, BUT if there's no encryption of the
channel, any data can be intercepted and rewritten. And of course, the
rogue server could actually run queries on behalf of the user...
- if using Kerberos, things are a bit different: even if a MITM intercepts
the token, only the server can decode it properly and set up encryption so
that only client can use it (a proxy server would not be able to decode the
traffic). So what you need to ensure is that you actually use Kerberos as
the only authentication mechanism, and that the channel is encrypted (if
channel is not encryted, see above). This is things you should do by
configuring the client by not sending the password (no need to), only
authorize Kerberos authentication, and verify that encryption is enabled
(which is already done I believe).
For comparison, HTTP protocol (using it since it is one of the most used
public protocol) has no issue with client sending an authentication header
to a remote server, without knowing based on the server response if
authentication happened.
As for the other issue at hand (compatibility between 1.11 client and 1.10
server), I am not sure to understand the proposed fix: is the logic you
proposed to be added to 1.10 server? why not simply add the missing enum
value (and that's it! no more values after that!)?
Laurent
On Tue, Oct 31, 2017 at 3:12 PM, Sorabh Hamirwasia <sh...@mapr.com>
wrote:
> Hi Laurent,
>
> We are preventing 2 cases here:
>
> * False positive for successful authentication. Even though MITM
> attack can happen after successful authentication, but client and server
> involved here has ensure successful authentication handshake has taken
> place. With the security flaw there can always be false positive on client
> side thinking authentication was successful even though that might not be
> the case.
> * False positive for successful handshake with encryption capability:
> In cases when server is properly configured to support encryption, MITM
> attack tweaking handshake response and making client to believe that after
> successful handshake all communications will be encrypted is again another
> bad false positive.
>
> IMHO Drill Client should be able to validate when server says that
> authentication is completed then it's actually completed, at least until
> the point SASL Handshake is initiated, rather than blindly trusting it.
> This is because if Drill client doesn't guarantees that, then it's making
> the support for protocol like Kerberos weaker which prevent's from any MITM
> attack at handshake level. Whereas mechanisms like PLAIN are still prone to
> MITM even during SASL handshake.
>
> As far as forward compatibility is concerned there are few things:
>
> * AFAIK DrillClient & Drillbit doesn't have any concept of supporting
> different RPC versions across releases.They are forced to talk on same RPC
> versions else connection will fail. Once we have that I think then we will
> be able to clearly justify or provide the matrix of backward and forward
> compatibility across releases.
> * We can put the check based on supported_methods to detect if server
> side supports SASL or not. But this is again just a work around not proper
> solution. With workaround there can still be similar security holes as
> PLAIN mechanism itself is prone to MITM. Given 1.9 is 3 releases behind
> now, not sure if we still want to support that combination.
> * At least for compatibility between Drill 1.11 client and Drill 1.10
> server, I think the fix should be made which is mentioned in first email of
> this thread.
>
> Thanks,
> Sorabh
>
> ________________________________
> From: Laurent Goujon <la...@dremio.com>
> Sent: Tuesday, October 31, 2017 9:38:13 AM
> To: dev
> Cc: Arina Lelchieva; sudheesh@apache.org
> Subject: Re: Drill SASL Forward Compatibility
>
> See my answers inline.
>
> On Tue, Oct 31, 2017 at 1:40 AM, Sorabh Hamirwasia <sh...@mapr.com>
> wrote:
>
> > Hi Laurent,
> >
> > Thanks for pointing issue with <= 1.9 version Drillbit, I looked into
> > supported_methods field and it doesn't advertise SASL support using that
> > [1][2]. Do you mean that checking if supported_methods list is non-empty
> > should suffice since it was introduced in 1.10 ?
> >
>
> My bad, I thought SASL_MESSAGE was added to the list of supported methods,
> but it's not. Alternatively you could check for authenticationMechanisms
> which should be not empty if version >= 1.10 and authentication is turned
> on.
>
>
> >
> >
> > For security flaw let's consider an example. Let say client is connecting
> > to a Drillbit with authentication (and or encryption) enabled for
> Kerberos
> > mechanism. The message flow will happen something like below:
> >
> >
> > Good Case:
> >
> > * DrillClient sends Handshake Request to Drillbit
> > * Drillbit sends the response back to DrillClient with AUTH_REQD as
> > status
> > * DrillClient exchange SASL handshake messages with Drillbit.
> > * Once handshake is successful DrillClient is connected to secure
> > Drillbit.
> > * App using DrillClient has actually established a connection to
> > secure Drillbit with authentication (and or encryption) and can submit
> it's
> > query.
> >
> > Bad Case:
> >
> > * Step 1 as above
> > * Step 2 as above. This message was intercepted by MITM and status
> was
> > changed to SUCCESS.
> > * Without the recent change DrillClient will not initiate SASL
> > handshake and will return connection successful.
> > * App using DrillClient will think it has successfully connected to
> > secure Drillbit which is NOT the case.
> >
>
> What are we preventing in the bad case? if this is credentials or data
> interception, a MITM could simply act as proxy to intercept all of it since
> traffic is not encrypted. If we were to prevent the loss of credentials,
> the solution to avoid transmitting the credentials in clear in the first
> place. For that, we don't need a protocol change but:
> - disable plain authentication, and use something like Kerberos or
> CRAM-MD5/SCRAM
> - make sure the password is not sent in the initial handshake (if using
> Kerberos, there should no credentials to send over in the first place)
>
>
> >
> > I think what you are pointing out is even in good case and in
> > authentication only scenario, even if connection is successful, the
> > messages between DrillClient and Drillbit can still be intercepted since
> > they will be in plain text. The only way to avoid that is using
> encryption.
> > But the fix was more of to avoid wrong behavior in that case too where
> > connection should fail, instead of client just relying on server
> response.
> >
>
> The "wrong" behavior was what allowed for compatibility with older servers
> in the original design...
>
>
> >
> > [1]: https://github.com/apache/drill/blob/1.11.0/exec/java-
> > exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java#L254
> > [2]: https://github.com/apache/drill/blob/1.11.0/exec/java-
> > exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java#L89
> >
> >
> > Thanks,
> > Sorabh
> >
> > ________________________________
> > From: Laurent Goujon <la...@dremio.com>
> > Sent: Monday, October 30, 2017 5:47 PM
> > To: dev
> > Cc: Arina Lelchieva; sudheesh@apache.org
> > Subject: Re: Drill SASL Forward Compatibility
> >
> > Regarding DRILL-5582, I see that fix as a breakage of the work to
> maintain
> > compatibility for an newer client to connect to a older version of the
> > server. Or put it differently: current (master) client does not connect
> > anymore to a server not supporting SASL (<=1.9). Note that the client
> could
> > detect if the server supports SASL as it is advertised in the
> > supported_methods field of the BitToUserHandshake, and it would restore
> > compatibility, but it seems the fix was done in response to a potential
> > security flaw (although I have to admin not sure what issue it does
> prevent
> > since it is still possible for a MITM to intercept all traffic between a
> > client and a server).
> >
> > Laurent
> >
> > On Mon, Oct 30, 2017 at 5:18 PM, Sorabh Hamirwasia <shamirwasia@mapr.com
> >
> > wrote:
> >
> > > Hi All,
> > >
> > > We recently added a check (as part of DRILL-5582<https://issues.
> > > apache.org/jira/browse/DRILL-5582>) on DrillClient side to enforce
> that
> > > if client showed intent for authentication and Drillbit say's it
> doesn't
> > > require authentication then connection will fail with proper error
> > message.
> > >
> > >
> > > With this change we found a hidden issue w.r.t forward compatibility of
> > > Drill. New clients running on 1.11+ authenticating to older Drillbit
> > > running on 1.10 are treated as client running without any SASL support
> or
> > > (<=1.9 version). The root cause for this issue is as follows:
> > >
> > >
> > > In 1.10 a new field SASL_SUPPORT was introduced in Handshake message
> > > between DrillCilent and Drillbit. The supported values for that field
> > are:
> > >
> > >
> > > Drill 1.10: [1]
> > >
> > >
> > > enum SASL_SUPPORT {
> > > UNKNOWN_SASL_SUPPORT = 0;
> > > SASL_AUTH = 1;
> > > }
> > >
> > >
> > > Drill 1.11/1.12: [2]
> > >
> > >
> > > enum SASL_SUPPORT {
> > > UNKNOWN_SASL_SUPPORT = 0;
> > > SASL_AUTH = 1;
> > > SASL_PRIVACY = 2;
> > > }
> > >
> > > A 1.11 client always has SASL_PRIVACY set in handshake. A 1.10 Drillbit
> > > getting the message interprets the value as UNKNOWN_SASL_SUPPORT [3].
> In
> > > 1.10 Drillbit treats that value as an indication of older client < 1.10
> > > [4], and it will try to authenticate using the 1.9 mechanism and send
> the
> > > SUCCESS/FAILURE state in Handshake Response. The 1.12 DrillClient will
> > fail
> > > the connection as it will expect AUTH_REQUIRED from Drillbit instead of
> > > SUCCESS/FAILURE.
> > >
> > >
> > > The fix for this issue can be to use only absence of field as
> indication
> > > of client < 1.10 and if the field is present but it evaluates to
> > > UNKNOWN_SASL_SUPPORT value then Drillbit should consider corresponding
> > > client to be of future version at least for authentication purpose and
> > > speak SASL protocol.
> > >
> > > We have to either back port above forward compatibility fix into 1.10
> and
> > > 1.11 or just fix in current release and support forward compatibility
> > post
> > > 1.12+.
> > >
> > >
> > > Arina/Sudheesh - Please suggest if the analysis and fix sounds good and
> > > what are the releases we should consider the fix for. Considering 1.12
> > > release is planned soon can we take the fix in 1.12 release ?
> > >
> > >
> > >
> > > [1]: https://github.com/apache/drill/blob/1.10.0/protocol/
> > > src/main/protobuf/User.proto#L70
> > >
> > > [2]: https://github.com/apache/drill/blob/1.11.0/protocol/
> > > src/main/protobuf/User.proto#L70
> > >
> > > [3]: http://androiddevblog.com/protocol-buffers-pitfall-
> > > adding-enum-values/
> > >
> > > [4]: https://github.com/apache/drill/blob/1.10.0/exec/java-
> > > exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java#L297
> > >
> > >
> > > Thanks,
> > > Sorabh
> > >
> >
>