apmanager: add DHCP firewall rule per interface basis
With the recent update in permission_broker to allow firewall rules per
interface basis, update apmanager to dynamically request/release DHCP
port access when an AP service is started/terminated. And only request
the port access for the interface that AP service is running on.
BUG=chromium:450408
TEST=USE="asan clang" FEATURES=test emerge-$BOARD apmanager
Run security_Firewall test
Manual Test:
1. Use "iptables -S" command to verify no firewall rule is added
for port 67 when AP service is not started.
2. Start an AP service, verify firewall rule for port 67 is added
for the wifi interface (wlan0 for wolf device) and client can
connect to it with IP connectivity.
3. Stop the AP service, verify firewall rule for port 67 is deleted.
CQ-DEPEND=CL:252931
Change-Id: If7a5150d224ff1a5085b5e8032a162e8ca07c545
Reviewed-on: https://chromium-review.googlesource.com/252941
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Tested-by: Zeping Qiu <zqiu@chromium.org>
Commit-Queue: Zeping Qiu <zqiu@chromium.org>
diff --git a/firewall_manager.cc b/firewall_manager.cc
index d9f2fda..c26a95e 100644
--- a/firewall_manager.cc
+++ b/firewall_manager.cc
@@ -30,7 +30,7 @@
}
}
-void FirewallManager::Start(const scoped_refptr<dbus::Bus>& bus) {
+void FirewallManager::Init(const scoped_refptr<dbus::Bus>& bus) {
CHECK(!permission_broker_proxy_) << "Already started";
if (!SetupLifelinePipe()) {
@@ -59,6 +59,29 @@
base::Unretained(this)));
}
+void FirewallManager::RequestDHCPPortAccess(const std::string& interface) {
+ CHECK(permission_broker_proxy_) << "Proxy not initialized yet";
+ if (dhcp_access_interfaces_.find(interface) !=
+ dhcp_access_interfaces_.end()) {
+ LOG(ERROR) << "DHCP access already requested for interface: " << interface;
+ return;
+ }
+ RequestUdpPortAccess(interface, kDhcpServerPort);
+ dhcp_access_interfaces_.insert(interface);
+}
+
+void FirewallManager::ReleaseDHCPPortAccess(const std::string& interface) {
+ CHECK(permission_broker_proxy_) << "Proxy not initialized yet";
+ if (dhcp_access_interfaces_.find(interface) ==
+ dhcp_access_interfaces_.end()) {
+ LOG(ERROR) << "DHCP access has not been requested for interface: "
+ << interface;
+ return;
+ }
+ ReleaseUdpPortAccess(interface, kDhcpServerPort);
+ dhcp_access_interfaces_.erase(interface);
+}
+
bool FirewallManager::SetupLifelinePipe() {
if (lifeline_read_fd_ != kInvalidFd) {
LOG(ERROR) << "Lifeline pipe already created";
@@ -83,7 +106,7 @@
if (!service_available) {
return;
}
- AddFirewallRules();
+ RequestAllPortsAccess();
}
void FirewallManager::OnServiceNameChanged(const string& old_owner,
@@ -94,14 +117,18 @@
if (new_owner.empty()) {
return;
}
- AddFirewallRules();
+ RequestAllPortsAccess();
}
-void FirewallManager::AddFirewallRules() {
- AddUdpPortRule(kDhcpServerPort);
+void FirewallManager::RequestAllPortsAccess() {
+ // Request access to DHCP port for all specified interfaces.
+ for (const auto& dhcp_interface : dhcp_access_interfaces_) {
+ RequestUdpPortAccess(dhcp_interface, kDhcpServerPort);
+ }
}
-void FirewallManager::AddUdpPortRule(uint16_t port) {
+void FirewallManager::RequestUdpPortAccess(const string& interface,
+ uint16_t port) {
bool allowed = false;
// Pass the read end of the pipe to permission_broker, for it to monitor this
// process.
@@ -109,18 +136,42 @@
fd.CheckValidity();
chromeos::ErrorPtr error;
if (!permission_broker_proxy_->RequestUdpPortAccess(port,
- "", /* interface */
+ interface,
fd,
&allowed,
&error)) {
- LOG(ERROR) << "Failed request UDP port access: "
+ LOG(ERROR) << "Failed to request UDP port access: "
<< error->GetCode() << " " << error->GetMessage();
return;
}
if (!allowed) {
- LOG(ERROR) << "Access request for UDP port " << port << " is denied";
+ LOG(ERROR) << "Access request for UDP port " << port
+ << " on interface " << interface << " is denied";
+ return;
}
- LOG(INFO) << "Added UDP port " << port;
+ LOG(INFO) << "Access granted for UDP port " << port
+ << " on interface " << interface;;
+}
+
+void FirewallManager::ReleaseUdpPortAccess(const string& interface,
+ uint16_t port) {
+ chromeos::ErrorPtr error;
+ bool success;
+ if (!permission_broker_proxy_->ReleaseUdpPort(port,
+ interface,
+ &success,
+ &error)) {
+ LOG(ERROR) << "Failed to release UDP port access: "
+ << error->GetCode() << " " << error->GetMessage();
+ return;
+ }
+ if (!success) {
+ LOG(ERROR) << "Release request for UDP port " << port
+ << " on interface " << interface << " is denied";
+ return;
+ }
+ LOG(INFO) << "Access released for UDP port " << port
+ << " on interface " << interface;
}
} // namespace apmanager
diff --git a/firewall_manager.h b/firewall_manager.h
index 092162f..0f81332 100644
--- a/firewall_manager.h
+++ b/firewall_manager.h
@@ -5,6 +5,7 @@
#ifndef APMANAGER_FIREWALL_MANAGER_H_
#define APMANAGER_FIREWALL_MANAGER_H_
+#include <set>
#include <string>
#include <base/macros.h>
@@ -20,7 +21,11 @@
FirewallManager();
~FirewallManager();
- void Start(const scoped_refptr<dbus::Bus>& bus);
+ void Init(const scoped_refptr<dbus::Bus>& bus);
+
+ // Request/release DHCP port access for the specified interface.
+ void RequestDHCPPortAccess(const std::string& interface);
+ void ReleaseDHCPPortAccess(const std::string& interface);
private:
// Setup lifeline pipe to allow the remote firewall server
@@ -32,11 +37,16 @@
void OnServiceNameChanged(const std::string& old_owner,
const std::string& new_owner);
- // Add all required firewall rules for apmanager.
- void AddFirewallRules();
- void AddUdpPortRule(uint16_t port);
+ // This is called when a new instance of permission_broker is detected. Since
+ // the new instance doesn't have any knowledge of previously port access
+ // requests, re-issue those requests to permission_broker to get in sync.
+ void RequestAllPortsAccess();
- // DBus proxy for shill manager.
+ // Request/release UDP port access for the specified interface and port.
+ void RequestUdpPortAccess(const std::string& interface, uint16_t port);
+ void ReleaseUdpPortAccess(const std::string& interface, uint16_t port);
+
+ // DBus proxy for permission_broker.
std::unique_ptr<org::chromium::PermissionBrokerProxy>
permission_broker_proxy_;
// File descriptors for the two end of the pipe use for communicating with
@@ -45,6 +55,9 @@
int lifeline_read_fd_;
int lifeline_write_fd_;
+ // List of interfaces with DHCP port access.
+ std::set<std::string> dhcp_access_interfaces_;
+
DISALLOW_COPY_AND_ASSIGN(FirewallManager);
};
diff --git a/manager.cc b/manager.cc
index 70dc757..a177250 100644
--- a/manager.cc
+++ b/manager.cc
@@ -41,11 +41,8 @@
sequencer->GetHandler("Manager.RegisterAsync() failed.", true));
bus_ = bus;
- // Initialize shill proxy.
shill_proxy_.Init(bus);
-
- // Start firewall manager.
- firewall_manager_.Start(bus);
+ firewall_manager_.Init(bus);
}
void Manager::Start() {
@@ -131,6 +128,14 @@
shill_proxy_.ReleaseInterface(interface_name);
}
+void Manager::RequestDHCPPortAccess(const string& interface) {
+ firewall_manager_.RequestDHCPPortAccess(interface);
+}
+
+void Manager::ReleaseDHCPPortAccess(const string& interface) {
+ firewall_manager_.ReleaseDHCPPortAccess(interface);
+}
+
void Manager::OnServiceRegistered(
scoped_ptr<DBusMethodResponse<dbus::ObjectPath>> response,
scoped_ptr<Service> service,
diff --git a/manager.h b/manager.h
index 9f08e15..cc79b03 100644
--- a/manager.h
+++ b/manager.h
@@ -61,6 +61,10 @@
// Release the given interface |interface_name| to shill.
virtual void ReleaseInterface(const std::string& interface_name);
+ // Request/release access to DHCP port for the specified interface.
+ virtual void RequestDHCPPortAccess(const std::string& interface);
+ virtual void ReleaseDHCPPortAccess(const std::string& interface);
+
private:
friend class ManagerTest;
diff --git a/mock_manager.h b/mock_manager.h
index 08ae117..dbbc731 100644
--- a/mock_manager.h
+++ b/mock_manager.h
@@ -27,6 +27,8 @@
scoped_refptr<Device>(const std::string& interface_name));
MOCK_METHOD1(ClaimInterface, void(const std::string& interface_name));
MOCK_METHOD1(ReleaseInterface, void(const std::string& interface_name));
+ MOCK_METHOD1(RequestDHCPPortAccess, void(const std::string& interface));
+ MOCK_METHOD1(ReleaseDHCPPortAccess, void(const std::string& interface));
private:
DISALLOW_COPY_AND_ASSIGN(MockManager);
diff --git a/service.cc b/service.cc
index 3f33922..feffa6a 100644
--- a/service.cc
+++ b/service.cc
@@ -137,6 +137,7 @@
ReleaseResources();
return false;
}
+ manager_->RequestDHCPPortAccess(config_->selected_interface());
}
// Start monitoring hostapd.
@@ -196,6 +197,7 @@
StopHostapdProcess();
dhcp_server_.reset();
config_->ReleaseDevice();
+ manager_->ReleaseDHCPPortAccess(config_->selected_interface());
}
void Service::HostapdEventCallback(HostapdMonitor::Event event,
diff --git a/service_unittest.cc b/service_unittest.cc
index da58e91..303d645 100644
--- a/service_unittest.cc
+++ b/service_unittest.cc
@@ -40,11 +40,11 @@
class ServiceTest : public testing::Test {
public:
ServiceTest()
- : service_(&manager_, kServiceIdentifier),
- dhcp_server_factory_(MockDHCPServerFactory::GetInstance()),
+ : dhcp_server_factory_(MockDHCPServerFactory::GetInstance()),
file_writer_(MockFileWriter::GetInstance()),
process_factory_(MockProcessFactory::GetInstance()),
- hostapd_monitor_(new MockHostapdMonitor()) {}
+ hostapd_monitor_(new MockHostapdMonitor()),
+ service_(&manager_, kServiceIdentifier) {}
virtual void SetUp() {
service_.dhcp_server_factory_ = dhcp_server_factory_;
@@ -66,12 +66,12 @@
}
protected:
- Service service_;
MockManager manager_;
MockDHCPServerFactory* dhcp_server_factory_;
MockFileWriter* file_writer_;
MockProcessFactory* process_factory_;
MockHostapdMonitor* hostapd_monitor_;
+ Service service_;
};
MATCHER_P(IsServiceErrorStartingWith, message, "") {
@@ -121,6 +121,7 @@
EXPECT_CALL(*dhcp_server_factory_, CreateDHCPServer(_, _))
.WillOnce(Return(dhcp_server));
EXPECT_CALL(*dhcp_server, Start()).WillOnce(Return(true));
+ EXPECT_CALL(manager_, RequestDHCPPortAccess(_));
EXPECT_CALL(*hostapd_monitor_, Start());
EXPECT_TRUE(service_.Start(&error));
EXPECT_EQ(nullptr, error);