Merge lp:~davidmhewitt/switchboard-plug-security-privacy/fix-1312461-control-rule into lp:~elementary-apps/switchboard-plug-security-privacy/trunk

[David Hewitt]

Firewall:
* UFW rule creation now respects the IPv4/IPv6 flag set in the Rule structure.
* New dropdown in rule creation popover to specify IPv4/IPv6/Both rule.
* New rules are created as IPv4 by default.
* Use regular expressions to parse UFW command output
* Add a checkbox to enable/disable rules

The IPv6 checkbox was slightly confusing in that it looked like a control that you could modify. This branch turns the IPv6 status into a text field and adds a checkbox in the same place that allows you to enable/disable a rule.

To allow this functionality, the parsing and displaying of UFW rules has been improved as rules that had been set using the UFW command line were often incorrectly displayed and interpreted by the plug. This made the enable/disable functionality on those rules really buggy.

Disabled rules are stored in GSettings as UFW has no concept of a disabled rule. When a rule is disabled by the plug, it is deleted from UFW and stored in GSettings.

[Adam Bieńkowski (donadigo)]

I really don't like the validation of IP addresses with the regex. It makes the code look like clutter.
Instead you can use much simpler solution with GLib.InetAddress: by using "InetAddress.from_string" to parse the IP address you can check if it's null to make sure it's valid and you can also call "get_family" on it to retrive if it's IPv4 or IPv6 address.

