- 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 <laur...@dremio.com> Sent: Tuesday, October 31, 2017 5:42 PM To: dev Cc: Arina Lelchieva; sudhe...@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 <shamirwa...@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 <laur...@dremio.com> > Sent: Tuesday, October 31, 2017 9:38:13 AM > To: dev > Cc: Arina Lelchieva; sudhe...@apache.org > Subject: Re: Drill SASL Forward Compatibility > > See my answers inline. > > On Tue, Oct 31, 2017 at 1:40 AM, Sorabh Hamirwasia <shamirwa...@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 <laur...@dremio.com> > > Sent: Monday, October 30, 2017 5:47 PM > > To: dev > > Cc: Arina Lelchieva; sudhe...@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 <shamirwa...@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 > > > > > >
