All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel]  [PATCH] Slirp reverse UDP firewall
@ 2011-04-12 16:19 Daisuke Nojiri
  2011-04-12 16:38 ` Jan Kiszka
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Daisuke Nojiri @ 2011-04-12 16:19 UTC (permalink / raw)
  To: qemu-devel

[-- Attachment #1: Type: text/plain, Size: 10515 bytes --]

This patch adds: -drop-udp, -allow-udp ADDR:PORT, -drop-log FILE

  e.g.) $ qemu -net user -drop-log qemu.drop -drop-udp -allow-udp
10.0.2.3:53

-drop-udp enables usermode firewall for out-going UDP packats from a guest.
All UDP packets except ones allowed by -allow-udp will be dropped. Dropped
packets are logged in the file specified by FILE. PORT can be a single
number
(e.g. 53) or a range (e.g. [80-81]). If ADDR is ommitted, all addresses
match
the rule.

Signed-off-by: Daisuke Nojiri <dnojiri@google.com>

--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1119,6 +1119,24 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
     "vde|"
 #endif
     "socket],id=str[,option][,option][,...]\n", QEMU_ARCH_ALL)
+
+DEF("drop-udp", 0, QEMU_OPTION_drop_udp,
+"-drop-udp\tDrop UDP packets by usermode firewall\n",
+QEMU_ARCH_ALL)
+
+DEF("allow-udp", HAS_ARG, QEMU_OPTION_allow_udp,
+    "-allow-udp addr:port\n"
+    "                Add an allowed rule for -drop-udp. If destination
matches\n"
+    "                the rule, the packet won't be dropped. 'port' can be a
single\n"
+    "                number (e.g. 53) or a range (e.g. [80-81]. If 'addr'
is omitted,
+    "                all addresses match.\n",
+    QEMU_ARCH_ALL)
+
+DEF("drop-log", HAS_ARG, QEMU_OPTION_drop_log,
+    "-drop-log file\n"
+    "                Set usermode firewall log filename to 'file'.\n",
+    QEMU_ARCH_ALL)
+
 STEXI
 @item -net nic[,vlan=@var{n}][,macaddr=@var{mac}][,model=@var{type}]
[,name=@var{name}][,addr=@var{addr}][,vectors=@var{v}]
 @findex -net
diff --git a/slirp/libslirp.h b/slirp/libslirp.h
index 67c70e3..1ce5d68 100644
--- a/slirp/libslirp.h
+++ b/slirp/libslirp.h
@@ -44,6 +44,15 @@ void slirp_socket_recv(Slirp *slirp, struct in_addr
guest_addr,
 size_t slirp_socket_can_recv(Slirp *slirp, struct in_addr guest_addr,
                              int guest_port);

+/* Usermode firewall functions */
+void slirp_enable_drop_udp(void);
+void slirp_set_drop_log_fd(FILE *fd);
+void slirp_add_allow(const char *optarg, u_int8_t proto);
+int slirp_drop_log(const char *format, ...);
+int slirp_should_drop(unsigned long dst_addr,
+                      unsigned short dst_port,
+                      u_int8_t proto);
+
 #else /* !CONFIG_SLIRP */

 static inline void slirp_select_fill(int *pnfds, fd_set *readfds,
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 1593be1..d321316 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -1111,3 +1111,192 @@ static int slirp_state_load(QEMUFile *f, void
*opaque, int version_id)

     return 0;
 }
+
+/*
+ * Allow rule for the usermode firewall
+ */
+struct ufw_allowed {
+    struct ufw_allowed *next;
+    unsigned long dst_addr;
+    /* Port range. For a single port, dst_lport = dst_hport. */
+    unsigned short dst_lport;   /* in host byte order */
+    unsigned short dst_hport;   /* in host byte order */
+};
+
+/*
+ * Global variables for the usermode firewall
+ */
+static int drop_udp = 0;
+static FILE *drop_log_fd = NULL;
+static struct ufw_allowed *fw_allowed_udp = NULL;
+
+void slirp_enable_drop_udp(void)
+{
+    drop_udp = 1;
+}
+
+void slirp_set_drop_log_fd(FILE *fd)
+{
+    drop_log_fd = fd;
+}
+
+int slirp_should_drop(unsigned long dst_addr,
+                      unsigned short dst_port,
+                      u_int8_t proto) {
+    struct ufw_allowed *fwa = NULL;
+    unsigned short dport;   /* host byte order */
+
+    switch (proto) {
+    case IPPROTO_UDP:
+        if (drop_udp == 0) {
+            return 0;
+        } else {
+            fwa = fw_allowed_udp;
+        }
+        break;
+    case IPPROTO_TCP:
+    default:
+        return 1;   /* unrecognized protocol. default drop. */
+    }
+
+    /* Find matching allow rule. 0 works as a wildcard for address. */
+    for (; fwa; fwa = fwa->next) {
+        dport = ntohs(dst_port);
+        if ((fwa->dst_lport <= dport) && (dport <= fwa->dst_hport)) {
+            /* allow any destination if 0 */
+            if (fwa->dst_addr == 0 || fwa->dst_addr == dst_addr) {
+                return 0;
+            }
+        }
+    }
+    return 1;
+}
+
+/*
+ * Register an allow rule for user mode firewall
+ */
+void slirp_add_allow_internal(unsigned long dst_addr,
+                              unsigned short dst_lport,
+                              unsigned short dst_hport,
+                              u_int8_t proto) {
+    struct ufw_allowed *fwa;
+
+    fwa = (struct ufw_allowed *)malloc(sizeof(struct ufw_allowed));
+    if (!fwa) {
+        DEBUG_MISC((dfd, "Unabled to create a new firewall rule "
+                    "due to malloc failure\n"));
+        exit(-1);
+    }
+
+    fwa->dst_addr = dst_addr;
+    fwa->dst_lport = dst_lport;
+    fwa->dst_hport = dst_hport;
+
+    switch (proto) {
+    case IPPROTO_UDP:
+        fwa->next = fw_allowed_udp;
+        fw_allowed_udp = fwa;
+        break;
+    case IPPROTO_TCP:
+    default:
+        free(fwa);
+        return;   /* unrecognized protocol */
+    }
+}
+
+/*
+ * Parse a null-terminated string specifying a network port or port range
(e.g.
+ * "[1024-65535]"). In case of a single port, lport and hport are the same.
+ * Returns 0 on success and -1 on error.
+ */
+int parse_port_range(const char *str,
+                     unsigned short *lport,
+                     unsigned short *hport) {
+    unsigned int low = 0, high = 0;
+    char *p, *arg = strdup(str);
+
+    p = rindex(arg, ']');
+    if ((*arg == '[') & (p != NULL)) {
+        p = arg + 1;   /* skip '[' */
+        low  = atoi(strsep(&p, "-"));
+        high = atoi(p);
+    } else {
+        low = atoi(arg);
+        high = low;
+    }
+
+    free(arg);
+
+    if ((low > 0) && (high > 0) && (low <= high) && (high < 65536)) {
+        *lport = low;
+        *hport = high;
+        return 0;
+    }
+
+    return -1;
+}
+
+/*
+ * Add allow rules for the usermode firewall.
+ */
+void slirp_add_allow(const char *optarg, u_int8_t proto)
+{
+    /*
+     * we expect the following format:
+     *  dst_addr:dst_port OR dst_addr:[dst_lport-dst_hport]
+     */
+    char *argument = strdup(optarg), *p = argument;
+    char *dst_addr_str, *dst_port_str;
+    struct in_addr dst_addr;
+    unsigned short dst_lport, dst_hport;
+
+    dst_addr_str = strsep(&p, ":");
+    dst_port_str = p;
+
+    if (dst_addr_str == NULL || dst_port_str == NULL) {
+        fprintf(stderr,
+                "Invalid argument %s for -allow. We expect "
+                "dst_addr:dst_port or dst_addr:[dst_lport-dst_hport]\n",
+                optarg);
+        exit(1);
+    }
+
+    /* handling ":port" notation (when IP address is omitted entirely). */
+    if (*dst_addr_str == '\0') {
+        dst_addr.s_addr = 0;
+    }
+    /* inet_aton returns 0 on failure. */
+    else if (inet_aton(dst_addr_str, &dst_addr) == 0) {
+        fprintf(stderr, "Invalid destination IP address: %s\n",
dst_addr_str);
+        exit(1);
+    }
+
+    if (parse_port_range(dst_port_str, &dst_lport, &dst_hport) == -1) {
+        fprintf(stderr, "Invalid destination port or port range\n");
+        exit(1);
+    }
+
+    slirp_add_allow_internal(dst_addr.s_addr, dst_lport, dst_hport, proto);
+
+    free(argument);
+}
+
+/*
+ * Write to drop-log
+ */
+int slirp_drop_log(const char *format, ...)
+{
+    va_list args;
+
+    if (!drop_log_fd) {
+        return 0;
 +    }
+
+    va_start(args, format);
+    vfprintf(drop_log_fd, format, args);
+    va_end(args);
+
+    fflush(drop_log_fd);
+
+    return 1;
+}
diff --git a/slirp/slirp.h b/slirp/slirp.h
index 954289a..29ee425 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -299,6 +299,15 @@ int tcp_emu(struct socket *, struct mbuf *);
 int tcp_ctl(struct socket *);
 struct tcpcb *tcp_drop(struct tcpcb *tp, int err);

+/* slirp.c */
+void slirp_add_allow_internal(unsigned long dst_addr,
+                              unsigned short dst_lport,
+                              unsigned short dst_hport,
+                              u_int8_t proto);
+int parse_port_range(const char *str,
+                     unsigned short *lport,
+                     unsigned short *hport);
+
 #ifdef USE_PPP
 #define MIN_MRU MINMRU
 #define MAX_MRU MAXMRU
diff --git a/slirp/udp.c b/slirp/udp.c
index 02b3793..db7e4ca 100644
--- a/slirp/udp.c
+++ b/slirp/udp.c
@@ -67,6 +67,8 @@ udp_input(register struct mbuf *m, int iphlen)
  DEBUG_ARG("m = %lx", (long)m);
  DEBUG_ARG("iphlen = %d", iphlen);

+        time_t timestamp = time(NULL);
+
  /*
  * Strip IP options, if any; should skip this,
  * make available to user, and use on returned packets,
@@ -98,6 +100,28 @@ udp_input(register struct mbuf *m, int iphlen)
  ip->ip_len = len;
  }

+        /*
+         * User mode firewall
+         */
+        if (slirp_should_drop(ip->ip_dst.s_addr, uh->uh_dport,
IPPROTO_UDP)) {
+            slirp_drop_log(
+                "Dropped UDP: src:0x%08x:0x%04hx dst:0x%08x:0x%04hx %ld\n",
+                ntohl(ip->ip_src.s_addr),
+                ntohs(uh->uh_sport),
+                ntohl(ip->ip_dst.s_addr),
+                ntohs(uh->uh_dport),
+                timestamp);
+            goto bad;
+        } else {
+            slirp_drop_log(
+                "Allowed UDP: src:0x%08x:0x%04hx dst:0x%08x:0x%04hx %ld\n",
+                ntohl(ip->ip_src.s_addr),
+                ntohs(uh->uh_sport),
+                ntohl(ip->ip_dst.s_addr),
+                ntohs(uh->uh_dport),
+                timestamp);
+        }
+
  /*
  * Save a copy of the IP header in case we want restore it
  * for sending an ICMP error message in response.
diff --git a/vl.c b/vl.c
index 68c3b53..b0583e3 100644
--- a/vl.c
+++ b/vl.c
@@ -2287,6 +2287,24 @@ int main(int argc, char **argv, char **envp)
                     exit(1);
                 break;
 #endif
+            case QEMU_OPTION_drop_udp:
+                slirp_enable_drop_udp();
+                break;
+            case QEMU_OPTION_allow_udp:
+                slirp_add_allow(optarg, IPPROTO_UDP);
+                break;
+            case QEMU_OPTION_drop_log:
+                {
+                    FILE *drop_log_fd;
+                    drop_log_fd = fopen(optarg, "w");
+
+                    if (!drop_log_fd) {
+                        fprintf(stderr, "Cannot open drop log: %s\n",
optarg);
+                        exit(1);
+                    }
+                    slirp_set_drop_log_fd(drop_log_fd);
+                }
+                break;
             case QEMU_OPTION_bt:
                 add_device_config(DEV_BT, optarg);
                 break;

[-- Attachment #2: Type: text/html, Size: 15422 bytes --]

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] Slirp reverse UDP firewall
  2011-04-12 16:19 [Qemu-devel] [PATCH] Slirp reverse UDP firewall Daisuke Nojiri
@ 2011-04-12 16:38 ` Jan Kiszka
  2011-04-13 15:35   ` Daisuke Nojiri
  2011-04-14 13:48   ` Avi Kivity
  2011-04-13 20:52 ` Blue Swirl
  2011-04-14 19:33 ` Stefan Berger
  2 siblings, 2 replies; 11+ messages in thread
