NTLM/CredSSP Message Encryption

[jborean93]

Added support for message encryption with NTLM authentication detailed here #127.

Also fixed up some minor changes in the README that were incorrect.

@nitzmahone no rush with this but happy to see your thoughts on this and if there are better ways of doing this compared to the change.

[coveralls]

Coverage increased (+1.3%) to 69.154% when pulling e04ff84 on jborean93:ntlm-encryption into 09a81d5 on diyan:master.

[coveralls]

Coverage increased (+1.3%) to 69.154% when pulling e04ff84 on jborean93:ntlm-encryption into 09a81d5 on diyan:master.

[nitzmahone]

This looks good- like you said, pretty straightforward. The only thing I'd like to figure out is a way to shut it off (for debug/troubleshooting, mostly, but just like CBT, there may be real world situations where this needs to be disabled). Several options there. We could pass through yet another arg from Protocol, a generic auth config dict, a base AuthConfig object (with authtype-specific subclasses), or just patch through arbitrary kwargs by convention (eg, <authtype>_key, like ntlm_encryption_enabled=False)... The AuthConfig object is probably the most self-documenting, while adding top-level Protocol ctor args is the simplest. Thoughts?

[jborean93]

I think keep it simple for now, if we want to create an AuthConfig object we should potentially look at this with the PSRP stuff in a larger release. I am also leaning to just having encryption_enabled=False which would disable encryption in general instead of making it specific to an auth type. There isn't a reason that I can see to have it specific to a type unless I am missing something.

[nitzmahone]

I was a little torn on the naming, since it's specifically message encryption (vs transport encryption ala https), but yeah, that's a good compromise: a generic arg on Protocol for now, then do something fancier with a new entrypoint to supersede Protocol as part of PSRP.

…

On Apr 3, 2017 7:43 PM, "Jordan Borean" ***@***.***> wrote: I think keep it simple for now, if we want to create an AuthConfig object we should potentially look at this with the PSRP stuff in a larger release. I am also leaning to just having encryption_enabled=False which would disable encryption in general instead of making it specific to an auth type. There isn't a reason that I can see to have it specific to a type unless I am missing something. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#156 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGdjzFlehhVD7VvU-gLFH3hwhwP-7-LYks5rsa5jgaJpZM4MBr_W> .

[jborean93]

I've added the parameter message_encryption_enabled and set that to True by default. This way it is clearer that this is just message encryption not transport.

[coveralls]

Coverage increased (+1.4%) to 69.231% when pulling 6446c62 on jborean93:ntlm-encryption into 09a81d5 on diyan:master.

[nitzmahone]

Thinking about this a little more: we might want to make it a tristate (something like always, never, and smart, defaulting to smart). because right now, we're double-encrypting over HTTPS (which is valid, but not necessary and a waste of resources). I'm not in love with those value names, but you get the gist.

The alternative would be to just force-disable message encryption when we're running under HTTPS, though I can definitely contrive situations where you might want the double encryption.

Thoughts?

[jborean93]

Giving people an option is good, I suppose having;

• never: Never encrypt the messages
• always: Always encrypt even if HTTPS is used
• auto/smart: Only encrypt if HTTPS is not used

