[Spice-devel] [PATCH spice-server 03/11] reds: Remove possible leak during SASL authentication

[Frediano Ziglio]

> 
> On 12/11/2017 12:28 PM, Frediano Ziglio wrote:
> > We need to free the connection if the mechanism name is wrong
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> 
> Acked-by: Uri Lublin <uril at redhat.com>
> 
> Looking at reds_handle_auth_mechname() and reds_handle_auth_mechlen(),
> one is calling reds_link_free the other reds_send_link_error.
> This patch fixes one of them.
> 
> Uri.
> 

See my last series.
The protocol is actually a bit weird. When you are using SASL for authentication
turns out you change a bit the protocol to a rountrip of:
server:
- u32 len
- u8[len] data 
- u8 continue or not
client
- u32 len
- u8[len] data
For these reds_send_link_error code send a SpiceLinkHeader + SpiceLinkReply
which are:
typedef struct SPICE_ATTR_PACKED SpiceLinkHeader {
    uint32_t magic;
    uint32_t major_version;
    uint32_t minor_version;
    uint32_t size;
} SpiceLinkHeader;
typedef struct SPICE_ATTR_PACKED SpiceLinkReply {
    uint32_t error;
    uint8_t pub_key[SPICE_TICKET_PUBKEY_BYTES];
    uint32_t num_common_caps;
    uint32_t num_channel_caps;
    uint32_t caps_offset;
} SpiceLinkReply;

so basically the client will receive a SpiceLinkHeader::magic (which is always SPICE_MAGIC)
instead of data len. Now, not a big issue, SPICE_MAGIC is "REDQ" and there's a check if data
len is not bigger than 1MB so SPICE_MAGIC will be rejected but I'm not sure this is really
handled by the client. In the first git commit the code is:

static void reds_send_link_result(RedLinkInfo *link, uint32_t error)
{
    sync_write(link->peer, &error, sizeof(error));
}

So this was more expected (in this case the question is how to distinguish a result and
a data len, at that commit SASL was not implemented so I suspect was implemented when
this result was in place but nobody changed SASL after result protocol was changed).

> > ---
> >   server/reds.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index e7b95980..384ebc58 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -2202,6 +2202,7 @@ static void reds_handle_auth_mechname(void *opaque)
> >   
> >       if (!red_sasl_handle_auth_mechname(link->stream,
> >       reds_handle_auth_startlen, link)) {
> >               reds_send_link_error(link, SPICE_LINK_ERR_INVALID_DATA);
> > +        reds_link_free(link);
> >       }
> >   }
> >   
> > 
> 
>