From: Jan Kiszka @ 2011-04-12 16:38 UTC (permalink / raw)
  To: Daisuke Nojiri; +Cc: qemu-devel

On 2011-04-12 18:19, Daisuke Nojiri wrote:
> This patch adds: -drop-udp, -allow-udp ADDR:PORT, -drop-log FILE
> 
>   e.g.) $ qemu -net user -drop-log qemu.drop -drop-udp -allow-udp
> 10.0.2.3:53

No more stand-alone slirp arguments please. That syntax breaks when
instantiating >1 back-ends.

> 
> -drop-udp enables usermode firewall for out-going UDP packats from a guest.
> All UDP packets except ones allowed by -allow-udp will be dropped. Dropped
> packets are logged in the file specified by FILE. PORT can be a single
> number
> (e.g. 53) or a range (e.g. [80-81]). If ADDR is ommitted, all addresses
> match
> the rule.

Will we see a TCP firewall as well? Can we prepare for a more generic
infrastructure, or what makes UDP special?

Also, please break up in smaller bits (example: logging would be a
separate topic). And make sure that your patches aren't line-wrapped.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] Slirp reverse UDP firewall
  2011-04-12 16:38 ` Jan Kiszka
@ 2011-04-13 15:35   ` Daisuke Nojiri
  2011-04-14 13:48   ` Avi Kivity
  1 sibling, 0 replies; 11+ messages in thread
From: Daisuke Nojiri @ 2011-04-13 15:35 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: qemu-devel

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

Thanks, Jan. I split my patch into three and started a new thread. I also
put all options in -net user. Yes, TCP firewall is coming. You'll see some
of the added functions will be shared.

Dai

On Tue, Apr 12, 2011 at 9:38 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 2011-04-12 18:19, Daisuke Nojiri wrote:
> > This patch adds: -drop-udp, -allow-udp ADDR:PORT, -drop-log FILE
> >
> >   e.g.) $ qemu -net user -drop-log qemu.drop -drop-udp -allow-udp
> > 10.0.2.3:53
>
> No more stand-alone slirp arguments please. That syntax breaks when
> instantiating >1 back-ends.
>
> >
> > -drop-udp enables usermode firewall for out-going UDP packats from a
> guest.
> > All UDP packets except ones allowed by -allow-udp will be dropped.
> Dropped
> > packets are logged in the file specified by FILE. PORT can be a single
> > number
> > (e.g. 53) or a range (e.g. [80-81]). If ADDR is ommitted, all addresses
> > match
> > the rule.
>
> Will we see a TCP firewall as well? Can we prepare for a more generic
> infrastructure, or what makes UDP special?
>
> Also, please break up in smaller bits (example: logging would be a
> separate topic). And make sure that your patches aren't line-wrapped.
>
> Thanks,
> Jan
>
> --
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
>

[-- Attachment #2: Type: text/html, Size: 1836 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] Slirp reverse UDP firewall
  2011-04-12 16:19 [Qemu-devel] [PATCH] Slirp reverse UDP firewall Daisuke Nojiri
  2011-04-12 16:38 ` Jan Kiszka
