Re: secur32: Take schannel backend capabilities into account when configuring enabled protocols.

[Ken Thomases]

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

secur32: Take schannel backend capabilities into account when configuring enabled protocols.

[Juan Lang]

On Thu, Mar 28, 2013 at 12:31 PM, 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;

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

secur32: Take schannel backend capabilities into account when configuring enabled protocols.

[Jacek Caban]

Hi Juan,

On 03/28/13 21:55, Juan Lang wrote:
> On Thu, Mar 28, 2013 at 12:31 PM, Ken Thomases <[e-mail]
> > 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

secur32: Take schannel backend capabilities into account when configuring enabled protocols.

[Juan Lang]

Hi Jacek,

thanks for the detailed reply.

On Fri, Mar 29, 2013 at 3:02 AM, Jacek Caban  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

secur32: Take schannel backend capabilities into account when configuring enabled protocols.

[Jacek Caban]

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 <[e-mail]
> > 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

secur32: Take schannel backend capabilities into account when configuring enabled protocols.

[Juan Lang]

Hi Jacek,

On Sat, Mar 30, 2013 at 8:36 AM, Jacek Caban  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

secur32: Take schannel backend capabilities into account when configuring enabled protocols.

[Jacek Caban]

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

secur32: Take schannel backend capabilities into account when configuring enabled protocols.

[Jacek Caban]

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

secur32: Take schannel backend capabilities into account when configuring enabled protocols.

[Ken Thomases]

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

secur32: Take schannel backend capabilities into account when configuring enabled protocols.

[Jacek Caban]

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

secur32: Take schannel backend capabilities into account when configuring enabled protocols.

[Ken Thomases]

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