Le 04/04/2017 à 12:05, Philippe Mathieu-Daudé a écrit :
> Hi Laurent,
Hi Philippe,
> I waited this feature for long and excited to try it soon :)
>
> Please find some comments inline.
>
> On 04/03/2017 08:56 PM, Laurent Vivier wrote:
>> When the VM is used behind a firewall, This allows
>> the use of a SOCKS5 proxy server to connect the VM IP stack
>> directly to the Internet.
>>
>> This implementation doesn't manage UDP packets, so they
>> are simply dropped (as with restrict=on), except for
>> the localhost as we need it for DNS.
>>
>> Signed-off-by: Laurent Vivier <laur...@vivier.eu>
>> ---
>> net/slirp.c | 39 ++++++-
>> qapi-schema.json | 9 ++
>> qemu-options.hx | 11 ++
>> slirp/Makefile.objs | 2 +-
>> slirp/ip_icmp.c | 2 +-
>> slirp/libslirp.h | 3 +
>> slirp/slirp.c | 68 ++++++++++-
>> slirp/slirp.h | 6 +
>> slirp/socket.h | 4 +
>> slirp/socks5.c | 328
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> slirp/socks5.h | 24 ++++
>> slirp/tcp_subr.c | 22 +++-
>> slirp/udp.c | 9 ++
>> slirp/udp6.c | 8 ++
>> 14 files changed, 524 insertions(+), 11 deletions(-)
>> create mode 100644 slirp/socks5.c
>> create mode 100644 slirp/socks5.h
>>
...
>> diff --git a/slirp/socks5.c b/slirp/socks5.c
>> new file mode 100644
>> index 0000000..813c3db
>> --- /dev/null
>> +++ b/slirp/socks5.c
>> @@ -0,0 +1,328 @@
>> +/* based on RFC 1928
>> + * SOCKS Protocol Version 5
>> + * based on RFC 1929
>> + * Username/Password Authentication for SOCKS V5
>> + * TODO:
>> + * - RFC 1961 GSS-API Authentication Method for SOCKS Version 5
>> + * - manage buffering on recv()
>> + * - IPv6 connection to proxy
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/sockets.h"
>> +
>> +#include "socks5.h"
>> +
>> +#define SOCKS_LEN_MAX 0xff
>
> I can't find 0xFF in the RFC1928, I prefer a self-explanatory UINT8_MAX
> but that's up to you (RFC1929 uses 255 for UNAME/PASSWD but not
> explicitly for FQDN).
I agree UINT8_MAX looks better.
...
>> +static int socks5_recv_connect(int fd)
>> +{
>> + uint8_t reply[7 + SOCKS_LEN_MAX]; /* can contains a FQDN */
>> +
>> + /*
>> + * reply[0] is protocol version: 5
>> + * reply[1] is reply field
>> + * reply[2] is reserved
>> + * reply[3] is address type */
>> +
>> + if (recv(fd, reply, 4, 0) != 4) {
>> + return -1;
>> + }
>> +
>> + if (reply[0] != SOCKS_VERSION_5) {
>> + errno = EINVAL;
>> + return -1;
>> + }
>> +
>> + if (reply[1] != SOCKS5_CMD_SUCCESS) {
>> + errno = EINVAL;
>
> Here the failure reason is lost! It should be notified to the user, can
> you add some function to display it?
Yes, I ignore it because I don't know what to do with result.
Perhaps I can add a log...
>
>> + return -1;
>> + }
>> +
>> + switch (reply[3]) {
>> + case SOCKS5_ATYPE_IPV4:
>> + if (recv(fd, reply + 4, 6, 0) != 6) {
>
> Can we avoid this magic using sizeof(struct in_addr) + sizeof(in_port_t)
> or a #define?
I have put the number directly here because in the RFC we have directly
the number, but I agree the sizeof() is a better solution.
>
>> + return -1;
>> + }
>> + break;
>> + case SOCKS5_ATYPE_IPV6:
>> + if (recv(fd, reply + 4, 18, 0) != 18) {
>
> same with sizeof(struct in6_addr) + sizeof(in_port_t)
OK
>> + return -1;
>> + }
>> + break;
>> + case SOCKS5_ATYPE_FQDN:
>> + if (recv(fd, reply + 4, 1, 0) != 1) {
>> + return -1;
>> + }
>> + if (recv(fd, reply + 5,
>> + reply[4], 0) != reply[4]) {
>
> Would be nice/useful to log the FQDN (but it needs to be sanitized in
> case of nasty invalid data). Let it be a /* TODO */ at least.
Yes, I can add a log.
...
>> +void socks5_recv(int fd, socks5_state_t *state)
>> +{
>> + int ret;
>> +
>> + switch (*state) {
>> + case SOCKS5_STATE_NEGOCIATING:
>> + switch (socks5_recv_negociate(fd)) {
>> + case SOCKS5_AUTH_METHOD_NONE: /* no authentification */
>> + *state = SOCKS5_STATE_ESTABLISH;
>> + break;
>> + case SOCKS5_AUTH_METHOD_PASSWORD: /* username/password */
>> + *state = SOCKS5_STATE_AUTHENTICATE;
>> + break;
>> + default:
>> + break;
>
> Reading the RFC1928 the server can reply SOCKS5_AUTH_METHOD_GSSAPI or
> SOCKS5_AUTH_METHOD_REJECTED which are valid. RFC states:
>
> Compliant implementations MUST support GSSAPI and SHOULD support
> USERNAME/PASSWORD authentication methods.
Yes, I know, GSSAPI is in the TODO of the file header ;)
For instance, neither ncat nor TOR implement it... so I think it can
wait someone really needs it.
>
> I wonder if this could lead to and infinite negociation and being
> paranoid an evil server could keep DDoS'ing this client :)
> Anyway I think this function has to handle this (eventually reporting
> some Unsupported/Invalid protocol issue) as state the RFC for
> SOCKS5_AUTH_METHOD_REJECTED:
>
> If the selected METHOD is X'FF', none of the methods listed by the
> client are acceptable, and the client MUST close the connection.
I agree error case is not correctly managed. I will fix.
>> + }
>> + break;
>> + case SOCKS5_STATE_AUTHENTICATING:
>> + ret = socks5_recv_password(fd);
>> + if (ret >= 0) {
>> + *state = SOCKS5_STATE_ESTABLISH;
>> + }
>> + break;
>> + case SOCKS5_STATE_ESTABLISHING:
>> + ret = socks5_recv_connect(fd);
>> + if (ret >= 0) {
>> + *state = SOCKS5_STATE_NONE;
>> + }
>> + break;
>> + default:
>> + break;
>
> I think only the case SOCKS5_STATE_NONE can break, all other states
> should be handled as error in protocol.
>
I agree.
Thanks,
Laurent