Also, about this line:
while (iter.next ("(ssssiiii)", &to, &to_ports, &from, &from_ports, &action, &protocol, &direction, &version)) {

do you really need to use those C like arguments in order to retrieve needed variables? Does "out" not compile?

review: Needs Fixing (code)

[David Hewitt (davidmhewitt)]

> I really don't like the validation of IP addresses with the regex. It makes
> the code look like clutter.
> Instead you can use much simpler solution with GLib.InetAddress: by using
> "InetAddress.from_string" to parse the IP address you can check if it's null
> to make sure it's valid and you can also call "get_family" on it to retrive if
> it's IPv4 or IPv6 address.
>
> Also, about this line:
> while (iter.next ("(ssssiiii)", &to, &to_ports, &from, &from_ports, &action,
> &protocol, &direction, &version)) {
>
> do you really need to use those C like arguments in order to retrieve needed
> variables? Does "out" not compile?

Thanks for the review, Adam. I'll try and fix the C like arguments, you're probably right that the out keyword will work.

As for using the InetAddress stuff, I tried that first and it wasn't suitable. Unfortunately, when you pass InetAddress.from_string an invalid IP Address, it throws an assertion instead of an exception meaning it terminates the program. Considering I'd be trying to use it to validate if something is an IP address or not, it's no help.

However, if you have any better solutions to check if a string is an IPv4 address, IPv6 address or something else, I'd be open to suggestions.

[David Hewitt (davidmhewitt)]

> I really don't like the validation of IP addresses with the regex. It makes
> the code look like clutter.
> Instead you can use much simpler solution with GLib.InetAddress: by using
> "InetAddress.from_string" to parse the IP address you can check if it's null
> to make sure it's valid and you can also call "get_family" on it to retrive if
> it's IPv4 or IPv6 address.
>
> Also, about this line:
> while (iter.next ("(ssssiiii)", &to, &to_ports, &from, &from_ports, &action,
> &protocol, &direction, &version)) {
>
> do you really need to use those C like arguments in order to retrieve needed
> variables? Does "out" not compile?

My mistake, you can totally use InetAddress instead of the regular expressions, don't know what made me think it didn't work the first time. Thanks for the suggestion, that's saved a bunch of horrible code :)

I've implemented both suggestions!

[Adam Bieńkowski (donadigo)]

Besides 2 places where the code is a little bit inconsistent in code style, it looks good (code).

review: Approve (code)

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2016-12-01 11:56:30 +0000
+++ CMakeLists.txt	2017-03-06 18:53:07 +0000
@@ -38,9 +38,7 @@
 include (CPack)
 add_custom_target (dist COMMAND ${CMAKE_MAKE_PROGRAM} package_source)
 
-configure_file (${CMAKE_SOURCE_DIR}/data/org.pantheon.security-privacy.policy.cmake ${CMAKE_BINARY_DIR}/data/org.pantheon.security-privacy.policy)
-install(FILES ${CMAKE_BINARY_DIR}/data/org.pantheon.security-privacy.policy DESTINATION ${CMAKE_INSTALL_FULL_DATAROOTDIR}/polkit-1/actions/)
-install(FILES ${CMAKE_SOURCE_DIR}/data/security-privacy-plug-helper PERMISSIONS OWNER_EXECUTE OWNER_READ DESTINATION ${PKGDATADIR}/)
+add_subdirectory (data)
 
 # Traslation stuff
 add_subdirectory (po)
 
=== added file 'data/CMakeLists.txt'
--- data/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ data/CMakeLists.txt	2017-03-06 18:53:07 +0000
@@ -0,0 +1,6 @@
+configure_file (${CMAKE_SOURCE_DIR}/data/org.pantheon.security-privacy.policy.cmake ${CMAKE_BINARY_DIR}/data/org.pantheon.security-privacy.policy)
+install(FILES ${CMAKE_BINARY_DIR}/data/org.pantheon.security-privacy.policy DESTINATION ${CMAKE_INSTALL_FULL_DATAROOTDIR}/polkit-1/actions/)
+install(FILES ${CMAKE_SOURCE_DIR}/data/security-privacy-plug-helper PERMISSIONS OWNER_EXECUTE OWNER_READ DESTINATION ${PKGDATADIR}/)
+
+include (GSettings)
+add_schema ("org.pantheon.security-privacy.gschema.xml")
 
=== added file 'data/org.pantheon.security-privacy.gschema.xml'
--- data/org.pantheon.security-privacy.gschema.xml	1970-01-01 00:00:00 +0000
+++ data/org.pantheon.security-privacy.gschema.xml	2017-03-06 18:53:07 +0000
@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<schemalist>
+
+<schema id="org.pantheon.security-privacy" path="/org/pantheon/security-privacy/">
+    <key name="disabled-firewall-rules" type="a(ssssiiii)">
+        <default>[]</default>
+        <summary>Firewall rules that have been disabled, but should still be listed in the plug</summary>
+        <description>An array of rules, each containing action, protocol, direction and (bool v6) in that order.</description>
+    </key>
+</schema>
+
+</schemalist>
 
=== modified file 'src/UFWHelpers.vala'
--- src/UFWHelpers.vala	2017-03-01 22:38:37 +0000
+++ src/UFWHelpers.vala	2017-03-06 18:53:07 +0000
@@ -103,17 +103,54 @@
                 default:
                     rule_str = "%s in".printf (rule_str);
                     break;
-            }
+            } 
 
             switch (rule.protocol) {
                 case Rule.Protocol.UDP:
-                    rule_str = "%s %s/udp".printf (rule_str, rule.ports);
+                    rule_str = "%s proto udp".printf (rule_str);
+                    break;
+                case Rule.Protocol.BOTH:
                     break;
                 default:
-                    rule_str = "%s %s/tcp".printf (rule_str, rule.ports);
+                    rule_str = "%s proto tcp".printf (rule_str);
                     break;
             }
 
+            if (rule.to != "" && !rule.to.contains ("Anywhere")) {
+                rule_str = "%s to %s".printf (rule_str, rule.to);
+                if (rule.to_ports != "") {
+                    rule_str = "%s port %s".printf (rule_str, rule.to_ports);                    
+                }            
+            } else {
+                if (rule.version == Rule.Version.BOTH) {
+                    rule_str = "%s to any".printf (rule_str);                
+                } else if (rule.version == Rule.Version.IPV6) {
+                    rule_str = "%s to ::/0".printf (rule_str);
+                } else if (rule.version == Rule.Version.IPV4) {
+                    rule_str = "%s to 0.0.0.0/0".printf (rule_str);
+                }
+                if (rule.to_ports != "") {
+                    rule_str = "%s port %s".printf (rule_str, rule.to_ports);
+                }
+            }
+
+            if (rule.from != "" && !rule.from.contains ("Anywhere")) {
+                rule_str = "%s from %s".printf (rule_str, rule.from);
+                if (rule.from_ports != "") {
+                    rule_str = "%s port %s".printf (rule_str, rule.from_ports);
+                }            
+            } else {
+                if (rule.version == Rule.Version.BOTH) {
+                    rule_str = "%s from any".printf (rule_str);                
+                } else if (rule.version == Rule.Version.IPV6) {
+                    rule_str = "%s from ::/0".printf (rule_str);
+                } else if (rule.version == Rule.Version.IPV4) {
+                    rule_str = "%s from 0.0.0.0/0".printf (rule_str);
+                }
+                if (rule.from_ports != "") {
+                    rule_str = "%s port %s".printf (rule_str, rule.from_ports);
+                }
+            }
             Process.spawn_command_line_sync ("pkexec %s -5 \"%s\"".printf (get_helper_path (), rule_str));
         } catch (Error e) {
             warning (e.message);
@@ -130,7 +167,8 @@
 
         public enum Protocol {
             UDP,
-            TCP
+            TCP,
+            BOTH
         }
 
         public enum Direction {
@@ -138,60 +176,127 @@
             OUT
         }
 
+        public enum Version {
+            IPV4,
+            IPV6,
+            BOTH
+        }
+
         public Action action;
         public Protocol protocol;
         public Direction direction;
-        public string ports;
-        public bool is_v6 = false;
+        public string to_ports = "";
+        public string from_ports = "";
+        public string to = "";
+        public string from = "";
+        public Version version = Version.BOTH;
         public int number;
 
         public Rule () {
             
         }
 
+        private void get_address_and_port (string input, ref Version version, ref string ports, ref string address) {
+            try {
+                var parts = input.split (" ");
+                if (parts.length > 1) {
+                    ports = parts[1].split("/")[0];
+                    address = parts[0];
+                    var ip = new InetAddress.from_string (parts[0].split ("/")[0]);
+                    if (ip != null) {
+                        if (ip.get_family () == SocketFamily.IPV6) {
+                            version = Version.IPV6;
+                        } else {
+                            version = Version.IPV4;
+                        }
+                    }
+                } else {
+                    var ip_parts = parts[0].split ("/");
+                    if (ip_parts.length > 1) {
+                        if (ip_parts[1] == "tcp" || ip_parts[1] == "udp") {
+                            ports = ip_parts[0];
+                        } else {
+                            address = parts[0];
+                            var ip = new InetAddress.from_string (ip_parts[0]);
+                            if (ip != null) {
+                                if (ip.get_family () == SocketFamily.IPV6) {
+                                    version = Version.IPV6;
+                                } else {
+                                    version = Version.IPV4;
+                                }
+                            }
+                        }
+                    } else {
+                        var ip = new InetAddress.from_string (ip_parts[0]);
+                        if (ip == null) {
+                            if (ip_parts[0].contains ("Anywhere")) {
+                                address = "Anywhere";
+                            } else {
+                                ports = ip_parts[0];
+                            }
+                        } else if (ip.get_family () == SocketFamily.IPV6) {
+                            address = ip_parts[0];
+                            version = Version.IPV6;
+                        } else if (ip.get_family () == SocketFamily.IPV4) {
+                            address = ip_parts[0];
+                            version = Version.IPV4;
+                        }
+                    }
+                }  
+            } catch (Error e) {
+                warning ("Error parsing to/from address: %s".printf (input));
+                warning (e.message);
+            }  
+        }
+
         public Rule.from_line (string line) {
-            if (line.contains ("(v6)"))
-                is_v6 = true;
-            var first = line.replace ("(v6)", "").split ("] ");
-            number = int.parse (first[0].replace ("[", ""));
-            var second = first[1];
-            var third = second.split ("/");
-            ports = third[0];
-            string current = "";
-            int position = 0;
-            foreach (var car in third[1].data) {
-                if (car == ' ') {
-                    if (current == "") {
-                        continue;
-                    }
-
-                    if (position == 0) {
-                        if ("udp" in current)
-                            protocol = Protocol.UDP;
-                        else if ("tcp" in current)
-                            protocol = Protocol.TCP;
-                    } else if (position == 1) {
-                        if ("ALLOW" in current)
-                            action = Action.ALLOW;
-                        else if ("DENY" in current)
-                            action = Action.DENY;
-                        else if ("REJECT" in current)
-                            action = Action.REJECT;
-                        else if ("LIMIT" in current)
-                            action = Action.LIMIT;
-                    } else if (position == 2) {
-                        if ("IN" in current)
-                            direction = Direction.IN;
-                        else if ("OUT" in current)
-                            direction = Direction.OUT;
-                        break;
-                    }
-
-                    current = "";
-                    position++;
-                    continue;
-                }
-                current = "%s%c".printf (current, car);
+            if (line.contains ("(v6)")) {
+                version = Version.IPV6;
+            } else {
+                version = Version.IPV4;
+            }
+
+            if (line.contains ("tcp")) {
+                protocol = Protocol.TCP;
+            } else if (line.contains ("udp")) {
+                protocol = Protocol.UDP;
+            } else {
+                protocol = Protocol.BOTH;
+            }
+
+            try {
+                               
+                var r = new Regex ("""\[\s*(\d+)\]\s{1}([A-Za-z0-9 \(\)/\.:,]+?)\s{2,}([A-Z ]+?)\s{2,}([A-Za-z0-9 \(\)/\.:,]+?)(?:\s{2,}.*)?$""");
+                MatchInfo info;
+                r.match (line, 0, out info);
+                
+                number = int.parse (info.fetch (1));
+
+                string to_match = info.fetch (2).replace (" (v6)", "");
+                string from_match = info.fetch (4).replace (" (v6)", "");
+
+                get_address_and_port (to_match, ref version, ref to_ports, ref to);
+                get_address_and_port (from_match, ref version, ref from_ports, ref from);
+                
+                string type = info.fetch (3);
+
+                if ("ALLOW" in type) {
+                    action = Action.ALLOW;
+                } else if ("DENY" in type) {
+                    action = Action.DENY;
+                } else if ("REJECT" in type) {
+                    action = Action.REJECT;
+                } else if ("LIMIT" in type) {
+                    action = Action.LIMIT;
+                }
+
+                if ("IN" in type) {
+                    direction = Direction.IN;
+                } else if ("OUT" in type) {
+                    direction = Direction.OUT;
+                }
+            } catch (Error e) {
+                return;
             }
         }
     }
 
=== modified file 'src/Views/FirewallPanel.vala'
--- src/Views/FirewallPanel.vala	2017-03-02 17:18:04 +0000
+++ src/Views/FirewallPanel.vala	2017-03-06 18:53:07 +0000
@@ -27,13 +27,17 @@
     private bool loading = false;
     private Gtk.Popover add_popover;
     private Gtk.ToolButton remove_button;
+    private Settings settings;
+    private Gee.HashMap<string, UFWHelpers.Rule> disabled_rules;
 
     private enum Columns {
         ACTION,
         PROTOCOL,
         DIRECTION,
-        PORTS,
+        TO,
+        FROM,
         V6,
+        ENABLED,
         RULE,
         N_COLUMNS
     }
@@ -45,6 +49,10 @@
     }
 
     construct {
+        settings = new Settings ("org.pantheon.security-privacy");
+        disabled_rules = new Gee.HashMap<string, UFWHelpers.Rule> ();
+        load_disabled_rules ();
+        
         status_switch.notify["active"].connect (() => {
             if (loading == false) {
                 view.sensitive = status_switch.active;
@@ -65,9 +73,7 @@
             remove_button.sensitive = false;
             if (status_switch.active == true) {
                 view.sensitive = true;
-                foreach (var rule in UFWHelpers.get_rules ()) {
-                    add_rule (rule);
-                }
+                show_rules ();
             } else {
                 view.sensitive = false;
             }
@@ -75,15 +81,114 @@
         });
     }
 
+    private void load_disabled_rules () {
+        disabled_rules = new Gee.HashMap<string, UFWHelpers.Rule> ();
+        string? to = "", to_ports = "", from = "", from_ports = "";
+        int action = 0, protocol = 0, direction = 0, version = 0;
+        var rules = settings.get_value ("disabled-firewall-rules");
+        VariantIter iter = rules.iterator ();
+        while (iter.next ("(ssssiiii)", ref to, ref to_ports, ref from, ref from_ports, ref action, ref protocol, ref direction, ref version)) {
+		    UFWHelpers.Rule new_rule = new UFWHelpers.Rule ();
+            new_rule.to = to;
+            new_rule.to_ports = to_ports;
+            new_rule.from = from;
+            new_rule.from_ports = from_ports;
+            new_rule.action = (UFWHelpers.Rule.Action)action;
+            new_rule.protocol = (UFWHelpers.Rule.Protocol)protocol;
+            new_rule.direction = (UFWHelpers.Rule.Direction)direction;
+            new_rule.version = (UFWHelpers.Rule.Version)version;
+            string hash = generate_hash_for_rule (new_rule);
+            disabled_rules.set (hash, new_rule);
+        }    
+    }
+
+    private string generate_hash_for_rule (UFWHelpers.Rule r) {
+        return r.to + 
+               r.to_ports + 
+               r.from + 
+               r.from_ports + 
+               r.action.to_string () + 
+               r.protocol.to_string () + 
+               r.direction.to_string () + 
+               r.version.to_string ();
+    }
+    
+    private void reload_rule_numbers () {
+        foreach (var rule in UFWHelpers.get_rules ()) {
+            string ufw_hash = generate_hash_for_rule (rule);
+            Gtk.TreeModelForeachFunc update_row = (model, path, iter) => {
+                Value val;
+
+		        list_store.get_value (iter, Columns.RULE, out val);
+                var tree_rule = (UFWHelpers.Rule)val;
+                string tree_hash = generate_hash_for_rule (tree_rule);
+		        if (ufw_hash == tree_hash) {
+                    tree_rule.number = rule.number;
+                    list_store.set_value (iter, Columns.RULE, tree_rule);
+                    return true;
+                }
+                
+		        return false;
+	        };
+            list_store.foreach (update_row);
+        }    
+    }
+
     private void show_rules () {
         list_store.clear ();
         remove_button.sensitive = false;
         foreach (var rule in UFWHelpers.get_rules ()) {
             add_rule (rule);
         }
-    }
-
-    public void add_rule (UFWHelpers.Rule rule) {
+        
+        load_disabled_rules ();
+        foreach (var rule in disabled_rules.entries) {
+            add_rule (rule.value, false, rule.key);
+        }
+    }
+
+    private void disable_rule (UFWHelpers.Rule rule) {
+        save_disabled_rules (rule);
+        UFWHelpers.remove_rule (rule);
+    }
+
+    private void enable_rule (string hash) {
+        UFWHelpers.add_rule (disabled_rules.get (hash));
+        delete_disabled_rule (hash);        
+    }
+
+    private void delete_disabled_rule (string hash) {
+        disabled_rules.unset (hash);
+        save_disabled_rules ();
+    }
+
+    private void save_disabled_rules (UFWHelpers.Rule? additional_rule = null) {
+        VariantBuilder builder = new VariantBuilder (new VariantType("a(ssssiiii)"));
+        foreach (var existing_rule in disabled_rules.values) {
+            builder.add ("(ssssiiii)", existing_rule.to, 
+                                       existing_rule.to_ports, 
+                                       existing_rule.from, 
+                                       existing_rule.from_ports,
+                                       existing_rule.action, 
+                                       existing_rule.protocol, 
+                                       existing_rule.direction, 
+                                       existing_rule.version);
+        }
+        if (additional_rule != null) {
+            builder.add ("(ssssiiii)", additional_rule.to, 
+                                       additional_rule.to_ports, 
+                                       additional_rule.from, 
+                                       additional_rule.from_ports,
+                                       additional_rule.action, 
+                                       additional_rule.protocol, 
+                                       additional_rule.direction, 
+                                       additional_rule.version);
+        }
+        settings.set_value ("disabled-firewall-rules", builder.end ());
+        load_disabled_rules ();
+    }
+
+    public void add_rule (UFWHelpers.Rule rule, bool enabled = true, string hash = "") {
         Gtk.TreeIter iter;
         string action = _("Unknown");
         if (rule.action == UFWHelpers.Rule.Action.ALLOW) {
@@ -100,6 +205,8 @@
             protocol = "UDP";
         } else if (rule.protocol == UFWHelpers.Rule.Protocol.TCP) {
             protocol = "TCP";
+        } else if (rule.protocol == UFWHelpers.Rule.Protocol.BOTH) {
+            protocol = "TCP/UDP";
         }
         string direction = _("Unknown");
         if (rule.direction == UFWHelpers.Rule.Direction.IN) {
@@ -107,27 +214,86 @@
         } else if (rule.direction == UFWHelpers.Rule.Direction.OUT) {
             direction = _("Out");
         }
+        string version = _("Unknown");
+        if (rule.version == UFWHelpers.Rule.Version.IPV6) {
+            version = "IPv6";
+        } else if (rule.version == UFWHelpers.Rule.Version.IPV4) {
+            version = "IPv4";
+        }
+        
+        string from = "";
+        string to = "";
+        if (rule.from_ports != "") {
+            if (rule.from_ports.contains (":") || rule.from_ports.contains (",")) {
+                from = _("%s Ports %s").printf (rule.from, rule.from_ports.replace (":", "-"));
+            } else {
+                from = _("%s Port %s").printf (rule.from, rule.from_ports.replace (":", "-"));
+            }
+        } else {
+            from = rule.from;
+        }
+
+        if (rule.to_ports != "") {
+            if (rule.to_ports.contains (":") || rule.to_ports.contains (",")) {
+                to = _("%s Ports %s").printf (rule.to, rule.to_ports.replace (":", "-"));
+            } else {
+                to = _("%s Port %s").printf (rule.to, rule.to_ports.replace (":", "-"));
+            }
+        } else {
+            to = rule.to;
+        }
+ 
         list_store.append (out iter);
         list_store.set (iter, Columns.ACTION, action, Columns.PROTOCOL, protocol,
-                Columns.DIRECTION, direction, Columns.PORTS, rule.ports.replace (":", "-"),
-                Columns.V6, rule.is_v6, Columns.RULE, rule);
+                Columns.DIRECTION, direction, Columns.V6, version, Columns.ENABLED, enabled, 
+                Columns.RULE, rule, Columns.TO, to.strip (), Columns.FROM, from.strip ());
     }
 
     private void create_treeview () {
         list_store = new Gtk.ListStore (Columns.N_COLUMNS, typeof (string),
-                typeof (string), typeof (string), typeof (string), typeof (bool), typeof (UFWHelpers.Rule));
+                                                           typeof (string), 
+                                                           typeof (string),     
+                                                           typeof (string),
+                                                           typeof (string), 
+                                                           typeof (string), 
+                                                           typeof (bool), 
+                                                           typeof (UFWHelpers.Rule));
 
         // The View:
         view = new Gtk.TreeView.with_model (list_store);
         view.vexpand = true;
