Re: [PATCH v2 1/2] secur32: Fix race between schan_(Encrypt|Decrypt)Message

[Jacek Caban]

Hi Anton,

On 14.09.2017 18:30, Anton Romanov wrote:
> ctx->req_ctx_attr = fContextReq;
>
> -    transport.ctx = ctx;
> -    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);
> +    ctx->transport.ctx = ctx;

One last thing, please move this line closer to
schan_imp_create_session, where it belongs. transport.ctx should be set
once, during context creation, and not touched later. Other than that,
the patch looks good.


Thanks,

Jacek

[PATCH v2 1/2] secur32: Fix race between schan_(Encrypt|Decrypt)Message

[Anton Romanov]

On Thu, Sep 14, 2017 at 10:57 AM, Jacek Caban <jacek(a)codeweavers.com> wrote:
> Hi Anton,
>
> On 14.09.2017 18:30, Anton Romanov wrote:
> > ctx->req_ctx_attr = fContextReq;
> >
> > -    transport.ctx = ctx;
> > -    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);
> > +    ctx->transport.ctx = ctx;
>
> One last thing, please move this line closer to
> schan_imp_create_session, where it belongs. transport.ctx should be set
> once, during context creation, and not touched later. Other than that,
> the patch looks good.
> But ctx is also being set in the else branch, just couple of lines
> above the changed ones [1]
> So, moving it closer to schan_imp_create_session would be wrong, wouldn't it?

1. https://github.com/theli-ua/wine/blob/5c2a7a64e1866f97d9522e310e2c881c704adf...

[PATCH v2 1/2] secur32: Fix race between schan_(Encrypt|Decrypt)Message

[Jacek Caban]

On 15.09.2017 07:05, Anton Romanov wrote:
> On Thu, Sep 14, 2017 at 10:57 AM, Jacek Caban <jacek(a)codeweavers.com> wrote:
> > Hi Anton,
> >
> > On 14.09.2017 18:30, Anton Romanov wrote:
> > > ctx->req_ctx_attr = fContextReq;
> > >
> > > -    transport.ctx = ctx;
> > > -    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);
> > > +    ctx->transport.ctx = ctx;
> >
> > One last thing, please move this line closer to
> > schan_imp_create_session, where it belongs. transport.ctx should be set
> > once, during context creation, and not touched later. Other than that,
> > the patch looks good.
> > But ctx is also being set in the else branch, just couple of lines
> > above the changed ones [1]
> > So, moving it closer to schan_imp_create_session would be wrong, wouldn't it?
>
> 1. https://github.com/theli-ua/wine/blob/5c2a7a64e1866f97d9522e310e2c881c704adf...

This else branch uses already created context. Those are contexts that
are created in if (!phContext) branch in earlier
schan_InitializeSecurityContextW call. Once created, context pointer
never changes, so if you set it once, it will still be valid in
subsequent calls. Or am I missing something?


Thanks,

Jacek