[Spice-devel] [PATCH spice-server v3 01/12] red-stream: Simplify mechname matching

[Frediano Ziglio]

> Hi

> On 12/22/2017 12:07 PM, Frediano Ziglio wrote:

> > Avoid over complicated matching using quoting and a simple strstr
> 
> > operation.
> 
> > The mech names are separated and quoted with the same chararacter (',')
> 
> > making possible to search for ",MECHNAME," instead of manually check for
> 
> > prefix and suffix after the search for "MECHNAME".
> 

> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com> ---
> 
> > Changes since v1:
> 
> > - use strstr == NULL instead of !strstr;
> 
> > - improve commit message.
> 
> > ---
> 
> > server/red-stream.c | 30 +++++++++---------------------
> 
> > 1 file changed, 9 insertions(+), 21 deletions(-)
> 

> > diff --git a/server/red-stream.c b/server/red-stream.c
> 
> > index 55e2c3e02..f4637808c 100644
> 
> > --- a/server/red-stream.c
> 
> > +++ b/server/red-stream.c
> 
> > @@ -744,6 +744,7 @@ static int auth_sasl_check_ssf(RedSASL *sasl, int
> > *runSSF)
> 
> > * u8-array serverout-strin
> 
> > * u8 continue
> 
> > */
> 
> > +#define SASL_MAX_MECHNAME_LEN 100
> 
> > #define SASL_DATA_MAX_LEN (1024 * 1024)
> 
> > 
> 
> > RedSaslError red_sasl_handle_auth_step(RedStream *stream, AsyncReadDone
> > read_cb, void *opaque)
> 
> > @@ -981,24 +982,11 @@ bool red_sasl_handle_auth_mechname(RedStream *stream,
> > AsyncReadDone read_cb, voi
> 
> > spice_debug("Got client mechname '%s' check against '%s'",
> 
> > sasl->mechname, sasl->mechlist);
> 
> > 
> 
> > -    if (strncmp(sasl->mechlist, sasl->mechname, sasl->len) == 0) {
> 
> > -        if (sasl->mechlist[sasl->len] != '\0' &&
> 
> > -            sasl->mechlist[sasl->len] != ',') {
> 
> > -            spice_debug("One %d", sasl->mechlist[sasl->len]);
> 
> > -            return false;
> 
> > -        }
> 
> > -    } else {
> 
> > -        char *offset = strstr(sasl->mechlist, sasl->mechname);
> 
> > -        spice_debug("Two %p", offset);
> 
> > -        if (!offset) {
> 
> > -            return false;
> 
> > -        }
> 
> > -        spice_debug("Two '%s'", offset);
> 
> > -        if (offset[-1] != ',' ||
> 
> > -            (offset[sasl->len] != '\0'&&
> 
> > -             offset[sasl->len] != ',')) {
> 
> > -            return false;
> 
> > -        }
> 
> > +    char quoted_mechname[SASL_MAX_MECHNAME_LEN + 4];
> 

> Could it be + 2 ?

No, at least 3 (2 for ',' separators and 1 for NUL terminator).

> > +    sprintf(quoted_mechname, ",%s,", sasl->mechname);
> 
> > +
> 
> > +    if (strstr(sasl->mechlist, quoted_mechname) == NULL) {
> 
> > +        return false;
> 
> > }
> 
> > 
> 
> > spice_debug("Validated mechname '%s'", sasl->mechname);
> 
> > @@ -1013,7 +1001,7 @@ bool red_sasl_handle_auth_mechlen(RedStream *stream,
> > AsyncReadDone read_cb, void
> 
> > {
> 
> > RedSASL *sasl = &stream->priv->sasl;
> 
> > 
> 
> > -    if (sasl->len < 1 || sasl->len > 100) {
> 
> > +    if (sasl->len < 1 || sasl->len > SASL_MAX_MECHNAME_LEN) {
> 
> > spice_warning("Got bad client mechname len %d", sasl->len);
> 
> > return false;
> 
> > }
> 
> > @@ -1106,9 +1094,9 @@ bool red_sasl_start_auth(RedStream *stream,
> > AsyncReadDone read_cb, void *opaque)
> 
> > 
> 
> > err = sasl_listmech(sasl->conn,
> 
> > NULL, /* Don't need to set user */
> 
> > -                        "", /* Prefix */
> 
> > +                        ",", /* Prefix */
> 
> > ",", /* Separator */
> 
> > -                        "", /* Suffix */
> 
> > +                        ",", /* Suffix */
> 
> > &mechlist,
> 
> > NULL,
> 
> > NULL);
> 

> Acked-by: Snir Sheriber <ssheribe at redhat.com>

Frediano