+        view.activate_on_single_click = true;
 
         var celltoggle = new Gtk.CellRendererToggle ();
         var cell = new Gtk.CellRendererText ();
-        view.insert_column_with_attributes (-1, _("IPv6"), celltoggle, "active", Columns.V6);
+        view.insert_column_with_attributes (-1, _("Enabled"), celltoggle, "active", Columns.ENABLED);
+        view.insert_column_with_attributes (-1, _("Version"), cell, "text", Columns.V6);
         view.insert_column_with_attributes (-1, _("Action"), cell, "text", Columns.ACTION);
         view.insert_column_with_attributes (-1, _("Protocol"), cell, "text", Columns.PROTOCOL);
         view.insert_column_with_attributes (-1, _("Direction"), cell, "text", Columns.DIRECTION);
-        view.insert_column_with_attributes (-1, _("Ports"), cell, "text", Columns.PORTS);
+        view.insert_column_with_attributes (-1, _("To"), cell, "text", Columns.TO);
+        view.insert_column_with_attributes (-1, _("From"), cell, "text", Columns.FROM);
+
+        celltoggle.toggled.connect ((path) => {
+            Value active;
+            Gtk.TreeIter iter;
+            list_store.get_iter (out iter, new Gtk.TreePath.from_string(path));
+            list_store.get_value (iter, Columns.ENABLED, out active);
+            var is_active = !active.get_boolean ();
+            list_store.set (iter, Columns.ENABLED, is_active);
+
+            Value rule_value;
+            list_store.get_value (iter, Columns.RULE, out rule_value);
+            UFWHelpers.Rule rule = (UFWHelpers.Rule)rule_value.get_object ();
+            string gen_hash = generate_hash_for_rule (rule);
+            if (is_active == false) {
+                disable_rule (rule);
+            } else {
+                enable_rule (gen_hash);
+            }
+
+            reload_rule_numbers ();
+        });
 
         list_toolbar = new Gtk.Toolbar ();
         list_toolbar.get_style_context ().add_class (Gtk.STYLE_CLASS_INLINE_TOOLBAR);
