Re: [PATCH] secur32: Split push/pull schannel transports

[Anton Romanov]

On Sat, Aug 5, 2017 at 11:27 AM, Anton Romanov  wrote:
> This makes schan_EncryptMessage/schan_DecryptMessage thread safe between
> each other.
> MSDN states that it's supposed to be thread safe.
> Fixes Magic The Gathering: Online crash:
> https://bugs.winehq.org/show_bug.cgi?id=43453
> Signed-off-by: Anton Romanov
>
> dlls/secur32/schannel.c        | 33 +++++++++++++++++++--------------
>  dlls/secur32/schannel_gnutls.c | 12 +++++++-----
>  dlls/secur32/schannel_macosx.c | 21 ++++++++++++---------
>  dlls/secur32/secur32_priv.h    |  5 +++--
>  4 files changed, 41 insertions(+), 30 deletions(-)
>
> diff --git a/dlls/secur32/schannel.c b/dlls/secur32/schannel.c
> index 82374efd55..1f938e1a37 100644
> --- a/dlls/secur32/schannel.c
> +++ b/dlls/secur32/schannel.c
> @@ -59,6 +59,8 @@ struct schan_context
>      schan_imp_session session;
>      ULONG req_ctx_attr;
>      const CERT_CONTEXT *cert;
> - struct schan_transport *push_transport;
> - struct schan_transport *pull_transport;
>
> };
>
> static struct schan_handle *schan_handle_table;
> @@ -895,7 +897,7 @@ static SECURITY_STATUS SEC_ENTRY schan_InitializeSecurityContextW(
>      init_schan_buffers(&transport.in, pInput, schan_init_sec_ctx_get_next_input_buffer);
>      transport.in.limit = expected_size;
>      init_schan_buffers(&transport.out, pOutput, schan_init_sec_ctx_get_next_output_buffer);
> - schan_imp_set_session_transport(ctx->session, &transport);
> -
>
> schan_imp_set_session_transports(ctx->session, &transport, &transport);
>
> /* Perform the TLS handshake */
> ret = schan_imp_handshake(ctx->session);
>
> @@ -927,6 +929,13 @@ static SECURITY_STATUS SEC_ENTRY schan_InitializeSecurityContextW(
>      if (ctx->req_ctx_attr & ISC_REQ_STREAM)
>          *pfContextAttr |= ISC_RET_STREAM;
> - ctx->push_transport = HeapAlloc(GetProcessHeap(), 0, sizeof(struct schan_transport));
> - ctx->push_transport->ctx = ctx;
> - ctx->pull_transport = HeapAlloc(GetProcessHeap(), 0, sizeof(struct schan_transport));
> - ctx->pull_transport->ctx = ctx;
> - schan_imp_set_session_transports(ctx->session, ctx->push_transport,
> -
>        ctx->pull_transport);
> - return ret;
>
> }
>
> @@ -1205,7 +1214,6 @@ static int schan_encrypt_message_get_next_buffer_token(const struct schan_transp
>  static SECURITY_STATUS SEC_ENTRY schan_EncryptMessage(PCtxtHandle context_handle,
>          ULONG quality, PSecBufferDesc message, ULONG message_seq_no)
>  {
> - struct schan_transport transport;
> struct schan_context *ctx;
> struct schan_buffers *b;
> SECURITY_STATUS status;
>
> @@ -1235,13 +1243,11 @@ static SECURITY_STATUS SEC_ENTRY schan_EncryptMessage(PCtxtHandle context_handle
>      data = HeapAlloc(GetProcessHeap(), 0, data_size);
>      memcpy(data, buffer->pvBuffer, data_size);
> - transport.ctx = ctx;
> - init_schan_buffers(&transport.in, NULL, NULL);
> - init_schan_buffers(&ctx->push_transport->in, NULL, NULL);
> if (schan_find_sec_buffer_idx(message, 0, SECBUFFER_STREAM_HEADER) != -1)
> -
>    init_schan_buffers(&transport.out, message, schan_encrypt_message_get_next_buffer);
> -
>    init_schan_buffers(&ctx->push_transport->out, message, schan_encrypt_message_get_next_buffer);
> else
> -
>    init_schan_buffers(&transport.out, message, schan_encrypt_message_get_next_buffer_token);
> - schan_imp_set_session_transport(ctx->session, &transport);
> -
>    init_schan_buffers(&ctx->push_transport->out, message, schan_encrypt_message_get_next_buffer_token);
>
> length = data_size;
>    status = schan_imp_send(ctx->session, data, &length);
>
> @@ -1251,7 +1257,7 @@ static SECURITY_STATUS SEC_ENTRY schan_EncryptMessage(PCtxtHandle context_handle
>      if (length != data_size)
>          status = SEC_E_INTERNAL_ERROR;
> - b = &transport.out;
> - b = &ctx->push_transport->out;
> b->desc->pBuffers[b->current_buffer_idx].cbBuffer = b->offset;
> HeapFree(GetProcessHeap(), 0, data);
>
> @@ -1327,7 +1333,6 @@ static void schan_decrypt_fill_buffer(PSecBufferDesc message, ULONG buffer_type,
>  static SECURITY_STATUS SEC_ENTRY schan_DecryptMessage(PCtxtHandle context_handle,
>          PSecBufferDesc message, ULONG message_seq_no, PULONG quality)
>  {
> - struct schan_transport transport;
> struct schan_context *ctx;
> SecBuffer *buffer;
> SIZE_T data_size;
>
> @@ -1371,11 +1376,9 @@ static SECURITY_STATUS SEC_ENTRY schan_DecryptMessage(PCtxtHandle context_handle
>      data_size = expected_size - 5;
>      data = HeapAlloc(GetProcessHeap(), 0, data_size);
> - transport.ctx = ctx;
> - init_schan_buffers(&transport.in, message, schan_decrypt_message_get_next_buffer);
> - transport.in.limit = expected_size;
> - init_schan_buffers(&transport.out, NULL, NULL);
> - schan_imp_set_session_transport(ctx->session, &transport);
> -
>
> init_schan_buffers(&ctx->pull_transport->in, message, schan_decrypt_message_get_next_buffer);
> -
>
> ctx->pull_transport->in.limit = expected_size;
> -
>
> init_schan_buffers(&ctx->pull_transport->out, NULL, NULL);
>
> while (received < data_size)
> {
>
> @@ -1573,6 +1576,8 @@ void SECUR32_deinitSchannelSP(void)
>          {
>              struct schan_context *ctx = schan_free_handle(i, SCHAN_HANDLE_CTX);
>              schan_imp_dispose_session(ctx->session);
> -
>        HeapFree(GetProcessHeap(), 0, ctx->push_transport);
> -
>        HeapFree(GetProcessHeap(), 0, ctx->pull_transport);
>        HeapFree(GetProcessHeap(), 0, ctx);
>    }
> }
>
> diff --git a/dlls/secur32/schannel_gnutls.c b/dlls/secur32/schannel_gnutls.c
> index bc3bbaf67d..97b2c2d767 100644
> --- a/dlls/secur32/schannel_gnutls.c
> +++ b/dlls/secur32/schannel_gnutls.c
> @@ -73,7 +73,7 @@ MAKE_FUNCPTR(gnutls_record_send);
>  MAKE_FUNCPTR(gnutls_server_name_set);
>  MAKE_FUNCPTR(gnutls_transport_get_ptr);
>  MAKE_FUNCPTR(gnutls_transport_set_errno);
> -MAKE_FUNCPTR(gnutls_transport_set_ptr);
> +MAKE_FUNCPTR(gnutls_transport_set_ptr2);
>  MAKE_FUNCPTR(gnutls_transport_set_pull_function);
>  MAKE_FUNCPTR(gnutls_transport_set_push_function);
>  #undef MAKE_FUNCPTR
> @@ -214,11 +214,13 @@ void schan_imp_dispose_session(schan_imp_session session)
>      pgnutls_deinit(s);
>  }
>
> -void schan_imp_set_session_transport(schan_imp_session session,
> -
>                                 struct schan_transport *t)
>
> +void schan_imp_set_session_transports(schan_imp_session session,
> -
>                                 struct schan_transport *t_push,
> -
>                                 struct schan_transport *t_pull)
>
> {
>      gnutls_session_t s = (gnutls_session_t)session;
> - pgnutls_transport_set_ptr(s, (gnutls_transport_ptr_t)t);
> - pgnutls_transport_set_ptr2(s, (gnutls_transport_ptr_t)t_pull,
> -
>        (gnutls_transport_ptr_t)t_push);
>
> }
>
> void schan_imp_set_session_target(schan_imp_session session, const char *target)
> @@ -568,7 +570,7 @@ BOOL schan_imp_init(void)
>      LOAD_FUNCPTR(gnutls_server_name_set)
>      LOAD_FUNCPTR(gnutls_transport_get_ptr)
>      LOAD_FUNCPTR(gnutls_transport_set_errno)
> - LOAD_FUNCPTR(gnutls_transport_set_ptr)
> - LOAD_FUNCPTR(gnutls_transport_set_ptr2)
> LOAD_FUNCPTR(gnutls_transport_set_pull_function)
> LOAD_FUNCPTR(gnutls_transport_set_push_function)
>
> #undef LOAD_FUNCPTR
> diff --git a/dlls/secur32/schannel_macosx.c b/dlls/secur32/schannel_macosx.c
> index 7f38133b4b..4cd562411b 100644
> --- a/dlls/secur32/schannel_macosx.c
> +++ b/dlls/secur32/schannel_macosx.c
> @@ -184,7 +184,8 @@ enum {
>
> struct mac_session {
>      SSLContextRef context;
> - struct schan_transport *transport;
> - struct schan_transport *push_transport;
> - struct schan_transport *pull_transport;
>
> };
>
> @@ -631,9 +632,9 @@ static OSStatus schan_pull_adapter(SSLConnectionRef transport, void *buff,
>      int status;
>      OSStatus ret;
> - TRACE("(%p/%p, %p, %p/%lu)\n", s, s->transport, buff, buff_len, *buff_len);
> - TRACE("(%p/%p, %p, %p/%lu)\n", s, s->pull_transport, buff, buff_len, *buff_len);
> - status = schan_pull(s->transport, buff, buff_len);
> - status = schan_pull(s->pull_transport, buff, buff_len);
> if (status == 0)
> {
>     if (*buff_len == 0)
>
> @@ -690,9 +691,9 @@ static OSStatus schan_push_adapter(SSLConnectionRef transport, const void *buff,
>      int status;
>      OSStatus ret;
> - TRACE("(%p/%p, %p, %p/%lu)\n", s, s->transport, buff, buff_len, *buff_len);
> - TRACE("(%p/%p, %p, %p/%lu)\n", s, s->push_transport, buff, buff_len, *buff_len);
> - status = schan_push(s->transport, buff, buff_len);
> - status = schan_push(s->push_transport, buff, buff_len);
> if (status == 0)
> {
>     TRACE("Pushed %lu bytes\n", *buff_len);
>
> @@ -806,14 +807,16 @@ void schan_imp_dispose_session(schan_imp_session session)
>      HeapFree(GetProcessHeap(), 0, s);
>  }
>
> -void schan_imp_set_session_transport(schan_imp_session session,
> -
>                                 struct schan_transport *t)
>
> +void schan_imp_set_session_transports(schan_imp_session session,
> -
>                                 struct schan_transport *t_push,
> -
>                                 struct schan_transport *t_pull)
>
> {
>      struct mac_session *s = (struct mac_session*)session;
> - TRACE("(%p/%p, %p)\n", s, s->context, t);
> - TRACE("(%p/%p, %p, %p)\n", s, s->context, t_push, t_pull);
> - s->transport = t;
> - s->push_transport = t_push;
> - s->pull_transport = t_pull;
>
> }
>
> void schan_imp_set_session_target(schan_imp_session session, const char *target)
> diff --git a/dlls/secur32/secur32_priv.h b/dlls/secur32/secur32_priv.h
> index 9a60d65788..0bc8a57c1a 100644
> --- a/dlls/secur32/secur32_priv.h
> +++ b/dlls/secur32/secur32_priv.h
> @@ -246,8 +246,9 @@ extern schan_imp_session schan_session_for_transport(struct schan_transport* t)
>  /* schannel implementation interface */
>  extern BOOL schan_imp_create_session(schan_imp_session *session, schan_credentials *cred) DECLSPEC_HIDDEN;
>  extern void schan_imp_dispose_session(schan_imp_session session) DECLSPEC_HIDDEN;
> -extern void schan_imp_set_session_transport(schan_imp_session session,
> -
>                                        struct schan_transport *t) DECLSPEC_HIDDEN;
>
> +extern void schan_imp_set_session_transports(schan_imp_session session,
> -
>                                        struct schan_transport *t_push,
> -
>                                        struct schan_transport *t_pull) DECLSPEC_HIDDEN;
>
> extern void schan_imp_set_session_target(schan_imp_session session, const char *target) DECLSPEC_HIDDEN;
>  extern SECURITY_STATUS schan_imp_handshake(schan_imp_session session) DECLSPEC_HIDDEN;
>  extern unsigned int schan_imp_get_session_cipher_block_size(schan_imp_session session) DECLSPEC_HIDDEN;
> --
> 2.11.0

So, it's been a month now. Is there something I could do to have this accepted?

Thanks.

[PATCH] secur32: Split push/pull schannel transports

[Alexandre Julliard]

Anton Romanov  writes:
> So, it's been a month now. Is there something I could do to have this accepted?

It's assigned to Jacek for review, but he's on vacation until tomorrow I
believe, so it may be a few more days until he gets around to it. Sorry
about the delay.

[PATCH] secur32: Split push/pull schannel transports

[Anton Romanov]

On Wed, Sep 6, 2017 at 7:50 AM, Alexandre Julliard  wrote:
> Anton Romanov  writes:
> > So, it's been a month now. Is there something I could do to have this accepted?
>
> It's assigned to Jacek for review, but he's on vacation until tomorrow I
> believe, so it may be a few more days until he gets around to it. Sorry
> about the delay.

I see, thanks!

[PATCH] secur32: Split push/pull schannel transports

[Jacek Caban]

Hi Anton,

On 06.09.2017 18:43, Anton Romanov wrote:
> On Wed, Sep 6, 2017 at 7:50 AM, Alexandre Julliard  wrote:
> > Anton Romanov  writes:
> > > So, it's been a month now. Is there something I could do to have this accepted?
> >
> > It's assigned to Jacek for review, but he's on vacation until tomorrow I
> > believe, so it may be a few more days until he gets around to it. Sorry
> > about the delay.
>
> I see, thanks!

I'm sorry for the delay. I actually started reviewing it before
vacations, had some concerns and didn't manage to take deeper look at this.

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.

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

[PATCH] secur32: Split push/pull schannel transports

[Anton Romanov]

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.
> 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.
However, libcurl (which I would trust) states that it is using Secure
Transport in an "entirely thread safe way" [3], 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_s...
[3] https://curl.haxx.se/libcurl/c/threadsafe.html
[4] https://github.com/curl/curl/blob/master/lib/vtls/darwinssl.c

[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_s...
> [3] https://curl.haxx.se/libcurl/c/threadsafe.html
> [4] https://github.com/curl/curl/blob/master/lib/vtls/darwinssl.c

Cheers,

Jacek

[PATCH] secur32: Split push/pull schannel transports

[Anton Romanov]

> > > 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).

Well, my point is that adding CS there would introduce contention in a
place where app tries to avoid it.
> > > 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.

It does say that there is locking needed for libcurl's general stuff.
I was talking about TLS section in that page specifically. But sure, I
might be reading it wrong.

So, anyway, would adding CS just for SecureTransport code path be fine?