[PATCH] secur32: Split push/pull schannel transports

[Jacek Caban]

On 09.09.2017 02:24, Anton Romanov wrote:
> Jacek,
>
>
>> The code looks generally good and it matches what MSDN states about
>> thread safety of those functions. Still, having little trust to MSDN, I
>> wonder if it would make sense to make them more generally thread safe.
>> We could, for example, store critical section inside schan_context and
>> lock it in all functions accessing it. Did you consider that? It would
>> make all those functions thread safe (with little overhead). It just
>> seems more straightforward solution to me.
> Well, I'd say that would just introduce additional complexity without
> much/any gain. I mean - if MSDN does not state other use is supposed
> to be safe - why would we try and make it?
> Unless there is an app that would actually benefit that.

Yeah, making it all may be questionable, but the solution does not
really add complexity. A simple enter/leave critical section inside
schan_EncryptMessage and schan_DecryptMessage would fix the problem that
you're trying to fix. It would also avoid dependency on backend
libraries to do the right thing (see more below).

>> As for the approach you took, it may be correct. According to [1] that's
>> also how gnutls works. Do you know if Secure Transport on Mac is
>> similar? I couldn't find any documentation about it.
>>
>> Thanks,
>> Jacek
>>
>> [1] https://gnutls.org/manual/html_node/Thread-safety.html
> Unfortunately I am not entirely sure if SecureTransport is (supposed
> to be) thread-safe/unsafe either... Yeah, documentation does not
> really clarify that. Source code [2] is not too clear either.

Actually, looking at the source, I don't see any evidence it allows what
we need. SSLRead/SSLWrite seem to freely access context without any
locking and call functions like SSLHandshakeProceed, which accesses both
read and write related fields. That said, I don't think it guarantees
what we need.

> However, libcurl (which I would trust) states that it is using Secure
> Transport in an "entirely thread safe way" [3],

That page says the opposite. It says that if you share a handle between
threads, you need to guaranty that it's not accessed concurrently
yourself. What we need is read/write threads sharing a handle.

> and I don't see it making any locks when doing sslread/write [4].
>
> [2] https://opensource.apple.com/source/Security/Security-55179.13/libsecurity_ssl/
> [3] https://curl.haxx.se/libcurl/c/threadsafe.html
> [4] https://github.com/curl/curl/blob/master/lib/vtls/darwinssl.c


Cheers,

Jacek