adapt pam_winbin.c to deal with later iniparser versions

Noel Power nopower at suse.com
Tue Feb 11 03:14:47 MST 2014


On 10/02/14 18:41, Jeremy Allison wrote:
[...]
> Ok, now I've looked over the iniparser update, things
> like this:
>
> +/*-------------------------------------------------------------------------*/
> +/**
> +  @brief    Remove blanks at the beginning and the end of a string.
> +  @param    s   String to parse.
> +  @return   ptr to statically allocated string.
> +
> +  This function returns a pointer to a statically allocated string,
> +  which is identical to the input string, except that all blank
> +  characters at the end and the beg. of the string have been removed.
> +  Do not free or modify the returned string! Since the returned string
> +  is statically allocated, it will be modified at each function call
> +  (not re-entrant).
> + */
> +/*--------------------------------------------------------------------------*/
> +static char * strstrip(const char * s)
> +{
> +    static char l[ASCIILINESZ+1];
> +    char * last ;
> +    
> +    if (s==NULL) return NULL ;
> +    
> +    while (isspace((int)*s) && *s) s++;
> +    memset(l, 0, ASCIILINESZ+1);
> +    strcpy(l, s);
> +    last = l + strlen(l);
> +    while (last > l) {
> +        if (!isspace((int)*(last-1)))
> +            break ;
> +        last -- ;
> +    }
> +    *last = (char)0;
> +    return (char*)l ;
> +}
>
> scare the *crap* out of me :-(.
:-)
>
> It's the unconstrained strcpy into a static
> buffer l from passed in value s that does it :-).
I don't think it's as unconstrained as you might think, strstrip is an
internal function and afaics internally all strings iniparser uses are
size bound by ASCIILINESZ and thus should be safe.
> At the very least this should be strncpy.
>
> There's a *reason* we have:
>
> #define strcpy(dest,src) __ERROR__XX__NEVER_USE_STRCPY___;
>
> in Samba :-).
be that as it may I suppose its impossible to impose samba code
guidelines on an external project :-/, if there is a security hole then
of course it should be fixed in the upstream version (and sure
temporarily fixed internally if absolutely necessary). But that begs the
question do we really want to continue to maintain effectively a fork of
iniparser? My hope with updating beside gaining whatever
improvements/fixes the upstream version has was that it would be a step
towards eventually removing the internal copy
>
> They aren't in the source code that's in the
> current master tree.
seems similar code does exist, see (master) src/iniparser/strlib.c (
strcrop & strstrip )
>
> I have to NAK this unless we can also add something
> that fixes up these uses. Sorry :-(.
>
so be it then

Noel


More information about the samba-technical mailing list