Re: [msmtp-users] Proxy support patch

[CustaiCo <custaico@...373...>]

On Tue, Oct 07, 2014 at 03:56:52PM -0400, grarpamp wrote:
> On Tue, Oct 7, 2014 at 7:46 AM, CustaiCo <custaico@...373...> wrote:
> > I have completed the work to add proxying to msmtp without any
> > dependancy on any other library. Initially I was just linking against
> > the proxychains code.
> 
> socks5 seems the most common, which this patch handles.
> And proxy.c has socks4.
> Though should we add socks4a for completeness?

The original didn't even correctly look things up when using SOCKS5.
What it would actually do is whenever you requested a DNS look up, it
would fork() into the background, calling a script, proxyresolv, that
LD_PRELOAD'd proxychains into dig, then used Google's DNS servers to
resolve the IP. It then stores the output of every DNS look up ever 
done in memory as an int in case you want to connect to that server.

That function really wants to be about 4 functions. I'm certainly not
gonna add new crap to it until there is other cleanup done.

> Lint...
> 
> # fn name in comment doesnt exist
>      33 + * net_proxy_connect()
> # lines are different text
>     175 + * proxychains The 'good parts' version by CustaiCo
>     461 +/* proxychans - The 'good parts' edition - CustaiCo */
> # bad comment form
>     142             len=strlen(user)+1; // username
> 
> > I still
> > haven't changed the auto configuration macros to add proxy.h and proxy.c
> > to the build.

I'd be way less worried about the style of the comments (which all were
incorrect c++ style until I changed them) and more about the other junk
that it pulls. It starts with allocating the the buffer on the stack,
BUFF_SIZE is 8*1024. The HTTP part still just does a random sprintf into
the buffer which is not safe (although the chances of a 8k host getting
passed into it is fairly small). It used to call strcat about 15 times
directly after that sprintf. I think all of those I replaced with safer
versions that don't overflow and check to see if their output got truncated.

> # don't think this is actually needed given HAVE_PROXY exists
> src/proxy.h:18:#ifndef __PROXY_HEADER
> 
> 
> net.c: In function 'net_connect_proxy':
> net.c:358: warning: implicit declaration of function 'tunnel_to'
> net.c:358: error: 'SOCKS5_TYPE' undeclared (first use in this function)
> 
> I must have missed something to still get the above.

It's a header guard. all the header files have one, although I should've
changed it to the style the rest of the code uses of using FILE_H and
instead of the __ thing. If you get those two errors it means proxy.h
wasn't included for some reason. 

Without it included into the autoconfigure stuff, it doesn't correctly make
or use proxy.c in the build. If you change parts of it after fixing the
monster of a Makefile that script builds, doing a clean or reconfiguring
anything and then remaking it undoes your work.

> > Included is completely untested ipv6 support when using a proxy. I've
> > written it in a way that I *think* ipv6 should but I have no way to
> > actually test this due to lack of one that supports the protocol.
> 
> Dante or some of the other socks5 tools listed might have an IPv6
> server that could be easily tested against through ports on ::1. ie:
> msmtp -> [::1]:1111 (dante socks5) -> [::1]:2222 (netcat TCP listen)

It could be done, but honestly, I am putting far more effort into this
than I am getting out of it. Dealing with it after having dealt with the
horror that was the proxychains code is too much.

CustaiCo