@@ -160,6 +326,14 @@
             protocol_combobox.append_text ("UDP");
             protocol_combobox.active = 0;
 
+            var version_label = new Gtk.Label (_("Version:"));
+            version_label.xalign = 1;
+            var version_combobox = new Gtk.ComboBoxText ();
+            version_combobox.append_text ("IPv4");
+            version_combobox.append_text ("IPv6");
+            version_combobox.append_text (_("Both"));
+            version_combobox.active = 0;
+
             var direction_label = new Gtk.Label (_("Direction:"));
             direction_label.xalign = 1;
             var direction_combobox = new Gtk.ComboBoxText ();
@@ -196,7 +370,14 @@
                 else
                     rule.action = UFWHelpers.Rule.Action.LIMIT;
 
-                rule.ports = ports_entry.text.replace ("-", ":");
+                if (version_combobox.active == 0)
+                    rule.version = UFWHelpers.Rule.Version.IPV4;
+                else if (version_combobox.active == 1)
+                    rule.version = UFWHelpers.Rule.Version.IPV6;
+                else
+                    rule.version = UFWHelpers.Rule.Version.BOTH;
+
+                rule.to_ports = ports_entry.text.replace ("-", ":");
                 UFWHelpers.add_rule (rule);
                 add_popover.hide ();
                 show_rules ();
