[vlc-devel] [PATCH] network/tcp: socks: prevent strlen( NULL )

[Filip Roséen]

Hi J-B,

On 2017-03-03 16:18, Jean-Baptiste Kempf wrote:

> I don't like this patch too much, tbh.
>
> IMHO, you should do a different if and a different message inside the
> 0x02 case, else, it's undebuggable for the user.

I do understand the initial thought about that the diagnostic should
maybe not be shared between the cases, but with the current
implementation (and the patch in which this email is about) the logs
would contain an call to `msg_Err` stating *"socks: authentication
needed"* (given that `b_auth = false` for the path in question) which
I initially thought was enough.

As also pointed out by *courmisch* in `#videolan`, tokens that are
shorter than `1` character are not acceptable as *username/password*
in the [*SOCKS* specification][1]:

    This begins with the client producing a Username/Password request:
    
               +----+------+----------+------+----------+
               |VER | ULEN |  UNAME   | PLEN |  PASSWD  |
               +----+------+----------+------+----------+
               | 1  |  1   | 1 to 255 |  1   | 1 to 255 |
               +----+------+----------+------+----------+

[1]: https://tools.ietf.org/html/rfc1929#section-2

See attached patch for one that tries to elaborate the diagnostic by
stating, more explicitly, what is going on; I hope that this is enough
as it would make me rest easier knowing that at least one potential
null-dereference is gone from the codebase.

Thanks,\
Filip Roséen

> On Wed, 1 Mar 2017, at 09:52, Filip Roséen wrote:
> > The previous implementation assumed that the remote entity would not
> > ask for authentication unless we have stated that we have the
> > capability for such, we should however not trust this assumption given
> > that it would (if it happens) cause problems with the username /
> > password being NULL.
> > ---
> >  src/network/tcp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/network/tcp.c b/src/network/tcp.c
> > index 4f4d4701f6..b85852de22 100644
> > --- a/src/network/tcp.c
> > +++ b/src/network/tcp.c
> > @@ -359,7 +359,7 @@ static int SocksNegotiate( vlc_object_t *p_obj,
> >      {
> >          msg_Dbg( p_obj, "socks: no authentication required" );
> >      }
> > -    else if( buffer[1] == 0x02 )
> > +    else if( buffer[1] == 0x02 && psz_socks_user && psz_socks_passwd )
> >      {
> >          int i_len1 = __MIN( strlen(psz_socks_user), 255 );
> >          int i_len2 = __MIN( strlen(psz_socks_passwd), 255 );
> > -- 
> > 2.12.0
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170306/e3c01523/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-network-tcp-socks-prevent-strlen-NULL.patch
Type: text/x-diff
Size: 1445 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170306/e3c01523/attachment.patch>