@ 2011-04-13 20:52 ` Blue Swirl
  2011-04-14 19:14   ` Daisuke Nojiri
  2011-04-14 19:33 ` Stefan Berger
  2 siblings, 1 reply; 11+ messages in thread
From: Blue Swirl @ 2011-04-13 20:52 UTC (permalink / raw)
  To: Daisuke Nojiri; +Cc: qemu-devel

On Tue, Apr 12, 2011 at 7:19 PM, Daisuke Nojiri <dnojiri@google.com> wrote:
> This patch adds: -drop-udp, -allow-udp ADDR:PORT, -drop-log FILE
>   e.g.) $ qemu -net user -drop-log qemu.drop -drop-udp -allow-udp
> 10.0.2.3:53
> -drop-udp enables usermode firewall for out-going UDP packats from a guest.
> All UDP packets except ones allowed by -allow-udp will be dropped. Dropped
> packets are logged in the file specified by FILE. PORT can be a single
> number
> (e.g. 53) or a range (e.g. [80-81]). If ADDR is ommitted, all addresses
> match
> the rule.
> Signed-off-by: Daisuke Nojiri <dnojiri@google.com>

I missed somehow 1/3, so I'll comment to this one.

> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1119,6 +1119,24 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
>      "vde|"
>  #endif
>      "socket],id=str[,option][,option][,...]\n", QEMU_ARCH_ALL)
> +
> +DEF("drop-udp", 0, QEMU_OPTION_drop_udp,

With the TCP firewall in mind, I'd use a more general syntax,
something like "deny=proto:udp". More complete rules would need
denying a block inside allowed block, so address part should be used
here (or "all" for deny all).

> +"-drop-udp\tDrop UDP packets by usermode firewall\n",
> +QEMU_ARCH_ALL)
> +
> +DEF("allow-udp", HAS_ARG, QEMU_OPTION_allow_udp,

Likewise, "allow=proto:udp:addr:port". Then, if "udp" is left out, the
rule should apply to all protocols. The address part should allow for
range specifiers, like "10.0.0.0/24". Colon is not so great choice
when thinking also of IPv6 addresses.

> +    "-allow-udp addr:port\n"
> +    "                Add an allowed rule for -drop-udp. If destination
> matches\n"
> +    "                the rule, the packet won't be dropped. 'port' can be a
> single\n"
> +    "                number (e.g. 53) or a range (e.g. [80-81]. If 'addr'
> is omitted,
> +    "                all addresses match.\n",
> +    QEMU_ARCH_ALL)
> +
> +DEF("drop-log", HAS_ARG, QEMU_OPTION_drop_log,
> +    "-drop-log file\n"
> +    "                Set usermode firewall log filename to 'file'.\n",
> +    QEMU_ARCH_ALL)
> +
>  STEXI
>  @item -net nic[,vlan=@var{n}][,macaddr=@var{mac}][,model=@var{type}]
> [,name=@var{name}][,addr=@var{addr}][,vectors=@var{v}]
>  @findex -net
> diff --git a/slirp/libslirp.h b/slirp/libslirp.h
> index 67c70e3..1ce5d68 100644
> --- a/slirp/libslirp.h
> +++ b/slirp/libslirp.h
> @@ -44,6 +44,15 @@ void slirp_socket_recv(Slirp *slirp, struct in_addr
> guest_addr,
>  size_t slirp_socket_can_recv(Slirp *slirp, struct in_addr guest_addr,
>                               int guest_port);
>
> +/* Usermode firewall functions */
> +void slirp_enable_drop_udp(void);
> +void slirp_set_drop_log_fd(FILE *fd);
> +void slirp_add_allow(const char *optarg, u_int8_t proto);
> +int slirp_drop_log(const char *format, ...);
> +int slirp_should_drop(unsigned long dst_addr,
> +                      unsigned short dst_port,
> +                      u_int8_t proto);
> +
>  #else /* !CONFIG_SLIRP */
>
>  static inline void slirp_select_fill(int *pnfds, fd_set *readfds,
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 1593be1..d321316 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -1111,3 +1111,192 @@ static int slirp_state_load(QEMUFile *f, void
> *opaque, int version_id)
>
>      return 0;
>  }
> +
> +/*
> + * Allow rule for the usermode firewall
> + */
> +struct ufw_allowed {
> +    struct ufw_allowed *next;

Please use QLIST macros.

> +    unsigned long dst_addr;

This is not IPv6 safe, though Slirp does not implement IPv6. I'd still
use the proper address type.

> +    /* Port range. For a single port, dst_lport = dst_hport. */
> +    unsigned short dst_lport;   /* in host byte order */
> +    unsigned short dst_hport;   /* in host byte order */
> +};
> +
> +/*
> + * Global variables for the usermode firewall
> + */
> +static int drop_udp = 0;
> +static FILE *drop_log_fd = NULL;
> +static struct ufw_allowed *fw_allowed_udp = NULL;

Don't use global variables. It's possible to have several interfaces,
each on a different user mode stack, so each interface may have
different rules. There's struct Slirp defined in slirp.h, please put
these there.

> +
> +void slirp_enable_drop_udp(void)
> +{
> +    drop_udp = 1;
> +}
> +
> +void slirp_set_drop_log_fd(FILE *fd)
> +{
> +    drop_log_fd = fd;
> +}
> +
> +int slirp_should_drop(unsigned long dst_addr,
> +                      unsigned short dst_port,
> +                      u_int8_t proto) {
> +    struct ufw_allowed *fwa = NULL;
> +    unsigned short dport;   /* host byte order */
> +
> +    switch (proto) {
> +    case IPPROTO_UDP:
> +        if (drop_udp == 0) {
> +            return 0;
> +        } else {
> +            fwa = fw_allowed_udp;
> +        }
> +        break;
> +    case IPPROTO_TCP:
> +    default:
> +        return 1;   /* unrecognized protocol. default drop. */
> +    }
> +
> +    /* Find matching allow rule. 0 works as a wildcard for address. */
> +    for (; fwa; fwa = fwa->next) {
> +        dport = ntohs(dst_port);
> +        if ((fwa->dst_lport <= dport) && (dport <= fwa->dst_hport)) {
> +            /* allow any destination if 0 */
> +            if (fwa->dst_addr == 0 || fwa->dst_addr == dst_addr) {
> +                return 0;
> +            }
> +        }
> +    }
> +    return 1;
> +}
> +
> +/*
> + * Register an allow rule for user mode firewall
> + */
> +void slirp_add_allow_internal(unsigned long dst_addr,

static? I'm not sure which functions are really needed outside of this file.

> +                              unsigned short dst_lport,
> +                              unsigned short dst_hport,
> +                              u_int8_t proto) {
> +    struct ufw_allowed *fwa;
> +
> +    fwa = (struct ufw_allowed *)malloc(sizeof(struct ufw_allowed));

The cast is useless in C. Please use qemu_malloc, then the following
check is not needed.

> +    if (!fwa) {
> +        DEBUG_MISC((dfd, "Unabled to create a new firewall rule "
> +                    "due to malloc failure\n"));
> +        exit(-1);
> +    }
> +
> +    fwa->dst_addr = dst_addr;
> +    fwa->dst_lport = dst_lport;
> +    fwa->dst_hport = dst_hport;
> +
> +    switch (proto) {
> +    case IPPROTO_UDP:
> +        fwa->next = fw_allowed_udp;
> +        fw_allowed_udp = fwa;
> +        break;
> +    case IPPROTO_TCP:
> +    default:
> +        free(fwa);

This should then become qemu_free().

> +        return;   /* unrecognized protocol */
> +    }
> +}
> +
> +/*
> + * Parse a null-terminated string specifying a network port or port range
> (e.g.
> + * "[1024-65535]"). In case of a single port, lport and hport are the same.
> + * Returns 0 on success and -1 on error.
> + */
> +int parse_port_range(const char *str,
> +                     unsigned short *lport,
> +                     unsigned short *hport) {
> +    unsigned int low = 0, high = 0;
> +    char *p, *arg = strdup(str);

qemu_strdup(), but I'd avoid that by using strtol().

> +
> +    p = rindex(arg, ']');
> +    if ((*arg == '[') & (p != NULL)) {
> +        p = arg + 1;   /* skip '[' */
> +        low  = atoi(strsep(&p, "-"));
> +        high = atoi(p);
> +    } else {
> +        low = atoi(arg);
> +        high = low;
> +    }
> +
> +    free(arg);
> +
> +    if ((low > 0) && (high > 0) && (low <= high) && (high < 65536)) {
> +        *lport = low;
> +        *hport = high;
> +        return 0;
> +    }
> +
> +    return -1;
> +}
> +
> +/*
> + * Add allow rules for the usermode firewall.
> + */
> +void slirp_add_allow(const char *optarg, u_int8_t proto)
> +{
> +    /*
> +     * we expect the following format:
> +     *  dst_addr:dst_port OR dst_addr:[dst_lport-dst_hport]
> +     */
> +    char *argument = strdup(optarg), *p = argument;
> +    char *dst_addr_str, *dst_port_str;
> +    struct in_addr dst_addr;
> +    unsigned short dst_lport, dst_hport;
> +
> +    dst_addr_str = strsep(&p, ":");
> +    dst_port_str = p;
> +
> +    if (dst_addr_str == NULL || dst_port_str == NULL) {
> +        fprintf(stderr,
> +                "Invalid argument %s for -allow. We expect "
> +                "dst_addr:dst_port or dst_addr:[dst_lport-dst_hport]\n",
> +                optarg);
> +        exit(1);
> +    }
> +
> +    /* handling ":port" notation (when IP address is omitted entirely). */
> +    if (*dst_addr_str == '\0') {
> +        dst_addr.s_addr = 0;
> +    }
> +    /* inet_aton returns 0 on failure. */
> +    else if (inet_aton(dst_addr_str, &dst_addr) == 0) {

else belongs to the line with }.

> +        fprintf(stderr, "Invalid destination IP address: %s\n",
> dst_addr_str);
> +        exit(1);
> +    }
> +
> +    if (parse_port_range(dst_port_str, &dst_lport, &dst_hport) == -1) {
> +        fprintf(stderr, "Invalid destination port or port range\n");
> +        exit(1);
> +    }
> +
> +    slirp_add_allow_internal(dst_addr.s_addr, dst_lport, dst_hport, proto);
> +
> +    free(argument);
> +}
> +
> +/*
> + * Write to drop-log
> + */
> +int slirp_drop_log(const char *format, ...)
> +{
> +    va_list args;
> +
> +    if (!drop_log_fd) {
> +        return 0;
> +    }
> +
> +    va_start(args, format);
> +    vfprintf(drop_log_fd, format, args);
> +    va_end(args);
> +
> +    fflush(drop_log_fd);
> +
> +    return 1;
> +}
> diff --git a/slirp/slirp.h b/slirp/slirp.h
> index 954289a..29ee425 100644
> --- a/slirp/slirp.h
> +++ b/slirp/slirp.h
> @@ -299,6 +299,15 @@ int tcp_emu(struct socket *, struct mbuf *);
>  int tcp_ctl(struct socket *);
>  struct tcpcb *tcp_drop(struct tcpcb *tp, int err);
>
> +/* slirp.c */
> +void slirp_add_allow_internal(unsigned long dst_addr,
> +                              unsigned short dst_lport,
> +                              unsigned short dst_hport,
> +                              u_int8_t proto);
> +int parse_port_range(const char *str,
> +                     unsigned short *lport,
> +                     unsigned short *hport);
> +
>  #ifdef USE_PPP
>  #define MIN_MRU MINMRU
>  #define MAX_MRU MAXMRU
> diff --git a/slirp/udp.c b/slirp/udp.c
> index 02b3793..db7e4ca 100644
> --- a/slirp/udp.c
> +++ b/slirp/udp.c
> @@ -67,6 +67,8 @@ udp_input(register struct mbuf *m, int iphlen)
>   DEBUG_ARG("m = %lx", (long)m);
>   DEBUG_ARG("iphlen = %d", iphlen);
>
> +        time_t timestamp = time(NULL);
> +
>   /*
>   * Strip IP options, if any; should skip this,
>   * make available to user, and use on returned packets,
> @@ -98,6 +100,28 @@ udp_input(register struct mbuf *m, int iphlen)
>   ip->ip_len = len;
>   }
>
> +        /*
> +         * User mode firewall
> +         */
> +        if (slirp_should_drop(ip->ip_dst.s_addr, uh->uh_dport,
> IPPROTO_UDP)) {
> +            slirp_drop_log(
> +                "Dropped UDP: src:0x%08x:0x%04hx dst:0x%08x:0x%04hx %ld\n",
> +                ntohl(ip->ip_src.s_addr),
> +                ntohs(uh->uh_sport),
> +                ntohl(ip->ip_dst.s_addr),
> +                ntohs(uh->uh_dport),
> +                timestamp);

Maybe tracepoints could be used instead of logging, though a separate
log may be useful too.

> +            goto bad;
> +        } else {
> +            slirp_drop_log(
> +                "Allowed UDP: src:0x%08x:0x%04hx dst:0x%08x:0x%04hx %ld\n",
> +                ntohl(ip->ip_src.s_addr),
> +                ntohs(uh->uh_sport),
> +                ntohl(ip->ip_dst.s_addr),
> +                ntohs(uh->uh_dport),
> +                timestamp);
> +        }
> +
>   /*
>   * Save a copy of the IP header in case we want restore it
>   * for sending an ICMP error message in response.
> diff --git a/vl.c b/vl.c
> index 68c3b53..b0583e3 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2287,6 +2287,24 @@ int main(int argc, char **argv, char **envp)
>                      exit(1);
>                  break;
>  #endif
> +            case QEMU_OPTION_drop_udp:
> +                slirp_enable_drop_udp();
> +                break;
> +            case QEMU_OPTION_allow_udp:
> +                slirp_add_allow(optarg, IPPROTO_UDP);
> +                break;
> +            case QEMU_OPTION_drop_log:
> +                {
> +                    FILE *drop_log_fd;
> +                    drop_log_fd = fopen(optarg, "w");
> +
> +                    if (!drop_log_fd) {
> +                        fprintf(stderr, "Cannot open drop log: %s\n",
> optarg);
> +                        exit(1);
> +                    }
> +                    slirp_set_drop_log_fd(drop_log_fd);
> +                }
> +                break;
>              case QEMU_OPTION_bt:
>                  add_device_config(DEV_BT, optarg);
>                  break;
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] Slirp reverse UDP firewall
  2011-04-12 16:38 ` Jan Kiszka
  2011-04-13 15:35   ` Daisuke Nojiri
@ 2011-04-14 13:48   ` Avi Kivity
  2011-04-14 14:08     ` Avi Kivity
  1 sibling, 1 reply; 11+ messages in thread
From: Avi Kivity @ 2011-04-14 13:48 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: Daisuke Nojiri, qemu-devel

On 04/12/2011 07:38 PM, Jan Kiszka wrote:
> On 2011-04-12 18:19, Daisuke Nojiri wrote:
> >  This patch adds: -drop-udp, -allow-udp ADDR:PORT, -drop-log FILE
> >
> >    e.g.) $ qemu -net user -drop-log qemu.drop -drop-udp -allow-udp
> >  10.0.2.3:53
>
> No more stand-alone slirp arguments please. That syntax breaks when
> instantiating>1 back-ends.
>
> >
> >  -drop-udp enables usermode firewall for out-going UDP packats from a guest.
> >  All UDP packets except ones allowed by -allow-udp will be dropped. Dropped
> >  packets are logged in the file specified by FILE. PORT can be a single
> >  number
> >  (e.g. 53) or a range (e.g. [80-81]). If ADDR is ommitted, all addresses
> >  match
> >  the rule.
>
> Will we see a TCP firewall as well? Can we prepare for a more generic
> infrastructure, or what makes UDP special?

If some generic firewall like BPF is available as a user library, 
perhaps we can integrate one instead of writing a new one from scratch.

-- 
error compiling committee.c: too many arguments to function

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] Slirp reverse UDP firewall
  2011-04-14 13:48   ` Avi Kivity