My only question would be if we are sending over HTTPS and set message_encryption to always but use an auth method like Basic or Kerberos (doesn't support yet :) ), should pywinrm throw an exception or would it consider it ok because we are over HTTPS? In a nutshell does the always only apply to message encryption or would it cover transport encryption, I would have thought the former.

[nitzmahone]

In that case, BANG IMO- we'll default to something sane (auto=="best effort/as implemented"), and if you ask us to do something impossible (eg, message encryption with basic), we should tell you we can't.

[jborean93]

Sounds like a plan I'll try and get this done shortly.

[jborean93]

@nitzmahone finally got round to this, have added message_encryption as an option to protocol and updated the README. It will fail if it is set to always and the encryption isn't available. It won't fail if set to auto and HTTP is used as I'm sure that will break a lot of builds.

[coveralls]

Coverage decreased (-1.3%) to 66.588% when pulling 4b0382a on jborean93:ntlm-encryption into 09a81d5 on diyan:master.

[coveralls]

Coverage increased (+0.5%) to 68.382% when pulling 196cb7f on jborean93:ntlm-encryption into b1a54bc on diyan:master.

[coveralls]

Coverage increased (+0.5%) to 68.382% when pulling 196cb7f on jborean93:ntlm-encryption into b1a54bc on diyan:master.

[coveralls]

Coverage increased (+0.5%) to 68.382% when pulling 196cb7f on jborean93:ntlm-encryption into b1a54bc on diyan:master.

[pdunnigan]

@diyan where do we stand with this one? I have a couple clients where this is becoming a big deal so I am planning on testing this pull request this week. Do you have any plans to merge this?

[nitzmahone]

This will be coming soon to a new pywinrm release. I've also implemented HTTP Kerberos message encryption, and will be merging this at the same time. I wanted to make sure that the same abstraction would work correctly for Kerberos before merging/releasing this, since it provides placeholders for both Kerberos and CredSSP encryption (coming later).

[pdunnigan]

I have tested this functionality (NTLM encryption) and also it does not seem to break existing functionality. I recommend a merge. I am getting requests now for CredSSP encryption too.

[coveralls]

Coverage decreased (-1.0%) to 66.889% when pulling 1c157a3 on jborean93:ntlm-encryption into b1a54bc on diyan:master.

[coveralls]

Coverage increased (+2.6%) to 70.444% when pulling f77ab31 on jborean93:ntlm-encryption into b1a54bc on diyan:master.

[coveralls]

Coverage increased (+2.6%) to 70.444% when pulling f77ab31 on jborean93:ntlm-encryption into b1a54bc on diyan:master.

[coveralls]

Coverage increased (+2.6%) to 70.444% when pulling f77ab31 on jborean93:ntlm-encryption into b1a54bc on diyan:master.

[coveralls]

Coverage increased (+2.6%) to 70.444% when pulling f77ab31 on jborean93:ntlm-encryption into b1a54bc on diyan:master.

[coveralls]

Coverage increased (+2.6%) to 70.444% when pulling f77ab31 on jborean93:ntlm-encryption into b1a54bc on diyan:master.

[byt3bl33d3r]

@jborean93 Any update on the status of merging this?

[jborean93]

@byt3bl33d3r I don't have merge rights to this repo so can't merge it in. The current status is to wait until the Kerberos message encryption is available in the downstream libraries to ensure the work we do in this library is workable for NTLM/Kerberos and CredSSP.

I've just recently added support for CredSSP encryption in this PR but have come across a weird issue I need to look into a bit more when using it with Server 2016.

@nitzmahone just an update on an issue I found right now when using this with Server 2016 hosts. When connecting to this server version it will fail to decrypt the message payload and I've found the issue is around the Length-Field value in the payload. The logic I've got to determine Length-Field can be found at https://github.com/jborean93/pywinrm/blob/f77ab31b201aeb01e66e71d80324e4719e1fda82/winrm/encryption.py#L160-L163. Length-Field for CredSSP as defined by MS-WSMV is

Length-Field: The Length-Field MUST follow immediately after the previous token. It MUST be a 32-bit
unsigned integer that specifies the length of any trailer portion of the Message field.

The logic I've had is the length of the encrypted message - length of unencrypted message - constant value, I originally had 21 for the constant but Server 2016 only works with 13. I am honestly not sure how this is calculated and came up with the value by just viewing the packets through Wireshark and this logic "seemed" to work. I then upgraded a Server 2012 host to use WMF 5 and it still worked using the old logic so it seems like a host setting or change somewhere.

I'll try look online at this as there surely is a way of correctly determining this length.

Edit: it seems like the cipher suite is behind this, I've changed the order to favour some of the older protocols and Server 2016 works with the normal logic. Obviously this isn't the behaviour we want and I need to find a way to dynamically determine this based on the cipher used.

[Bouke]

Thanks, you saved my bacon today 👍

[coveralls]

Coverage increased (+4.02%) to 71.882% when pulling e394798 on jborean93:ntlm-encryption into b1a54bc on diyan:master.