Re: secur32: Take schannel backend capabilities into account when configuring enabled protocols.
On Mar 28, 2013, at 6:05 AM, Jacek Caban wrote:
--- a/dlls/secur32/schannel_macosx.c +++ b/dlls/secur32/schannel_macosx.c @@ -630,6 +630,11 @@ static OSStatus schan_push_adapter(SSLConnectionRef transport, const void *buff, return ret; }
+DWORD schan_imp_enabled_protocols(void) +{ + /* NOTE: No support for TLS 1.1 and TLS 1.2 */ + return SP_PROT_SSL2_CLIENT | SP_PROT_SSL3_CLIENT | SP_PROT_TLS1_0_CLIENT; +}
Mac OS X 10.8 introduced support for TLS 1.1 and 1.2. You can test at build time with: #if MAC_OS_X_VERSION_MAX_ALLOWED >= 1080 ... #else ... #endif If we want to support building on 10.8 for deployment to earlier versions, we'd do something like: #if MAC_OS_X_VERSION_MAX_ALLOWED >= 1080 SSLProtocol maxProtocol; if (SSLGetProtocolVersionMax != NULL && SSLGetProtocolVersionMax(context, &maxProtocol) == noErr) { ... compare maxProtocol against kTLSProtocol11 and kTLSProtocol12 ... } ... #else ... #endif The idea is that SSLGetProtocolVersionMax() would be weak linked, so we'd check if it was actually available before calling it. Of course, the other complication is that that function requires a context parameter, but we can create one just for the query if we're interested in the framework capabilities (as opposed to what's been configured for a particular context). -Ken
On Thu, Mar 28, 2013 at 12:31 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
On Mar 28, 2013, at 6:05 AM, Jacek Caban wrote:
--- a/dlls/secur32/schannel_macosx.c +++ b/dlls/secur32/schannel_macosx.c @@ -630,6 +630,11 @@ static OSStatus schan_push_adapter(SSLConnectionRef transport, const void *buff, return ret; }
+DWORD schan_imp_enabled_protocols(void) +{ + /* NOTE: No support for TLS 1.1 and TLS 1.2 */ + return SP_PROT_SSL2_CLIENT | SP_PROT_SSL3_CLIENT | SP_PROT_TLS1_0_CLIENT;
Do we really want to continue supporting SSL2? It's got a number of vulnerabilities, and is disabled pretty much everywhere by now: http://en.wikipedia.org/wiki/Transport_Layer_Security#SSL_2.0 --Juan
Hi Juan, On 03/28/13 21:55, Juan Lang wrote:
On Thu, Mar 28, 2013 at 12:31 PM, Ken Thomases <ken(a)codeweavers.com <mailto:ken(a)codeweavers.com>> wrote:
On Mar 28, 2013, at 6:05 AM, Jacek Caban wrote:
> --- a/dlls/secur32/schannel_macosx.c > +++ b/dlls/secur32/schannel_macosx.c > @@ -630,6 +630,11 @@ static OSStatus schan_push_adapter(SSLConnectionRef transport, const void *buff, > return ret; > } > > +DWORD schan_imp_enabled_protocols(void) > +{ > + /* NOTE: No support for TLS 1.1 and TLS 1.2 */ > + return SP_PROT_SSL2_CLIENT | SP_PROT_SSL3_CLIENT | SP_PROT_TLS1_0_CLIENT;
Do we really want to continue supporting SSL2? It's got a number of vulnerabilities, and is disabled pretty much everywhere by now: http://en.wikipedia.org/wiki/Transport_Layer_Security#SSL_2.0
I implemented it the way it's done in Windows. It's a bit under-documented and contains usual MSDN mistakes, so let me explain what I found when testing Windows (most of it is implemented by http://source.winehq.org/git/wine.git/commitdiff/0f2e0365ea1f5c6baba4cfd9c0f...). Each protocol has two kinds of enable/disable flags: "enabled" and "disabled by default". Those have different default values for each protocol and there are registry setting allowing to change each of them. Only "enabled" protocols are used at all. This patch limits "enabled" protocols to those that we can really support. If an application asks schannel to use default set of protocols (which I'd expect them to do unless they have a good reason), schannel will use all "enabled" protocols that are not "disabled by default". An alternative to default set of protocols is listing each allowed separately. This means that if protocol is "enabled" and "disabled by default" it won't be used unless application explicitly asks for it. SSL2 is such a protocol by default. Do you think we should do this differently? Thanks, Jacek
Hi Jacek, thanks for the detailed reply. On Fri, Mar 29, 2013 at 3:02 AM, Jacek Caban <jacek(a)codeweavers.com> wrote:
Each protocol has two kinds of enable/disable flags: "enabled" and "disabled by default". Those have different default values for each protocol and there are registry setting allowing to change each of them. Only "enabled" protocols are used at all. This patch limits "enabled" protocols to those that we can really support. If an application asks schannel to use default set of protocols (which I'd expect them to do unless they have a good reason), schannel will use all "enabled" protocols that are not "disabled by default". An alternative to default set of protocols is listing each allowed separately.
This means that if protocol is "enabled" and "disabled by default" it won't be used unless application explicitly asks for it. SSL2 is such a protocol by default. Do you think we should do this differently?
Yes, my suggestion here is to explicitly disable SSL2 support altogether. GnuTLS doesn't support it, and having behavior that differs between Linux and Mac can be kind of maddening. I can imagine something like, "embedded browser doesn't work for game X", with lots of "works for me" reports and the occasional "fails here too", only to discover that it works on Mac but not Linux. The additional cost of a difference in behavior doesn't seem worth it, especially when the protocol itself really should have died long ago. --Juan
Hi Juan, On 03/29/13 18:19, Juan Lang wrote:
Hi Jacek,
thanks for the detailed reply.
On Fri, Mar 29, 2013 at 3:02 AM, Jacek Caban <jacek(a)codeweavers.com <mailto:jacek(a)codeweavers.com>> wrote:
Each protocol has two kinds of enable/disable flags: "enabled" and "disabled by default". Those have different default values for each protocol and there are registry setting allowing to change each of them. Only "enabled" protocols are used at all. This patch limits "enabled" protocols to those that we can really support. If an application asks schannel to use default set of protocols (which I'd expect them to do unless they have a good reason), schannel will use all "enabled" protocols that are not "disabled by default". An alternative to default set of protocols is listing each allowed separately.
This means that if protocol is "enabled" and "disabled by default" it won't be used unless application explicitly asks for it. SSL2 is such a protocol by default. Do you think we should do this differently?
Yes, my suggestion here is to explicitly disable SSL2 support altogether. GnuTLS doesn't support it, and having behavior that differs between Linux and Mac can be kind of maddening. I can imagine something like, "embedded browser doesn't work for game X", with lots of "works for me" reports and the occasional "fails here too", only to discover that it works on Mac but not Linux. The additional cost of a difference in behavior doesn't seem worth it, especially when the protocol itself really should have died long ago.
Most of the argument could be used against enabling TLS 1.1 and TLS 1.2, because it's not present on older Macs (nor enabled by default on Windows), so we'll have different behaviour. That's sadly something we have to live with. Anyway, I'm all for trying to avoid using SSL2, but I'd like to be a bit less extreme. How about this patch: http://source.winehq.org/patches/data/95298 With this patch, SSL2 will be completely disabled in default Wine config, but it will be still possible to enable by registries, if someone really needs it. Jacek
Hi Jacek, On Sat, Mar 30, 2013 at 8:36 AM, Jacek Caban <jacek(a)codeweavers.com> wrote:
Most of the argument could be used against enabling TLS 1.1 and TLS 1.2, because it's not present on older Macs (nor enabled by default on Windows), so we'll have different behaviour. That's sadly something we have to live with. Anyway, I'm all for trying to avoid using SSL2, but I'd like to be a bit less extreme. How about this patch:
http://source.winehq.org/patches/data/95298
With this patch, SSL2 will be completely disabled in default Wine config, but it will be still possible to enable by registries, if someone really needs it.
Thanks, this approach looks good to me. --Juan
Hi Ken, On 03/28/13 20:31, Ken Thomases wrote:
On Mar 28, 2013, at 6:05 AM, Jacek Caban wrote:
--- a/dlls/secur32/schannel_macosx.c +++ b/dlls/secur32/schannel_macosx.c @@ -630,6 +630,11 @@ static OSStatus schan_push_adapter(SSLConnectionRef transport, const void *buff, return ret; }
+DWORD schan_imp_enabled_protocols(void) +{ + /* NOTE: No support for TLS 1.1 and TLS 1.2 */ + return SP_PROT_SSL2_CLIENT | SP_PROT_SSL3_CLIENT | SP_PROT_TLS1_0_CLIENT; +} Mac OS X 10.8 introduced support for TLS 1.1 and 1.2. You can test at build time with:
#if MAC_OS_X_VERSION_MAX_ALLOWED >= 1080 ... #else ... #endif
If we want to support building on 10.8 for deployment to earlier versions, we'd do something like:
#if MAC_OS_X_VERSION_MAX_ALLOWED >= 1080 SSLProtocol maxProtocol; if (SSLGetProtocolVersionMax != NULL && SSLGetProtocolVersionMax(context, &maxProtocol) == noErr) { ... compare maxProtocol against kTLSProtocol11 and kTLSProtocol12 ... } ... #else ... #endif
Thanks for the pointers, I've been meaning to explore it as follow-up. My problem is that I'm still on 10.6 with Xcode 3.2. Would you mind taking care of the patch?
The idea is that SSLGetProtocolVersionMax() would be weak linked, so we'd check if it was actually available before calling it. Of course, the other complication is that that function requires a context parameter, but we can create one just for the query if we're interested in the framework capabilities (as opposed to what's been configured for a particular context).
Yes, in this case we're only interested in framework capabilities. We should determine protocols used for given context ourselves, based on caller's requested protocol and confuration, and pass that to framework. Setting up framework is not implemented yet, I have patches for that that I want to test a bit more before sending. Thanks, Jacek
On 3/28/13 8:31 PM, Ken Thomases wrote:
Mac OS X 10.8 introduced support for TLS 1.1 and 1.2.
Can someone with Mac OS X 10.8 test the attached patch for me, please. All I need is to verify that it compiles and when running dlls/secur32/tests/secur32_test.exe.so schannel, TLS 1.1 and TLS 1.2 are listed as supported protocol. Thanks, Jacek
Hi Jacek, On Apr 10, 2013, at 4:24 AM, Jacek Caban wrote:
On 3/28/13 8:31 PM, Ken Thomases wrote:
Mac OS X 10.8 introduced support for TLS 1.1 and 1.2.
Can someone with Mac OS X 10.8 test the attached patch for me, please. All I need is to verify that it compiles and when running dlls/secur32/tests/secur32_test.exe.so schannel, TLS 1.1 and TLS 1.2 are listed as supported protocol.
Thanks for doing this. I don't have a 10.8 build system handy, myself, or I would have done it or would test what you've done. However, Apple's guidance on using weak linking says that you must explicitly compare against NULL. They don't quite say that testing the symbol as a standalone boolean expression won't work, but they do say that using negation (the ! operator) isn't sufficient. https://developer.apple.com/library/mac/documentation/developertools/concept... I'm not sure what the rationale is, but I think there's a peculiarity of what the compiler thinks it knows versus what the linker actually knows. Basically, the compiler figures there's a symbol that will be resolved (or cause a link error), so the check can be optimized away somehow. I think. Other than that, the patch looks good to me. -Ken
Hi Ken, On 04/10/13 16:16, Ken Thomases wrote:
Hi Jacek,
On Apr 10, 2013, at 4:24 AM, Jacek Caban wrote:
On 3/28/13 8:31 PM, Ken Thomases wrote:
Mac OS X 10.8 introduced support for TLS 1.1 and 1.2. Can someone with Mac OS X 10.8 test the attached patch for me, please. All I need is to verify that it compiles and when running dlls/secur32/tests/secur32_test.exe.so schannel, TLS 1.1 and TLS 1.2 are listed as supported protocol. Thanks for doing this. I don't have a 10.8 build system handy, myself, or I would have done it or would test what you've done.
However, Apple's guidance on using weak linking says that you must explicitly compare against NULL. They don't quite say that testing the symbol as a standalone boolean expression won't work, but they do say that using negation (the ! operator) isn't sufficient. https://developer.apple.com/library/mac/documentation/developertools/concept...
I'm not sure what the rationale is, but I think there's a peculiarity of what the compiler thinks it knows versus what the linker actually knows. Basically, the compiler figures there's a symbol that will be resolved (or cause a link error), so the check can be optimized away somehow. I think.
Oh, such restriction seem to indicate a broken design of this weak linking... Following this is fine, through, but perhaps we should use nil to be sure that NULL override in winnt.h won't confuse the heuristic?
Other than that, the patch looks good to me.
Thanks for the review. Cheers, Jacek
On Apr 11, 2013, at 8:49 AM, Jacek Caban wrote:
On 04/10/13 16:16, Ken Thomases wrote:
However, Apple's guidance on using weak linking says that you must explicitly compare against NULL. They don't quite say that testing the symbol as a standalone boolean expression won't work, but they do say that using negation (the ! operator) isn't sufficient. https://developer.apple.com/library/mac/documentation/developertools/concept...
I'm not sure what the rationale is, but I think there's a peculiarity of what the compiler thinks it knows versus what the linker actually knows. Basically, the compiler figures there's a symbol that will be resolved (or cause a link error), so the check can be optimized away somehow. I think.
Oh, such restriction seem to indicate a broken design of this weak linking... Following this is fine, through, but perhaps we should use nil to be sure that NULL override in winnt.h won't confuse the heuristic?
I expect that "nil" is only meaningful in Objective-C code. I'd be surprised if there was anything special about NULL (either Apple's or Wine's), per se, but if you want an alternative that's insulated against any Wine peculiarities, I'd just use a literal "0" (zero).
Other than that, the patch looks good to me.
Thanks for the review.
Happy to help. -Ken
participants (3)
-
Jacek Caban -
Juan Lang -
Ken Thomases