@ 2011-04-14 14:08     ` Avi Kivity
  2011-04-14 20:04       ` Daisuke Nojiri
  0 siblings, 1 reply; 11+ messages in thread
From: Avi Kivity @ 2011-04-14 14:08 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: Daisuke Nojiri, qemu-devel

On 04/14/2011 04:48 PM, Avi Kivity wrote:
>> Will we see a TCP firewall as well? Can we prepare for a more generic
>> infrastructure, or what makes UDP special?
>
>
> If some generic firewall like BPF is available as a user library, 
> perhaps we can integrate one instead of writing a new one from scratch.
>

Heck, you could even write a tcg backend for bpf instructions and run 
the jit the firewall filter set.

-- 
error compiling committee.c: too many arguments to function

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] Slirp reverse UDP firewall
  2011-04-13 20:52 ` Blue Swirl
@ 2011-04-14 19:14   ` Daisuke Nojiri
  0 siblings, 0 replies; 11+ messages in thread
From: Daisuke Nojiri @ 2011-04-14 19:14 UTC (permalink / raw)
  To: Blue Swirl; +Cc: qemu-devel

[-- Attachment #1: Type: text/plain, Size: 22144 bytes --]

Hi, Blue,

> I missed somehow 1/3, so I'll comment to this one.

I updated 1/3 and pasted it below. The rest of the points you raised will be
addressed in 2/3 and 3/3.

> With the TCP firewall in mind, I'd use a more general syntax,
> something like "deny=proto:udp". More complete rules would need
> denying a block inside allowed block, so address part should be used
> here (or "all" for deny all).

How about drop=udp|tcp|all? drop switch makes all packets denied and allow
rules will work as exceptions.
This way, we can avoid conflicts between denys and allows.

> Don't use global variables. It's possible to have several interfaces,
> each on a different user mode stack, so each interface may have
> different rules. There's struct Slirp defined in slirp.h, please put
> these there.

Addressed as suggested. Thanks, Blue.

Dai

----------- Subject: [Qemu-devel] [Patch 1/3] Slirp Reverse UDP Firewall
------------
This patch series adds a simple reverse UDP firewall functionality to Slirp.
The series consists of three patches. Each adds one -net user option:

    1. drop=udp|all - enables the firewall
    2. droplog=FILE - sets the drop log filename
    3. allow=PROTO:ADDR:PORT - adds an allow rule

  e.g.) $ qemu -net user,drop=udp,droplog=qemu.drop,allow=udp:10.0.2.3:53