@@ -210,11 +391,13 @@
             popover_grid.attach (policy_combobox, 1, 0, 1, 1);
             popover_grid.attach (protocol_label, 0, 1, 1, 1);
             popover_grid.attach (protocol_combobox, 1, 1, 1, 1);
-            popover_grid.attach (direction_label, 0, 2, 1, 1);
-            popover_grid.attach (direction_combobox, 1, 2, 1, 1);
-            popover_grid.attach (ports_label, 0, 3, 1, 1);
-            popover_grid.attach (ports_entry, 1, 3, 1, 1);
-            popover_grid.attach (add_button_grid, 0, 4, 2, 1);
+            popover_grid.attach (version_label, 0, 2, 1, 1);
+            popover_grid.attach (version_combobox, 1, 2, 1, 1);
+            popover_grid.attach (direction_label, 0, 3, 1, 1);
+            popover_grid.attach (direction_combobox, 1, 3, 1, 1);
+            popover_grid.attach (ports_label, 0, 4, 1, 1);
+            popover_grid.attach (ports_entry, 1, 4, 1, 1);
+            popover_grid.attach (add_button_grid, 0, 5, 2, 1);
 
             add_popover.show_all ();
         });
@@ -230,7 +413,15 @@
             list_store.get_iter (out iter, path);
             Value val;
             list_store.get_value (iter, Columns.RULE, out val);
-            UFWHelpers.remove_rule ((UFWHelpers.Rule) val.get_object ());
+            var rule = (UFWHelpers.Rule) val.get_object ();
+            string gen_hash = generate_hash_for_rule (rule);
+            Value active;
+            list_store.get_value (iter, Columns.ENABLED, out active);
+            if (active.get_boolean ()) {                
+                UFWHelpers.remove_rule (rule);
+            } else {
+                delete_disabled_rule (gen_hash);
+            }
             show_rules ();
         });
         list_toolbar.insert (remove_button, -1);