Le 02/04/2017 à 22:19, Samuel Thibault a écrit :
> Hello,
Hi,
> Thanks for the patch!
>
> Laurent Vivier, on mar. 28 mars 2017 21:07:56 +0200, wrote:
>> @@ -617,6 +621,10 @@ void slirp_pollfds_poll(GArray *pollfds, int
>> select_error)
>> * Check sockets for reading
>> */
>> else if (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR)) {
>> + if (so->so_state & SS_ISFCONNECTING) {
>> + socks5_recv(so->s, &so->so_proxy_state);
>> + continue;
>> + }
>
> It looks odd to be calling socks5_recv in all cases. Shouldn't this
> check for so_proxy_state != 0?
>
We don't need, because we poll for G_IO_IN in SS_ISFCONNECTING state
only if so_proxy_state != 0 (see slirp_pollfds_fill() part)
>> @@ -645,11 +653,19 @@ void slirp_pollfds_poll(GArray *pollfds, int
>> select_error)
>> /*
>> * Check for non-blocking, still-connecting sockets
>> */
>> - if (so->so_state & SS_ISFCONNECTING) {
>> - /* Connected */
>> - so->so_state &= ~SS_ISFCONNECTING;
>>
>> - ret = send(so->s, (const void *) &ret, 0, 0);
>> + if (so->so_state & SS_ISFCONNECTING) {
>> + ret = socks5_send(so->s, slirp->proxy_user,
>
> Ditto. Socks5 and non-socks5 code should be properly separated I guess.
The socks5_send() behavior is based on so_proxy_state, and once the
proxy is connected all must be as if we don't have a proxy, so it seems
logic to interleave it there.
>> @@ -1069,6 +1085,48 @@ int slirp_add_exec(Slirp *slirp, int do_pty, const
>> void *args,
>> htons(guest_port));
>> }
>>
>> +int slirp_add_proxy(Slirp *slirp, const char *server, int port,
>> + const char *user, const char *password)
>> +{
>> + int fd;
>> + socks5_state_t state;
>> + struct sockaddr_storage addr;
>> +
>> + /* check the connection */
>
> Please be a bit more verbose :) For instance:
>
>> + /* just check that the connection to the socks5 server works with
>> the given credentials, and close without doing anything with it. */
>
OK
>
>> diff --git a/slirp/socks5.c b/slirp/socks5.c
>> new file mode 100644
>> index 0000000..1c5e071
>> --- /dev/null
>> +++ b/slirp/socks5.c
>> @@ -0,0 +1,284 @@
>> +/* some parts from nmap/ncat GPLv2 */
>
>
> One can't just steal code like this. You *have* to copy over the
> copyright notice, since there's notably the author information.
Sorry, I've been naive...
Well, for ncat.h license is 281 lines long (with clarifications and
extensions) for 60 lines copied... I think it is better to rewrite this
part from the RFC1928.
>> +static int socks5_proxy_connect(int fd, const char *server, int port)
>> +{
>> + struct sockaddr_in saddr;
>> + struct hostent *he;
>> +
>> + he = gethostbyname(server);
>> + if (he == NULL) {
>> + errno = EINVAL;
>> + return -1;
>> + }
>> + saddr.sin_family = AF_INET;
>> + saddr.sin_addr = *(struct in_addr *)he->h_addr;
>> + saddr.sin_port = htons(port);
>
> Please add a TODO: v6 version
OK
>
>> +static int socks5_recv_authenticate(int fd)
>> +{
>> + char socksbuf[2];
>> + if (recv(fd, socksbuf, 2, 0) != 2) {
>> + return -1;
>> + }
>> + if (socksbuf[0] != 1 || socksbuf[1] != 0) {
>
> These look like magic numbers :) Please document what they represent.
OK
>
>> + int len;
>> +
>> + memset(&socks5msg, 0, sizeof(socks5msg));
>> +
>> + socks5msg.ver = SOCKS5_VERSION;
>> + socks5msg.cmd = SOCKS_CONNECT;
>> + socks5msg.rsv = 0;
>
> Please rather set len to 4 here, and increment later on
>
>> + switch (addr->ss_family) {
>> + case AF_INET: {
>> + struct sockaddr_in *a = (struct sockaddr_in *)addr;
>> +
>> + socks5msg.atyp = SOCKS5_ATYP_IPv4;
>> + memcpy(socks5msg.dst, &a->sin_addr, sizeof(a->sin_addr));
>> + len = sizeof(a->sin_addr);
>
> here
>
>> + memcpy(socks5msg.dst + len, &a->sin_port, sizeof(a->sin_port));
>> + len += sizeof(a->sin_port);
>> + }
>> + break;
>> + case AF_INET6: {
>> + struct sockaddr_in6 *a = (struct sockaddr_in6 *)addr;
>> +
>> + socks5msg.atyp = SOCKS5_ATYP_IPv6;
>> + memcpy(socks5msg.dst, &a->sin6_addr, sizeof(a->sin6_addr));
>> + len = sizeof(a->sin6_addr);
>
> and there.
>
>> + memcpy(socks5msg.dst + len, &a->sin6_port,
>> sizeof(a->sin6_port));
>> + len += sizeof(a->sin6_port);
>> + }
>> + break;
>> + default:
>> + errno = EINVAL;
>> + return -1;
>> + }
>> + len += 4;
>
> and not have this here, where people wonder what that is :)
OK
>
>> +static int socks5_recv_connect(int fd)
>> +{
>> + struct socks5_request socks5msg;
>> +
>> + if (recv(fd, &socks5msg, 4, 0) != 4) {
>
> Thinking about it (there are more like that above and below). You can
> theoretically have a short read here... So please at least leave a note
> that we actually should manage buffering of recv()s.
OK
>
>> +++ b/slirp/socks5.h
>> +struct socks4_data {
>> + char version;
>> + char type;
>> + unsigned short port;
>> + uint32_t address;
>> + char data[SOCKS_BUFF_SIZE]; /* to hold FQDN and username */
>> +} __attribute__((packed));
>> +
>> +/* SOCKS4 protocol responses */
>> +#define SOCKS4_VERSION 4
>> +#define SOCKS_CONNECT 1
>> +#define SOCKS_BIND 2
>> +#define SOCKS4_CONN_ACC 90
>> +#define SOCKS4_CONN_REF 91
>> +#define SOCKS4_CONN_IDENT 92
>> +#define SOCKS4_CONN_IDENTDIFF 93
>
> These are unused, better just drop them?
I will rewrite this to avoid copy from ncat.
>
>> @@ -394,11 +395,21 @@ tcp_sockclosed(struct tcpcb *tp)
>> int tcp_fconnect(struct socket *so, unsigned short af)
>> {
>> int ret=0;
>> + Slirp *slirp = so->slirp;
>>
>> DEBUG_CALL("tcp_fconnect");
>> DEBUG_ARG("so = %p", so);
>>
>> - ret = so->s = qemu_socket(af, SOCK_STREAM, 0);
>> + /* local traffic doesn't go to the proxy */
>
> Please fix the comment into "only pass local traffic through the proxy",
> to actually reflect what the following "if" does :)
>
>> + if (slirp->proxy_server &&
>> + !(af == AF_INET &&
>> + (so->so_faddr.s_addr & slirp->vnetwork_mask.s_addr) ==
>> + slirp->vnetwork_addr.s_addr)) {
>> + ret = so->s = socks5_socket(&so->so_proxy_state);
>
> Please do support AF_INET6 too here, otherwise when working with an ipv6
> host setup it will break.
OK
>
>> diff --git a/slirp/udp6.c b/slirp/udp6.c
>> index 9fa314b..995181d 100644
>> --- a/slirp/udp6.c
>> +++ b/slirp/udp6.c
>> @@ -22,7 +22,7 @@ void udp6_input(struct mbuf *m)
>> DEBUG_CALL("udp6_input");
>> DEBUG_ARG("m = %lx", (long)m);
>>
>> - if (slirp->restricted) {
>> + if (slirp->restricted || slirp->proxy_server) {
>> goto bad;
>
> Ditto: please do support ipv6 here. It shouldn't be hard, it should be a
> matter of using the same test as v4 :)
OK
Thanks,
Laurent