All UDP packets except ones allowed by allow rules will be dropped. The
source
and the destination of the dropped packets are logged in the file specified
by FILE.
PORT can be a single number (e.g. 53) or a range (e.g. [80-81]). ADDR can be
a
single address (e.g. 1.2.3.4) or a range (e.g. 1.2.3.4/24). If ADDR
is ommitted,
all addresses match the rule. If PROTO is omitted, all protocols match the
rule.

TCP support will follow in another patch series.

Signed-off-by: Daisuke Nojiri <dnojiri@google.com>

diff --git a/net.c b/net.c
index 8d6a555..2742741 100644
--- a/net.c
+++ b/net.c
@@ -925,6 +925,10 @@ static const struct {
                 .name = "guestfwd",
                 .type = QEMU_OPT_STRING,
                 .help = "IP address and port to forward guest TCP
connections",
+            }, {
+                .name = "drop",
+                .type = QEMU_OPT_STRING,
+                .help = "Enable the simple reverse firewall",
             },
             { /* end of list */ }
         },
diff --git a/net/slirp.c b/net/slirp.c
index b41c60a..bd83700 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -141,7 +141,7 @@ static int net_slirp_init(VLANState *vlan, const char
*model,
                           const char *vhostname, const char *tftp_export,
                           const char *bootfile, const char *vdhcp_start,
                           const char *vnameserver, const char *smb_export,
-                          const char *vsmbserver)
+                          const char *vsmbserver, unsigned char drop)
 {
     /* default settings according to historic slirp */
     struct in_addr net  = { .s_addr = htonl(0x0a000200) }; /* 10.0.2.0 */
@@ -246,7 +246,7 @@ static int net_slirp_init(VLANState *vlan, const char
*model,
     s = DO_UPCAST(SlirpState, nc, nc);

     s->slirp = slirp_init(restricted, net, mask, host, vhostname,
-                          tftp_export, bootfile, dhcp, dns, s);
+                          tftp_export, bootfile, dhcp, dns, drop, s);
     QTAILQ_INSERT_TAIL(&slirp_stacks, s, entry);

     for (config = slirp_configs; config; config = config->next) {
@@ -693,6 +693,7 @@ int net_init_slirp(QemuOpts *opts,
     char *vnet = NULL;
     int restricted = 0;
     int ret;
+    unsigned char drop = 0;

     vhost       = qemu_opt_get(opts, "host");
     vhostname   = qemu_opt_get(opts, "hostname");
@@ -726,11 +727,25 @@ int net_init_slirp(QemuOpts *opts,
         restricted = 1;
     }

+    if (qemu_opt_get(opts, "drop")) {
+        switch (qemu_opt_get(opts, "drop")[0]) {
+        case 'u':
+            drop &= SLIRP_DROP_UDP;
+            break;
+        case 'a':
+            drop &= SLIRP_DROP_UDP;
+            break;
+        default:
+            error_report("Unknown protocol name given to drop option");
+            return -1;
+        }
+    }
+
     qemu_opt_foreach(opts, net_init_slirp_configs, NULL, 0);

     ret = net_slirp_init(vlan, "user", name, restricted, vnet, vhost,
                          vhostname, tftp_export, bootfile, vdhcp_start,
-                         vnamesrv, smb_export, vsmbsrv);
+                         vnamesrv, smb_export, vsmbsrv, drop);

     while (slirp_configs) {
         config = slirp_configs;
@@ -768,4 +783,3 @@ int net_slirp_parse_legacy(QemuOptsList *opts_list,
const char *optarg, int *ret

     return 1;
 }
-
diff --git a/qemu-options.hx b/qemu-options.hx
index ef60730..ef3e726 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1067,7 +1067,7 @@ DEF("net", HAS_ARG, QEMU_OPTION_net,
 #ifdef CONFIG_SLIRP
     "-net
user[,vlan=n][,name=str][,net=addr[/mask]][,host=addr][,restrict=y|n]\n"
     "
[,hostname=host][,dhcpstart=addr][,dns=addr][,tftp=dir][,bootfile=f]\n"
-    "         [,hostfwd=rule][,guestfwd=rule]"
+    "         [,hostfwd=rule][,guestfwd=rule][,drop=udp|all]"
 #ifndef _WIN32

"[,smb=dir[,smbserver=addr]]\n"
 #endif
diff --git a/slirp/libslirp.h b/slirp/libslirp.h
index 67c70e3..5778bf4 100644
--- a/slirp/libslirp.h
+++ b/slirp/libslirp.h
@@ -14,7 +14,8 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,
                   struct in_addr vnetmask, struct in_addr vhost,
                   const char *vhostname, const char *tftp_path,
                   const char *bootfile, struct in_addr vdhcp_start,
-                  struct in_addr vnameserver, void *opaque);
+                  struct in_addr vnameserver, unsigned char drop,
+                  void *opaque);
 void slirp_cleanup(Slirp *slirp);

 void slirp_select_fill(int *pnfds,
@@ -44,6 +45,14 @@ void slirp_socket_recv(Slirp *slirp, struct in_addr
guest_addr,
 size_t slirp_socket_can_recv(Slirp *slirp, struct in_addr guest_addr,
                              int guest_port);

+/* Reverse Firewall */
+#define SLIRP_DROP_UDP    1
+
+int slirp_should_drop(Slirp *slirp,
+                      struct in_addr dst_addr,
+                      unsigned short dst_port,
+                      u_int8_t proto);
+
 #else /* !CONFIG_SLIRP */

 static inline void slirp_select_fill(int *pnfds, fd_set *readfds,
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 1593be1..298ccb4 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -200,7 +200,7 @@ Slirp *slirp_init(int restricted, struct in_addr
vnetwork,
                   struct in_addr vnetmask, struct in_addr vhost,
                   const char *vhostname, const char *tftp_path,
                   const char *bootfile, struct in_addr vdhcp_start,
-                  struct in_addr vnameserver, void *opaque)
+                  struct in_addr vnameserver, unsigned char drop, void
*opaque)
 {
     Slirp *slirp = qemu_mallocz(sizeof(Slirp));

@@ -230,6 +230,8 @@ Slirp *slirp_init(int restricted, struct in_addr
vnetwork,
     slirp->vdhcp_startaddr = vdhcp_start;
     slirp->vnameserver_addr = vnameserver;

+    slirp->drop = drop;
+
     slirp->opaque = opaque;

     register_savevm(NULL, "slirp", 0, 3,
@@ -1111,3 +1113,20 @@ static int slirp_state_load(QEMUFile *f, void
*opaque, int version_id)

     return 0;
 }
+
+int slirp_should_drop(Slirp *slirp,
+                      struct in_addr dst_addr,
+                      unsigned short dst_port,
+                      u_int8_t proto) {
+    switch (proto) {
+    case IPPROTO_UDP:
+        if (!(slirp->drop & SLIRP_DROP_UDP)) {
+            return 0;
+        }
+        break;
+    default:
+        return 0;   /* unrecognized protocol. default pass. */
+    }
+
+    return 1;
+}
diff --git a/slirp/slirp.h b/slirp/slirp.h
index 954289a..bfea30d 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -180,6 +180,9 @@ struct Slirp {
     struct in_addr vdhcp_startaddr;
     struct in_addr vnameserver_addr;

+    /* Reverse Firewall configuration */
+    unsigned char drop;
+
     /* ARP cache for the guest IP addresses (XXX: allow many entries) */
     uint8_t client_ethaddr[6];

diff --git a/slirp/udp.c b/slirp/udp.c
index 02b3793..95c4af0 100644
--- a/slirp/udp.c
+++ b/slirp/udp.c
@@ -98,6 +98,17 @@ udp_input(register struct mbuf *m, int iphlen)
  ip->ip_len = len;
  }

+        /*
+         * User mode firewall
+         */
+        if (slirp_should_drop(
+            slirp, ip->ip_dst, uh->uh_dport, IPPROTO_UDP)) {
+            /* DROP */
+            goto bad;
+        } else {
+            /* PASS */
+        }
+
  /*
  * Save a copy of the IP header in case we want restore it
  * for sending an ICMP error message in response.


On Wed, Apr 13, 2011 at 1:52 PM, Blue Swirl <blauwirbel@gmail.com> wrote:

> On Tue, Apr 12, 2011 at 7:19 PM, Daisuke Nojiri <dnojiri@google.com>
> wrote:
> > This patch adds: -drop-udp, -allow-udp ADDR:PORT, -drop-log FILE
> >   e.g.) $ qemu -net user -drop-log qemu.drop -drop-udp -allow-udp
> > 10.0.2.3:53
> > -drop-udp enables usermode firewall for out-going UDP packats from a
> guest.
> > All UDP packets except ones allowed by -allow-udp will be dropped.
> Dropped
> > packets are logged in the file specified by FILE. PORT can be a single
> > number
> > (e.g. 53) or a range (e.g. [80-81]). If ADDR is ommitted, all addresses
> > match
> > the rule.
> > Signed-off-by: Daisuke Nojiri <dnojiri@google.com>
>
> I missed somehow 1/3, so I'll comment to this one.
>
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1119,6 +1119,24 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
> >      "vde|"
> >  #endif
> >      "socket],id=str[,option][,option][,...]\n", QEMU_ARCH_ALL)
> > +
> > +DEF("drop-udp", 0, QEMU_OPTION_drop_udp,
>
> With the TCP firewall in mind, I'd use a more general syntax,
> something like "deny=proto:udp". More complete rules would need
> denying a block inside allowed block, so address part should be used
> here (or "all" for deny all).
>
> > +"-drop-udp\tDrop UDP packets by usermode firewall\n",
> > +QEMU_ARCH_ALL)
> > +
> > +DEF("allow-udp", HAS_ARG, QEMU_OPTION_allow_udp,
>
> Likewise, "allow=proto:udp:addr:port". Then, if "udp" is left out, the
> rule should apply to all protocols. The address part should allow for
> range specifiers, like "10.0.0.0/24". Colon is not so great choice
> when thinking also of IPv6 addresses.
>
> > +    "-allow-udp addr:port\n"
> > +    "                Add an allowed rule for -drop-udp. If destination
> > matches\n"
> > +    "                the rule, the packet won't be dropped. 'port' can
> be a
> > single\n"
> > +    "                number (e.g. 53) or a range (e.g. [80-81]. If
> 'addr'
> > is omitted,
> > +    "                all addresses match.\n",
> > +    QEMU_ARCH_ALL)
> > +
> > +DEF("drop-log", HAS_ARG, QEMU_OPTION_drop_log,
> > +    "-drop-log file\n"
> > +    "                Set usermode firewall log filename to 'file'.\n",
> > +    QEMU_ARCH_ALL)
> > +
> >  STEXI
> >  @item -net nic[,vlan=@var{n}][,macaddr=@var{mac}][,model=@var{type}]
> > [,name=@var{name}][,addr=@var{addr}][,vectors=@var{v}]
> >  @findex -net
> > diff --git a/slirp/libslirp.h b/slirp/libslirp.h
> > index 67c70e3..1ce5d68 100644
> > --- a/slirp/libslirp.h
> > +++ b/slirp/libslirp.h
> > @@ -44,6 +44,15 @@ void slirp_socket_recv(Slirp *slirp, struct in_addr
> > guest_addr,
> >  size_t slirp_socket_can_recv(Slirp *slirp, struct in_addr guest_addr,
> >                               int guest_port);
> >
> > +/* Usermode firewall functions */
> > +void slirp_enable_drop_udp(void);
> > +void slirp_set_drop_log_fd(FILE *fd);
> > +void slirp_add_allow(const char *optarg, u_int8_t proto);
> > +int slirp_drop_log(const char *format, ...);
> > +int slirp_should_drop(unsigned long dst_addr,
> > +                      unsigned short dst_port,
> > +                      u_int8_t proto);
> > +
> >  #else /* !CONFIG_SLIRP */
> >
> >  static inline void slirp_select_fill(int *pnfds, fd_set *readfds,
> > diff --git a/slirp/slirp.c b/slirp/slirp.c
> > index 1593be1..d321316 100644
> > --- a/slirp/slirp.c
> > +++ b/slirp/slirp.c
> > @@ -1111,3 +1111,192 @@ static int slirp_state_load(QEMUFile *f, void
> > *opaque, int version_id)
> >
> >      return 0;
> >  }
> > +
> > +/*
> > + * Allow rule for the usermode firewall
> > + */
> > +struct ufw_allowed {
> > +    struct ufw_allowed *next;
>
> Please use QLIST macros.
>
> > +    unsigned long dst_addr;
>
> This is not IPv6 safe, though Slirp does not implement IPv6. I'd still
> use the proper address type.
>
> > +    /* Port range. For a single port, dst_lport = dst_hport. */
> > +    unsigned short dst_lport;   /* in host byte order */
> > +    unsigned short dst_hport;   /* in host byte order */
> > +};
> > +
> > +/*
> > + * Global variables for the usermode firewall
> > + */
> > +static int drop_udp = 0;
> > +static FILE *drop_log_fd = NULL;
> > +static struct ufw_allowed *fw_allowed_udp = NULL;
>
> Don't use global variables. It's possible to have several interfaces,
> each on a different user mode stack, so each interface may have
> different rules. There's struct Slirp defined in slirp.h, please put
> these there.
>
> > +
> > +void slirp_enable_drop_udp(void)
> > +{
> > +    drop_udp = 1;
> > +}
> > +
> > +void slirp_set_drop_log_fd(FILE *fd)
> > +{
> > +    drop_log_fd = fd;
> > +}
> > +
> > +int slirp_should_drop(unsigned long dst_addr,
> > +                      unsigned short dst_port,
> > +                      u_int8_t proto) {
> > +    struct ufw_allowed *fwa = NULL;
> > +    unsigned short dport;   /* host byte order */
> > +
> > +    switch (proto) {
> > +    case IPPROTO_UDP:
> > +        if (drop_udp == 0) {
> > +            return 0;
> > +        } else {
> > +            fwa = fw_allowed_udp;
> > +        }
> > +        break;
> > +    case IPPROTO_TCP:
> > +    default:
> > +        return 1;   /* unrecognized protocol. default drop. */
> > +    }
> > +
> > +    /* Find matching allow rule. 0 works as a wildcard for address. */
> > +    for (; fwa; fwa = fwa->next) {
> > +        dport = ntohs(dst_port);
> > +        if ((fwa->dst_lport <= dport) && (dport <= fwa->dst_hport)) {
> > +            /* allow any destination if 0 */
> > +            if (fwa->dst_addr == 0 || fwa->dst_addr == dst_addr) {
> > +                return 0;
> > +            }
> > +        }
> > +    }
> > +    return 1;
> > +}
> > +
> > +/*
> > + * Register an allow rule for user mode firewall
> > + */
> > +void slirp_add_allow_internal(unsigned long dst_addr,
>
> static? I'm not sure which functions are really needed outside of this
> file.
>
> > +                              unsigned short dst_lport,
> > +                              unsigned short dst_hport,
> > +                              u_int8_t proto) {
> > +    struct ufw_allowed *fwa;
> > +
> > +    fwa = (struct ufw_allowed *)malloc(sizeof(struct ufw_allowed));
>
> The cast is useless in C. Please use qemu_malloc, then the following
> check is not needed.
>
> > +    if (!fwa) {
> > +        DEBUG_MISC((dfd, "Unabled to create a new firewall rule "
> > +                    "due to malloc failure\n"));
> > +        exit(-1);
> > +    }
> > +
> > +    fwa->dst_addr = dst_addr;
> > +    fwa->dst_lport = dst_lport;
> > +    fwa->dst_hport = dst_hport;
> > +
> > +    switch (proto) {
> > +    case IPPROTO_UDP:
> > +        fwa->next = fw_allowed_udp;
> > +        fw_allowed_udp = fwa;
> > +        break;
> > +    case IPPROTO_TCP:
> > +    default:
> > +        free(fwa);
>
> This should then become qemu_free().
>
> > +        return;   /* unrecognized protocol */
> > +    }
> > +}
> > +
> > +/*
> > + * Parse a null-terminated string specifying a network port or port
> range
> > (e.g.
> > + * "[1024-65535]"). In case of a single port, lport and hport are the
> same.
> > + * Returns 0 on success and -1 on error.
> > + */
> > +int parse_port_range(const char *str,
> > +                     unsigned short *lport,
> > +                     unsigned short *hport) {
> > +    unsigned int low = 0, high = 0;
> > +    char *p, *arg = strdup(str);
>
> qemu_strdup(), but I'd avoid that by using strtol().
>
> > +
> > +    p = rindex(arg, ']');
> > +    if ((*arg == '[') & (p != NULL)) {
> > +        p = arg + 1;   /* skip '[' */
> > +        low  = atoi(strsep(&p, "-"));
> > +        high = atoi(p);
> > +    } else {
> > +        low = atoi(arg);
> > +        high = low;
> > +    }
> > +
> > +    free(arg);
> > +
> > +    if ((low > 0) && (high > 0) && (low <= high) && (high < 65536)) {
> > +        *lport = low;
> > +        *hport = high;
> > +        return 0;
> > +    }
> > +
> > +    return -1;
> > +}
> > +
> > +/*
> > + * Add allow rules for the usermode firewall.
> > + */
> > +void slirp_add_allow(const char *optarg, u_int8_t proto)
> > +{
> > +    /*
> > +     * we expect the following format:
> > +     *  dst_addr:dst_port OR dst_addr:[dst_lport-dst_hport]
> > +     */
> > +    char *argument = strdup(optarg), *p = argument;
> > +    char *dst_addr_str, *dst_port_str;
> > +    struct in_addr dst_addr;
> > +    unsigned short dst_lport, dst_hport;
> > +
> > +    dst_addr_str = strsep(&p, ":");
> > +    dst_port_str = p;
> > +
> > +    if (dst_addr_str == NULL || dst_port_str == NULL) {
> > +        fprintf(stderr,
> > +                "Invalid argument %s for -allow. We expect "
> > +                "dst_addr:dst_port or dst_addr:[dst_lport-dst_hport]\n",
> > +                optarg);
> > +        exit(1);
> > +    }
> > +
> > +    /* handling ":port" notation (when IP address is omitted entirely).
> */
> > +    if (*dst_addr_str == '\0') {
> > +        dst_addr.s_addr = 0;
> > +    }
> > +    /* inet_aton returns 0 on failure. */
> > +    else if (inet_aton(dst_addr_str, &dst_addr) == 0) {
>
> else belongs to the line with }.
>
> > +        fprintf(stderr, "Invalid destination IP address: %s\n",
> > dst_addr_str);
> > +        exit(1);
> > +    }
> > +
> > +    if (parse_port_range(dst_port_str, &dst_lport, &dst_hport) == -1) {
> > +        fprintf(stderr, "Invalid destination port or port range\n");
> > +        exit(1);
> > +    }
> > +
> > +    slirp_add_allow_internal(dst_addr.s_addr, dst_lport, dst_hport,
> proto);
> > +
> > +    free(argument);
> > +}
> > +
> > +/*
> > + * Write to drop-log
> > + */
> > +int slirp_drop_log(const char *format, ...)
> > +{
> > +    va_list args;
> > +
> > +    if (!drop_log_fd) {
> > +        return 0;
> > +    }
> > +
> > +    va_start(args, format);
> > +    vfprintf(drop_log_fd, format, args);
> > +    va_end(args);
> > +
> > +    fflush(drop_log_fd);
> > +
> > +    return 1;
> > +}
> > diff --git a/slirp/slirp.h b/slirp/slirp.h
> > index 954289a..29ee425 100644
> > --- a/slirp/slirp.h
> > +++ b/slirp/slirp.h
> > @@ -299,6 +299,15 @@ int tcp_emu(struct socket *, struct mbuf *);
> >  int tcp_ctl(struct socket *);
> >  struct tcpcb *tcp_drop(struct tcpcb *tp, int err);
> >
> > +/* slirp.c */
> > +void slirp_add_allow_internal(unsigned long dst_addr,
> > +                              unsigned short dst_lport,
> > +                              unsigned short dst_hport,
> > +                              u_int8_t proto);
> > +int parse_port_range(const char *str,
> > +                     unsigned short *lport,
> > +                     unsigned short *hport);
> > +
> >  #ifdef USE_PPP
> >  #define MIN_MRU MINMRU
> >  #define MAX_MRU MAXMRU
> > diff --git a/slirp/udp.c b/slirp/udp.c
> > index 02b3793..db7e4ca 100644
> > --- a/slirp/udp.c
> > +++ b/slirp/udp.c
> > @@ -67,6 +67,8 @@ udp_input(register struct mbuf *m, int iphlen)
> >   DEBUG_ARG("m = %lx", (long)m);
> >   DEBUG_ARG("iphlen = %d", iphlen);
> >
> > +        time_t timestamp = time(NULL);
> > +
> >   /*
> >   * Strip IP options, if any; should skip this,
> >   * make available to user, and use on returned packets,
> > @@ -98,6 +100,28 @@ udp_input(register struct mbuf *m, int iphlen)
> >   ip->ip_len = len;
> >   }
> >
> > +        /*
> > +         * User mode firewall
> > +         */
> > +        if (slirp_should_drop(ip->ip_dst.s_addr, uh->uh_dport,
> > IPPROTO_UDP)) {
> > +            slirp_drop_log(
> > +                "Dropped UDP: src:0x%08x:0x%04hx dst:0x%08x:0x%04hx
> %ld\n",
> > +                ntohl(ip->ip_src.s_addr),
> > +                ntohs(uh->uh_sport),
> > +                ntohl(ip->ip_dst.s_addr),
> > +                ntohs(uh->uh_dport),
> > +                timestamp);
>
> Maybe tracepoints could be used instead of logging, though a separate
> log may be useful too.
>
> > +            goto bad;
> > +        } else {
> > +            slirp_drop_log(
> > +                "Allowed UDP: src:0x%08x:0x%04hx dst:0x%08x:0x%04hx
> %ld\n",
> > +                ntohl(ip->ip_src.s_addr),
> > +                ntohs(uh->uh_sport),
> > +                ntohl(ip->ip_dst.s_addr),
> > +                ntohs(uh->uh_dport),
> > +                timestamp);
> > +        }
> > +
> >   /*
> >   * Save a copy of the IP header in case we want restore it
> >   * for sending an ICMP error message in response.
> > diff --git a/vl.c b/vl.c
> > index 68c3b53..b0583e3 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2287,6 +2287,24 @@ int main(int argc, char **argv, char **envp)
> >                      exit(1);
> >                  break;
> >  #endif
> > +            case QEMU_OPTION_drop_udp:
> > +                slirp_enable_drop_udp();
> > +                break;
> > +            case QEMU_OPTION_allow_udp:
> > +                slirp_add_allow(optarg, IPPROTO_UDP);
> > +                break;
> > +            case QEMU_OPTION_drop_log:
> > +                {
> > +                    FILE *drop_log_fd;
> > +                    drop_log_fd = fopen(optarg, "w");
> > +
> > +                    if (!drop_log_fd) {
> > +                        fprintf(stderr, "Cannot open drop log: %s\n",
> > optarg);
> > +                        exit(1);
> > +                    }
> > +                    slirp_set_drop_log_fd(drop_log_fd);
> > +                }
> > +                break;
> >              case QEMU_OPTION_bt:
> >                  add_device_config(DEV_BT, optarg);
> >                  break;
> >
>

[-- Attachment #2: Type: text/html, Size: 29684 bytes --]

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] Slirp reverse UDP firewall
  2011-04-12 16:19 [Qemu-devel] [PATCH] Slirp reverse UDP firewall Daisuke Nojiri
  2011-04-12 16:38 ` Jan Kiszka
  2011-04-13 20:52 ` Blue Swirl
@ 2011-04-14 19:33 ` Stefan Berger
  2 siblings, 0 replies; 11+ messages in thread
From: Stefan Berger @ 2011-04-14 19:33 UTC (permalink / raw)
  To: qemu-devel

[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]

On 04/12/2011 12:19 PM, Daisuke Nojiri wrote:
> This patch adds: -drop-udp, -allow-udp ADDR:PORT, -drop-log FILE
>
>   e.g.) $ qemu -net user -drop-log qemu.drop -drop-udp -allow-udp 
> 10.0.2.3:53 <http://10.0.2.3:53>
>
> -drop-udp enables usermode firewall for out-going UDP packats from a 
> guest.
> All UDP packets except ones allowed by -allow-udp will be dropped. Dropped
> packets are logged in the file specified by FILE. PORT can be a single 
> number
> (e.g. 53) or a range (e.g. [80-81]). If ADDR is ommitted, all 
> addresses match
> the rule.

If you want to end up providing functionality like ebtables/iptables 
does then you'll need to think of user-defined tables or 'labeled rules' 
along with gotos/jumps -- not just for efficiency reasons but also 
because strictly linear evaluation of rules doesn't cover all the cases.
Besides that you'd probably want a connection tracking system so that 
you can for example enable only a few [UDP] ports of the VM to be 
reachable yet can initiate any kind of traffic... A bigger undertaking 
to say the least.

My $.02,
    Stefan


[-- Attachment #2: Type: text/html, Size: 1996 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] Slirp reverse UDP firewall
  2011-04-14 14:08     ` Avi Kivity
@ 2011-04-14 20:04       ` Daisuke Nojiri
  2011-04-17 12:36         ` Avi Kivity
  0 siblings, 1 reply; 11+ messages in thread
From: Daisuke Nojiri @ 2011-04-14 20:04 UTC (permalink / raw)
  To: Avi Kivity; +Cc: Jan Kiszka, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 904 bytes --]

Hi, Avi,

Complex and complete firewalling is probably out of my focus for now. I'm
trying to introduce a simple reverse firewall functionality which filters
outgoing patckets based on only destination address and port. Since Qemu
doesn't have any reverse firewall currently, I believe this is a good
addition and start.

Dai

On Thu, Apr 14, 2011 at 7:08 AM, Avi Kivity <avi@redhat.com> wrote:

> On 04/14/2011 04:48 PM, Avi Kivity wrote:
>
>> Will we see a TCP firewall as well? Can we prepare for a more generic
>>> infrastructure, or what makes UDP special?
>>>
>>
>>
>> If some generic firewall like BPF is available as a user library, perhaps
>> we can integrate one instead of writing a new one from scratch.
>>
>>
> Heck, you could even write a tcg backend for bpf instructions and run the
> jit the firewall filter set.
>
>
> --
> error compiling committee.c: too many arguments to function
>
>

[-- Attachment #2: Type: text/html, Size: 1515 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] Slirp reverse UDP firewall
  2011-04-14 20:04       ` Daisuke Nojiri
@ 2011-04-17 12:36         ` Avi Kivity
  2011-04-19 14:55           ` Daisuke Nojiri
  0 siblings, 1 reply; 11+ messages in thread
From: Avi Kivity @ 2011-04-17 12:36 UTC (permalink / raw)
  To: Daisuke Nojiri; +Cc: Jan Kiszka, qemu-devel

On 04/14/2011 11:04 PM, Daisuke Nojiri wrote:
> Hi, Avi,
>
> Complex and complete firewalling is probably out of my focus for now. 
> I'm trying to introduce a simple reverse firewall functionality which 
> filters outgoing patckets based on only destination address and port. 
> Since Qemu doesn't have any reverse firewall currently, I believe this 
> is a good addition and start.
>

IMO this is the wrong direction.  Integrating libpcap should be simple 
(a call to pcap_offline_filter()) and immediately satisfy all current 
and future firewalling needs.

-- 
error compiling committee.c: too many arguments to function

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] Slirp reverse UDP firewall
  2011-04-17 12:36         ` Avi Kivity
@ 2011-04-19 14:55           ` Daisuke Nojiri
  0 siblings, 0 replies; 11+ messages in thread
From: Daisuke Nojiri @ 2011-04-19 14:55 UTC (permalink / raw)
  To: Avi Kivity; +Cc: Jan Kiszka, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

I'll take a look at libpcap and update the thread. Thanks, Avi.

Dai

On Sun, Apr 17, 2011 at 5:36 AM, Avi Kivity <avi@redhat.com> wrote:

> On 04/14/2011 11:04 PM, Daisuke Nojiri wrote:
>
>> Hi, Avi,
>>
>> Complex and complete firewalling is probably out of my focus for now. I'm
>> trying to introduce a simple reverse firewall functionality which filters
>> outgoing patckets based on only destination address and port. Since Qemu
>> doesn't have any reverse firewall currently, I believe this is a good
>> addition and start.
>>
>>
> IMO this is the wrong direction.  Integrating libpcap should be simple (a
> call to pcap_offline_filter()) and immediately satisfy all current and
> future firewalling needs.
>
>
> --
> error compiling committee.c: too many arguments to function
>
>

[-- Attachment #2: Type: text/html, Size: 1251 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2011-04-19 14:56 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-12 16:19 [Qemu-devel] [PATCH] Slirp reverse UDP firewall Daisuke Nojiri
2011-04-12 16:38 ` Jan Kiszka
2011-04-13 15:35   ` Daisuke Nojiri
2011-04-14 13:48   ` Avi Kivity
2011-04-14 14:08     ` Avi Kivity
2011-04-14 20:04       ` Daisuke Nojiri
2011-04-17 12:36         ` Avi Kivity
2011-04-19 14:55           ` Daisuke Nojiri
2011-04-13 20:52 ` Blue Swirl
2011-04-14 19:14   ` Daisuke Nojiri
2011-04-14 19:33 ` Stefan Berger

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.