[Merge] lp:~thomas-voss/location-service/fix-1219164 into lp:location-service
Seth Arnold
seth.arnold at canonical.com
Sat Aug 2 02:23:44 UTC 2014
Review: Needs Information
The AppArmor policies are being looked up by pid which can be a racy interface. Do the races matter to us? Will something else in the system prevent the following chain of events?
A process with pid 4242 running with AppArmor profile Foo makes a location request
A process dies from some event
B process with any pid spawns children until one has pid 4242
C process with pid 4242 running with AppArmor profile Bar receives permission to use location from previous request
It seems fairly unlikely, I'll admit, but if an attacker can chew up enough CPU time, some race conditions can become arbitrarily easy to exploit.
Thanks
Diff comments:
> === modified file 'CMakeLists.txt'
> --- CMakeLists.txt 2014-07-29 09:41:24 +0000
> +++ CMakeLists.txt 2014-08-01 15:59:41 +0000
> @@ -6,6 +6,8 @@
> set(UBUNTU_LOCATION_SERVICE_VERSION_MINOR 0)
> set(UBUNTU_LOCATION_SERVICE_VERSION_PATCH 0)
>
> +set(UBUNTU_LOCATION_SERVICE_TRUST_STORE_SERVICE_NAME "UbuntuLocationService")
> +
> set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -pedantic -Wextra -fPIC")
> set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall -fno-strict-aliasing -pedantic -Wextra -fPIC")
>
> @@ -27,10 +29,11 @@
> pkg_check_modules(DBUS dbus-1 REQUIRED)
> pkg_check_modules(DBUS_CPP dbus-cpp REQUIRED)
> pkg_check_modules(JSON_C json-c REQUIRED)
> -# TODO(tvoss): Re-enable once net-cpp hits the archive.
> +pkg_check_modules(LIBAPPARMOR libapparmor REQUIRED)
> pkg_check_modules(NET_CPP net-cpp REQUIRED)
> pkg_check_modules(PROCESS_CPP process-cpp REQUIRED)
> pkg_check_modules(PROPERTIES_CPP properties-cpp REQUIRED)
> +pkg_check_modules(TRUST_STORE trust-store REQUIRED)
> #####################################################################
> # Enable code coverage calculation with gcov/gcovr/lcov
> # Usage:
> @@ -56,14 +59,17 @@
> ${DBUS_INCLUDE_DIRS}
> ${DBUS_CPP_INCLUDE_DIRS}
> ${JSON_C_INCLUDE_DIRS}
> + ${LIBAPPARMOR_INCLUDE_DIRS}
> ${NET_CPP_INCLUDE_DIRS}
> ${PROPERTIES_CPP_INCLUDE_DIRS}
> ${PROCESS_CPP_INCLUDE_DIRS}
> + ${TRUST_STORE_INCLUDE_DIRS}
> ${GLog_INCLUDE_DIR}
> ${GFlags_INCLUDE_DIR}
>
> ${CMAKE_SOURCE_DIR}/include/location_service
> ${CMAKE_SOURCE_DIR}/src/location_service
> + ${CMAKE_BINARY_DIR}/src/location_service
> )
>
> file(GLOB_RECURSE UBUNTU_LOCATION_SERVICE_PUBLIC_HEADERS ${CMAKE_CURRENT_SOURCE_DIR} *.h)
>
> === modified file 'data/CMakeLists.txt'
> --- data/CMakeLists.txt 2014-05-19 18:00:57 +0000
> +++ data/CMakeLists.txt 2014-08-01 15:59:41 +0000
> @@ -26,6 +26,10 @@
> ubuntu-location-service.conf.in ubuntu-location-service.conf @ONLY
> )
>
> +configure_file(
> + ubuntu-location-service-trust-stored.conf.in ubuntu-location-service-trust-stored.conf @ONLY
> +)
> +
> install(
> FILES ${CMAKE_CURRENT_BINARY_DIR}/ubuntu-location-service.pc
> DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig
> @@ -45,3 +49,8 @@
> FILES ${CMAKE_CURRENT_BINARY_DIR}/ubuntu-location-service.conf
> DESTINATION /etc/init/
> )
> +
> +install(
> + FILES ${CMAKE_CURRENT_BINARY_DIR}/ubuntu-location-service-trust-stored.conf
> + DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/upstart/sessions/
> +)
>
> === modified file 'data/com.ubuntu.location.Service.conf'
> --- data/com.ubuntu.location.Service.conf 2013-05-28 14:20:45 +0000
> +++ data/com.ubuntu.location.Service.conf 2014-08-01 15:59:41 +0000
> @@ -8,13 +8,17 @@
> <allow send_destination="com.ubuntu.location.Service.Session"/>
> <allow send_interface="com.ubuntu.location.Service"/>
> <allow send_interface="com.ubuntu.location.Service.Session"/>
> + <allow own="core.trust.dbus.Agent.UbuntuLocationService"/>
> + <allow send_interface="core.trust.dbus.Agent"/>
> + <allow send_interface="core.trust.dbus.AgentRegistry"/>
> </policy>
> <policy context="default">
> - <deny own="com.ubuntu.location.Service"/>
> + <deny own="com.ubuntu.location.Service"/>
> <allow own="com.ubuntu.location.Service.Session"/>
> <allow send_destination="com.ubuntu.location.Service"/>
> <allow send_destination="com.ubuntu.location.Service.Session"/>
> <allow send_interface="com.ubuntu.location.Service"/>
> <allow send_interface="com.ubuntu.location.Service.Session"/>
> + <allow send_interface="core.trust.dbus.AgentRegistry"/>
> </policy>
> </busconfig>
>
> === added file 'data/ubuntu-location-service-trust-stored.conf.in'
> --- data/ubuntu-location-service-trust-stored.conf.in 1970-01-01 00:00:00 +0000
> +++ data/ubuntu-location-service-trust-stored.conf.in 2014-08-01 15:59:41 +0000
> @@ -0,0 +1,12 @@
> +description "Location Services Trust Store Daemon"
> +
> +start on started dbus
> +
> +respawn
> +
> +exec /usr/bin/trust-stored-skeleton \
> + --remote-agent DBusRemoteAgent --bus=system \
> + --local-agent MirAgent \
> + --trusted-mir-socket=/var/run/user/$(id -u)/mir_socket_trusted \
> + --for-service @UBUNTU_LOCATION_SERVICE_TRUST_STORE_SERVICE_NAME@ \
> + --store-bus session
>
> === modified file 'debian/control'
> --- debian/control 2014-07-30 14:16:29 +0000
> +++ debian/control 2014-08-01 15:59:41 +0000
> @@ -15,6 +15,7 @@
> # in libstdc++ causing us issues, we explicitly select a G++
> # version.
> g++-4.9,
> + libapparmor-dev,
> libboost-filesystem-dev,
> libboost-program-options-dev,
> libboost-system-dev,
> @@ -26,6 +27,7 @@
> libjson-c-dev,
> libnet-cpp-dev,
> libprocess-cpp-dev,
> + libtrust-store-dev,
> libubuntu-platform-hardware-api-headers [!arm64 !ppc64el !powerpc],
> libubuntu-platform-hardware-api-dev [!arm64 !ppc64el !powerpc],
> libproperties-cpp-dev,
>
> === modified file 'debian/ubuntu-location-service-bin.install'
> --- debian/ubuntu-location-service-bin.install 2013-12-16 12:37:07 +0000
> +++ debian/ubuntu-location-service-bin.install 2014-08-01 15:59:41 +0000
> @@ -1,4 +1,5 @@
> etc/dbus-1/system.d/
> etc/init/*
> +usr/share/upstart/sessions/ubuntu-location-service-trust-stored.conf
> usr/bin/ubuntu-location-serviced
> usr/bin/ubuntu-location-serviced-cli
>
> === modified file 'examples/service/service.cpp'
> --- examples/service/service.cpp 2014-06-23 04:57:59 +0000
> +++ examples/service/service.cpp 2014-08-01 15:59:41 +0000
> @@ -165,7 +165,7 @@
> incoming,
> outgoing,
> config.the_engine(instantiated_providers, config.the_provider_selection_policy()),
> - config.the_permission_manager(),
> + config.the_permission_manager(incoming),
> culs::Harvester::Configuration
> {
> cul::connectivity::platform_default_manager(),
>
> === modified file 'include/location_service/com/ubuntu/location/service/interface.h'
> --- include/location_service/com/ubuntu/location/service/interface.h 2014-02-03 13:52:21 +0000
> +++ include/location_service/com/ubuntu/location/service/interface.h 2014-08-01 15:59:41 +0000
> @@ -81,7 +81,7 @@
>
> inline static const std::chrono::milliseconds default_timeout()
> {
> - return std::chrono::seconds{1};
> + return std::chrono::seconds{25};
> }
> };
>
>
> === modified file 'src/location_service/com/ubuntu/location/CMakeLists.txt'
> --- src/location_service/com/ubuntu/location/CMakeLists.txt 2014-07-31 10:18:42 +0000
> +++ src/location_service/com/ubuntu/location/CMakeLists.txt 2014-08-01 15:59:41 +0000
> @@ -6,6 +6,10 @@
>
> add_subdirectory(providers)
>
> +configure_file(
> + service/config.h.in ${CMAKE_CURRENT_BINARY_DIR}/service/config.h @ONLY
> +)
> +
> add_library(
> ubuntu-location-service SHARED
>
> @@ -25,6 +29,7 @@
> service/default_configuration.cpp
> service/default_permission_manager.cpp
> service/harvester.cpp
> + service/trust_store_permission_manager.cpp
>
> service/implementation.cpp
> service/skeleton.cpp
> @@ -99,7 +104,9 @@
> ${DBUS_LIBRARIES}
> ${DBUS_CPP_LDFLAGS}
> ${JSON_C_LDFLAGS}
> + ${LIBAPPARMOR_LDFLAGS}
> ${NET_CPP_LDFLAGS}
> + ${TRUST_STORE_LDFLAGS}
> ${GLog_LIBRARY}
> ${GFlags_LIBRARY}
> )
>
> === added file 'src/location_service/com/ubuntu/location/service/config.h.in'
> --- src/location_service/com/ubuntu/location/service/config.h.in 1970-01-01 00:00:00 +0000
> +++ src/location_service/com/ubuntu/location/service/config.h.in 2014-08-01 15:59:41 +0000
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright © 2012-2013 Canonical Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 3,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Thomas Voß <thomas.voss at canonical.com>
> + */
> +#ifndef LOCATION_SERVICE_COM_UBUNTU_LOCATION_SERVICE_CONFIG_H_
> +#define LOCATION_SERVICE_COM_UBUNTU_LOCATION_SERVICE_CONFIG_H_
> +
> +namespace com
> +{
> +namespace ubuntu
> +{
> +namespace location
> +{
> +namespace service
> +{
> +static constexpr const char* trust_store_service_name
> +{
> + "@UBUNTU_LOCATION_SERVICE_TRUST_STORE_SERVICE_NAME@"
> +};
> +}
> +}
> +}
> +}
> +
> +#endif // LOCATION_SERVICE_COM_UBUNTU_LOCATION_SERVICE_CONFIG_H_
>
> === modified file 'src/location_service/com/ubuntu/location/service/daemon.cpp'
> --- src/location_service/com/ubuntu/location/service/daemon.cpp 2014-07-29 10:51:48 +0000
> +++ src/location_service/com/ubuntu/location/service/daemon.cpp 2014-08-01 15:59:41 +0000
> @@ -192,12 +192,13 @@
> config.outgoing->install_executor(dbus::asio::make_executor(config.outgoing));
>
> location::service::DefaultConfiguration dc;
> +
> location::service::Implementation::Configuration configuration
> {
> config.incoming,
> config.outgoing,
> dc.the_engine(instantiated_providers, dc.the_provider_selection_policy()),
> - dc.the_permission_manager(),
> + dc.the_permission_manager(config.outgoing),
> location::service::Harvester::Configuration
> {
> location::connectivity::platform_default_manager(),
> @@ -211,7 +212,9 @@
> };
>
> std::thread t1{[&config](){config.incoming->run();}};
> - std::thread t2{[&config](){config.outgoing->run();}};
> + std::thread t2{[&config](){config.incoming->run();}};
> + std::thread t3{[&config](){config.incoming->run();}};
> + std::thread t4{[&config](){config.outgoing->run();}};
>
> trap->run();
>
> @@ -224,6 +227,12 @@
> if (t2.joinable())
> t2.join();
>
> + if (t3.joinable())
> + t3.join();
> +
> + if (t4.joinable())
> + t4.join();
> +
> return EXIT_SUCCESS;
> }
>
>
> === modified file 'src/location_service/com/ubuntu/location/service/default_configuration.cpp'
> --- src/location_service/com/ubuntu/location/service/default_configuration.cpp 2013-12-14 09:10:29 +0000
> +++ src/location_service/com/ubuntu/location/service/default_configuration.cpp 2014-08-01 15:59:41 +0000
> @@ -17,20 +17,13 @@
> */
> #include <com/ubuntu/location/service/default_configuration.h>
> #include <com/ubuntu/location/service/default_permission_manager.h>
> +#include <com/ubuntu/location/service/trust_store_permission_manager.h>
>
> #include <com/ubuntu/location/default_provider_selection_policy.h>
>
> namespace cul = com::ubuntu::location;
> namespace culs = com::ubuntu::location::service;
>
> -culs::DefaultConfiguration::DefaultConfiguration()
> -{
> -}
> -
> -culs::DefaultConfiguration::~DefaultConfiguration() noexcept
> -{
> -}
> -
> cul::Engine::Ptr culs::DefaultConfiguration::the_engine(
> const std::set<cul::Provider::Ptr>& provider_set,
> const cul::ProviderSelectionPolicy::Ptr& provider_selection_policy)
> @@ -53,8 +46,8 @@
> return std::set<cul::Provider::Ptr>{seed};
> }
>
> -culs::PermissionManager::Ptr culs::DefaultConfiguration::the_permission_manager()
> +culs::PermissionManager::Ptr culs::DefaultConfiguration::the_permission_manager(const core::dbus::Bus::Ptr& bus)
> {
> - return DefaultPermissionManager::Ptr(new DefaultPermissionManager());
> + return culs::TrustStorePermissionManager::create_default_instance_with_bus(bus);
> }
>
>
> === renamed file 'include/location_service/com/ubuntu/location/service/default_configuration.h' => 'src/location_service/com/ubuntu/location/service/default_configuration.h'
> --- include/location_service/com/ubuntu/location/service/default_configuration.h 2013-12-10 09:42:54 +0000
> +++ src/location_service/com/ubuntu/location/service/default_configuration.h 2014-08-01 15:59:41 +0000
> @@ -22,6 +22,8 @@
>
> #include <set>
>
> +#include <core/dbus/bus.h>
> +
> namespace com
> {
> namespace ubuntu
> @@ -30,24 +32,27 @@
> {
> namespace service
> {
> +// A helper class to create dependencies of the service implementation.
> class DefaultConfiguration
> {
> public:
> - DefaultConfiguration();
> - DefaultConfiguration(const DefaultConfiguration&) = delete;
> - DefaultConfiguration& operator=(const DefaultConfiguration&) = delete;
> - ~DefaultConfiguration() noexcept;
> -
> + DefaultConfiguration() = default;
> + virtual ~DefaultConfiguration() = default;
> +
> + // Creates a policy instance for selecting a set of providers given a criteria
> + // as specified by a client application.
> + virtual ProviderSelectionPolicy::Ptr the_provider_selection_policy();
> +
> + // Returns a set of providers, seeded with the seed provider if it is not null.
> + virtual std::set<Provider::Ptr> the_provider_set(const Provider::Ptr& seed = Provider::Ptr {});
> +
> + // Creates an engine instance given a set of providers and a provider selection policy.
> virtual Engine::Ptr the_engine(
> const std::set<Provider::Ptr>& provider_set,
> - const ProviderSelectionPolicy::Ptr& provider_selection_policy);
> -
> - ProviderSelectionPolicy::Ptr the_provider_selection_policy();
> -
> - std::set<Provider::Ptr> the_provider_set(
> - const Provider::Ptr& seed = Provider::Ptr {});
> -
> - PermissionManager::Ptr the_permission_manager();
> + const ProviderSelectionPolicy::Ptr& provider_selection_policy);
> +
> + // Instantiates an instance of the permission manager.
> + virtual PermissionManager::Ptr the_permission_manager(const std::shared_ptr<core::dbus::Bus>& bus);
> };
> }
> }
>
> === added file 'src/location_service/com/ubuntu/location/service/trust_store_permission_manager.cpp'
> --- src/location_service/com/ubuntu/location/service/trust_store_permission_manager.cpp 1970-01-01 00:00:00 +0000
> +++ src/location_service/com/ubuntu/location/service/trust_store_permission_manager.cpp 2014-08-01 15:59:41 +0000
> @@ -0,0 +1,176 @@
> +/*
> + * Copyright © 2012-2013 Canonical Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 3,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Thomas Voß <thomas.voss at canonical.com>
> + */
> +
> +#include <com/ubuntu/location/service/trust_store_permission_manager.h>
> +
> +#include <com/ubuntu/location/logging.h>
> +#include <com/ubuntu/location/service/config.h>
> +
> +#include <core/trust/dbus_agent.h>
> +
> +#include <core/dbus/bus.h>
> +#include <core/dbus/asio/executor.h>
> +
> +#include <core/posix/this_process.h>
> +
> +#include <sys/apparmor.h>
> +
> +namespace location = com::ubuntu::location;
> +namespace service = com::ubuntu::location::service;
> +
> +namespace
> +{
> +bool is_running_under_testing()
> +{
> + return core::posix::this_process::env::get(
> + "TRUST_STORE_PERMISSION_MANAGER_IS_RUNNING_UNDER_TESTING",
> + "0") == "1";
> +
> +}
> +}
> +
> +service::TrustStorePermissionManager::AppArmorProfileResolver service::TrustStorePermissionManager::libapparmor_profile_resolver()
> +{
> + return [](core::trust::Pid pid)
> + {
> + static const int app_armor_error{-1};
> +
> + // We make sure to clean up the returned string.
> + struct Scope
> + {
> + ~Scope()
> + {
> + if (con) ::free(con);
free(NULL) is fine in POSIX / C -- I'm not sure about C++, but I hope there's nothing wrong with it all the same.
> + }
> +
> + char* con{nullptr};
> + char* mode{nullptr};
> + } scope;
> +
> + // Reach out to apparmor
> + auto rc = aa_gettaskcon(pid.value, &scope.con, &scope.mode);
Now that I see the API in action I have concerns: pids are a renewable resource, and they renew relatively quickly. I would much rather see aa_getpeercon() used here if we can use a Unix domain socket or org.freedesktop.DBus.GetConnectionAppArmorSecurityContext() used if we are using a DBus connection. (This DBus method requires a patched DBus daemon, which should be fine for our needs, but I figured I should point that out all the same.)
> +
> + // From man aa_gettaskcon:
> + // On success size of data placed in the buffer is returned, this includes the mode if
> + //present and any terminating characters. On error, -1 is returned, and errno(3) is
> + //set appropriately.
> + if (rc == app_armor_error) throw std::system_error
> + {
> + errno,
> + std::system_category()
> + };
> +
> + // Safely construct the string
> + return std::string
> + {
> + scope.con ? scope.con : ""
> + };
> + };
> +}
> +
> +core::trust::Feature service::TrustStorePermissionManager::default_feature()
> +{
> + return core::trust::Feature{0};
> +}
> +
> +service::TrustStorePermissionManager::Ptr service::TrustStorePermissionManager::create_default_instance_with_bus(const std::shared_ptr<core::dbus::Bus>& bus)
> +{
> + return Ptr
> + {
> + new TrustStorePermissionManager
> + {
> + core::trust::dbus::create_multi_user_agent_for_bus_connection(
> + bus,
> + com::ubuntu::location::service::trust_store_service_name),
> + TrustStorePermissionManager::libapparmor_profile_resolver()
> + }
> + };
> +}
> +
> +service::TrustStorePermissionManager::TrustStorePermissionManager(
> + const std::shared_ptr<core::trust::Agent>& agent,
> + service::TrustStorePermissionManager::AppArmorProfileResolver app_armor_profile_resolver)
> + : agent{agent},
> + app_armor_profile_resolver{app_armor_profile_resolver}
> +{
> +}
> +
> +service::PermissionManager::Result service::TrustStorePermissionManager::check_permission_for_credentials(
> + const location::Criteria&,
> + const service::Credentials& credentials)
> +{
> + // This is ugly and we should get rid of it. Ideally, we would be able
> + // inject a mocked trust-store into our acceptance testing.
> + if (is_running_under_testing())
> + return Result::granted;
> +
> + std::string profile;
> + try
> + {
> + profile = app_armor_profile_resolver(core::trust::Pid{credentials.pid});
> + } catch(const std::exception& e)
> + {
> + LOG(ERROR) << "Could not resolve PID " << credentials.pid << " to apparmor profile: " << e.what();
> + return service::PermissionManager::Result::rejected;
> + } catch(...)
> + {
> + LOG(ERROR) << "Could not resolve PID " << credentials.pid << " to apparmor profile.";
> + return service::PermissionManager::Result::rejected;
> + }
> +
> + std::string description;
> +
> + if (profile == "unconfined")
> + {
> + description = "An unconfined application is trying to access the location service.";
> + } else
> + {
> + description = profile + " is trying to access the location service.";
> + }
> +
> + core::trust::Agent::RequestParameters params
> + {
> + core::trust::Uid{credentials.uid},
> + core::trust::Pid{credentials.pid},
> + profile,
> + TrustStorePermissionManager::default_feature(),
> + description
> + };
> +
> + Result result{Result::rejected};
> +
> + try
> + {
> + auto answer = agent->authenticate_request_with_parameters(params);
> + switch(answer)
> + {
> + case core::trust::Request::Answer::granted:
> + result = Result::granted;
> + break;
> + case core::trust::Request::Answer::denied:
> + result = Result::rejected;
> + break;
> + }
> + } catch(...)
> + {
> + // We silently drop all issues here and return rejected.
> + result = Result::rejected;
> + }
> +
> + return result;
> +}
>
> === added file 'src/location_service/com/ubuntu/location/service/trust_store_permission_manager.h'
> --- src/location_service/com/ubuntu/location/service/trust_store_permission_manager.h 1970-01-01 00:00:00 +0000
> +++ src/location_service/com/ubuntu/location/service/trust_store_permission_manager.h 2014-08-01 15:59:41 +0000
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright © 2012-2013 Canonical Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 3,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Thomas Voß <thomas.voss at canonical.com>
> + */
> +#ifndef LOCATION_SERVICE_COM_UBUNTU_LOCATION_SERVICE_TRUST_STORE_PERMISSION_MANAGER_H_
> +#define LOCATION_SERVICE_COM_UBUNTU_LOCATION_SERVICE_TRUST_STORE_PERMISSION_MANAGER_H_
> +
> +#include <com/ubuntu/location/service/permission_manager.h>
> +
> +#include <core/trust/agent.h>
> +
> +namespace core
> +{
> +namespace dbus
> +{
> +class Bus;
> +}
> +}
> +
> +namespace com
> +{
> +namespace ubuntu
> +{
> +namespace location
> +{
> +namespace service
> +{
> +// A PermmissionManager implementation leveraging the trust-store
> +// infrastructure to cache a user's answer and to dispatch
> +// to user-specific trust-store instances.
> +class TrustStorePermissionManager : public PermissionManager
> +{
> +public:
> + // Just a convenience typedef.
> + typedef std::shared_ptr<TrustStorePermissionManager> Ptr;
> +
> + // Functor for resolving a process id to an app-armor profile name.
> + typedef std::function<std::string(const core::trust::Pid&)> AppArmorProfileResolver;
> +
> + // Returns an AppArmorProfileResolver leveraging libapparmor.
> + static AppArmorProfileResolver libapparmor_profile_resolver();
> +
> + // The default feature tag we use when calling out to the agent.
> + static core::trust::Feature default_feature();
> +
> + // Creates a default instance, initializing the agent and resolver
> + // fields to sensible default choices.
> + static Ptr create_default_instance_with_bus(const std::shared_ptr<core::dbus::Bus>& bus);
> +
> + // Sets up the manager for operation and stores the agent and resolver
> + // instances given to the ctor.
> + TrustStorePermissionManager(
> + const std::shared_ptr<core::trust::Agent>& agent,
> + AppArmorProfileResolver app_armor_profile_resolver);
> +
> + // From PermissionManager
> + Result check_permission_for_credentials(const Criteria&, const Credentials& credentials) override;
> +
> +private:
> + // The agent instance we leverage to authenticate
> + // permission requests.
> + std::shared_ptr<core::trust::Agent> agent;
> +
> + // Helper to resolve an application's pid to an app-armor profile name.
> + AppArmorProfileResolver app_armor_profile_resolver;
> +};
> +}
> +}
> +}
> +}
> +
> +#endif // LOCATION_SERVICE_COM_UBUNTU_LOCATION_SERVICE_TRUST_STORE_PERMISSION_MANAGER_H_
>
> === modified file 'tests/CMakeLists.txt'
> --- tests/CMakeLists.txt 2014-07-11 12:56:40 +0000
> +++ tests/CMakeLists.txt 2014-08-01 15:59:41 +0000
> @@ -87,6 +87,7 @@
> LOCATION_SERVICE_ADD_TEST(provider_factory_test provider_factory_test.cpp)
> LOCATION_SERVICE_ADD_TEST(provider_test provider_test.cpp)
> LOCATION_SERVICE_ADD_TEST(wgs84_test wgs84_test.cpp)
> +LOCATION_SERVICE_ADD_TEST(trust_store_permission_manager_test trust_store_permission_manager_test.cpp)
>
> # Provider-specific test-cases go here.
> if (LOCATION_SERVICE_ENABLE_GPS_PROVIDER)
>
> === modified file 'tests/acceptance_tests.cpp'
> --- tests/acceptance_tests.cpp 2014-06-23 04:57:59 +0000
> +++ tests/acceptance_tests.cpp 2014-08-01 15:59:41 +0000
> @@ -64,6 +64,15 @@
>
> namespace
> {
> +
> +bool setup_trust_store_permission_manager_for_testing()
> +{
> + core::posix::this_process::env::set_or_throw("TRUST_STORE_PERMISSION_MANAGER_IS_RUNNING_UNDER_TESTING", "1");
> + return true;
> +}
> +
> +static const bool trust_store_is_set_up_for_testing = setup_trust_store_permission_manager_for_testing();
> +
> struct NullReporter : public culs::Harvester::Reporter
> {
> NullReporter() = default;
> @@ -168,6 +177,8 @@
>
> TEST_F(LocationServiceStandalone, SessionsReceiveUpdatesViaDBus)
> {
> + EXPECT_TRUE(trust_store_is_set_up_for_testing);
> +
> core::testing::CrossProcessSync sync_start;
> core::testing::CrossProcessSync sync_session_created;
>
> @@ -199,7 +210,7 @@
> incoming,
> outgoing,
> config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy()),
> - config.the_permission_manager(),
> + config.the_permission_manager(incoming),
> cul::service::Harvester::Configuration
> {
> cul::connectivity::platform_default_manager(),
> @@ -293,6 +304,8 @@
>
> TEST_F(LocationServiceStandalone, EngineStatusCanBeQueriedAndAdjusted)
> {
> + EXPECT_TRUE(trust_store_is_set_up_for_testing);
> +
> core::testing::CrossProcessSync sync_start;
>
> auto server = [this, &sync_start]()
> @@ -319,7 +332,7 @@
> incoming,
> outgoing,
> config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy()),
> - config.the_permission_manager(),
> + config.the_permission_manager(incoming),
> cul::service::Harvester::Configuration
> {
> cul::connectivity::platform_default_manager(),
> @@ -371,6 +384,8 @@
>
> TEST_F(LocationServiceStandalone, SatellitePositioningStatusCanBeQueriedAndAdjusted)
> {
> + EXPECT_TRUE(trust_store_is_set_up_for_testing);
> +
> core::testing::CrossProcessSync sync_start;
>
> auto server = [this, &sync_start]()
> @@ -398,7 +413,7 @@
> incoming,
> outgoing,
> config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy()),
> - config.the_permission_manager(),
> + config.the_permission_manager(incoming),
> cul::service::Harvester::Configuration
> {
> cul::connectivity::platform_default_manager(),
> @@ -450,6 +465,8 @@
>
> TEST_F(LocationServiceStandalone, WifiAndCellIdReportingStateCanBeQueriedAndAjdusted)
> {
> + EXPECT_TRUE(trust_store_is_set_up_for_testing);
> +
> core::testing::CrossProcessSync sync_start;
>
> auto server = [this, &sync_start]()
> @@ -476,7 +493,7 @@
> incoming,
> outgoing,
> config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy()),
> - config.the_permission_manager(),
> + config.the_permission_manager(incoming),
> cul::service::Harvester::Configuration
> {
> cul::connectivity::platform_default_manager(),
> @@ -527,6 +544,8 @@
>
> TEST_F(LocationServiceStandalone, VisibleSpaceVehiclesCanBeQueried)
> {
> + EXPECT_TRUE(trust_store_is_set_up_for_testing);
> +
> core::testing::CrossProcessSync sync_start;
>
> cul::SpaceVehicle sv;
> @@ -562,7 +581,7 @@
> incoming,
> outgoing,
> config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy()),
> - config.the_permission_manager(),
> + config.the_permission_manager(incoming),
> cul::service::Harvester::Configuration
> {
> cul::connectivity::platform_default_manager(),
> @@ -713,6 +732,8 @@
>
> TEST_F(LocationServiceStandaloneLoad, MultipleClientsConnectingAndDisconnectingWorks)
> {
> + EXPECT_TRUE(trust_store_is_set_up_for_testing);
> +
> options.print(LOG(INFO));
>
> auto server = core::posix::fork([this]()
>
> === added file 'tests/app_armor_testing_profile'
> --- tests/app_armor_testing_profile 1970-01-01 00:00:00 +0000
> +++ tests/app_armor_testing_profile 2014-08-01 15:59:41 +0000
> @@ -0,0 +1,3 @@
> +profile an_empty_profile_for_testing_purposes
> +{
> +}
> \ No newline at end of file
>
> === added file 'tests/trust_store_permission_manager_test.cpp'
> --- tests/trust_store_permission_manager_test.cpp 1970-01-01 00:00:00 +0000
> +++ tests/trust_store_permission_manager_test.cpp 2014-08-01 15:59:41 +0000
> @@ -0,0 +1,204 @@
> +/*
> + * Copyright © 2012-2013 Canonical Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 3,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Thomas Voß <thomas.voss at canonical.com>
> + */
> +
> +#include <com/ubuntu/location/service/trust_store_permission_manager.h>
> +
> +#include <com/ubuntu/location/criteria.h>
> +
> +#include <core/posix/fork.h>
> +#include <core/testing/cross_process_sync.h>
> +
> +#include <gmock/gmock.h>
> +
> +#include <thread>
> +
> +#include <sys/apparmor.h>
> +
> +namespace location = com::ubuntu::location;
> +namespace service = com::ubuntu::location::service;
> +
> +namespace
> +{
> +struct MockAgent : public core::trust::Agent
> +{
> + /**
> + * @brief Presents the given request to the user, returning the user-provided answer.
> + * @param request The trust request that a user has to answer.
> + * @param description Extended description of the trust request.
> + */
> + MOCK_METHOD1(authenticate_request_with_parameters, core::trust::Request::Answer(const core::trust::Agent::RequestParameters&));
> +};
> +
> +struct MockAppArmorProfileResolver
> +{
> + MOCK_METHOD1(resolve_pid_to_app_armor_profile, std::string(const core::trust::Pid&));
> +
> + service::TrustStorePermissionManager::AppArmorProfileResolver to_functional()
> + {
> + return [this](const core::trust::Pid& pid)
> + {
> + return resolve_pid_to_app_armor_profile(pid);
> + };
> + }
> +};
> +
> +location::Criteria default_criteria;
> +}
> +
> +TEST(TrustStorePermissionManager, calls_out_to_agent)
> +{
> + using namespace ::testing;
> +
> + auto mock_agent = std::make_shared<MockAgent>();
> +
> + EXPECT_CALL(*mock_agent, authenticate_request_with_parameters(_))
> + .Times(1)
> + .WillRepeatedly(Return(core::trust::Request::Answer::denied));
> +
> + service::TrustStorePermissionManager pm
> + {
> + mock_agent,
> + service::TrustStorePermissionManager::libapparmor_profile_resolver()
> + };
> +
> + EXPECT_EQ(service::PermissionManager::Result::rejected,
> + pm.check_permission_for_credentials(
> + default_criteria,
> + location::service::Credentials{::getpid(), ::getuid()}));
> +}
> +
> +TEST(TrustStorePermissionManager, returns_rejected_for_throwing_agent)
> +{
> + using namespace ::testing;
> +
> + auto mock_agent = std::make_shared<MockAgent>();
> +
> + EXPECT_CALL(*mock_agent, authenticate_request_with_parameters(_))
> + .Times(1)
> + .WillRepeatedly(Throw(std::runtime_error{"Thrown from mock agent"}));
> +
> + service::TrustStorePermissionManager pm{mock_agent, service::TrustStorePermissionManager::libapparmor_profile_resolver()};
> +
> + EXPECT_EQ(service::PermissionManager::Result::rejected,
> + pm.check_permission_for_credentials(default_criteria, location::service::Credentials{::getpid(), ::getuid()}));
> +}
> +
> +TEST(TrustStorePermissionManager, resolves_app_id)
> +{
> + using namespace ::testing;
> +
> + const pid_t pid = ::getpid();
> + const uid_t uid = ::getuid();
> +
> + auto mock_agent = std::make_shared<MockAgent>();
> +
> + EXPECT_CALL(*mock_agent, authenticate_request_with_parameters(_))
> + .Times(1)
> + .WillRepeatedly(Return(core::trust::Request::Answer::denied));
> +
> + MockAppArmorProfileResolver resolver;
> + EXPECT_CALL(resolver, resolve_pid_to_app_armor_profile(core::trust::Pid{pid}))
> + .Times(1)
> + .WillRepeatedly(Return(std::string{"does.not.exist"}));
> +
> + service::TrustStorePermissionManager pm
> + {
> + mock_agent,
> + resolver.to_functional()
> + };
> +
> + EXPECT_EQ(service::PermissionManager::Result::rejected,
> + pm.check_permission_for_credentials(default_criteria, location::service::Credentials{pid, uid}));
> +}
> +
> +TEST(TrustStorePermissionManager, returns_rejected_for_throwing_app_id_resolver)
> +{
> + using namespace ::testing;
> +
> + const pid_t pid = ::getpid();
> + const uid_t uid = ::getuid();
> +
> + auto mock_agent = std::make_shared<MockAgent>();
> +
> + EXPECT_CALL(*mock_agent, authenticate_request_with_parameters(_))
> + .Times(0); // This should never be called if we cannot resolve an apparmor profile.
> +
> + MockAppArmorProfileResolver resolver;
> + EXPECT_CALL(resolver, resolve_pid_to_app_armor_profile(core::trust::Pid{pid}))
> + .Times(1)
> + .WillRepeatedly(Throw(std::runtime_error{"Thrown from MockAppArmorProfileResolver"}));
> +
> + service::TrustStorePermissionManager pm
> + {
> + mock_agent,
> + resolver.to_functional()
> + };
> +
> + EXPECT_EQ(service::PermissionManager::Result::rejected,
> + pm.check_permission_for_credentials(default_criteria, location::service::Credentials{pid, uid}));
> +}
> +
> +// We should be provided with this kind of functionality by the trust-store.
> +// The respective request is captured here:
> +// https://bugs.launchpad.net/trust-store/+bug/1350736
> +TEST(AppArmorProfileResolver, libapparmor_profile_resolver_returns_correct_profile_for_unconfined_process)
> +{
> + auto child = core::posix::fork(
> + []() { while (true); return core::posix::exit::Status::success;},
> + core::posix::StandardStream::empty);
> +
> + EXPECT_EQ("unconfined",
> + service::TrustStorePermissionManager::libapparmor_profile_resolver()(core::trust::Pid{child.pid()}));
> +}
> +
> +TEST(AppArmorProfileResolver, libapparmor_profile_resolver_throws_for_apparmor_error)
> +{
> + // Passing -1 as the pid value results in the underlying apparmor call failing
> + // and the implementation translating to a std::system_error.
> + EXPECT_THROW(service::TrustStorePermissionManager::libapparmor_profile_resolver()(core::trust::Pid{-1}),
> + std::system_error);
> +}
> +
> +// We disabel this test by default as it requires the developer to take some manual preparations.
> +// Specifically:
> +// sudo apparmor-parser -n for_testing --add tests/app_armor_testing_profile
> +namespace for_testing
> +{
> + static constexpr const char* an_empty_profile_for_testing_purposes
> + {
> + "an_empty_profile_for_testing_purposes"
> + };
> +}
> +TEST(AppArmorProfileResolver, DISABLED_libapparmor_profile_resolver_returns_correct_profile_for_confined_process)
> +{
> + core::testing::CrossProcessSync cps; // child --| aa_profile_changed |--> parent
> + auto child = core::posix::fork(
> + [&cps]()
> + {
> + aa_change_profile(for_testing::an_empty_profile_for_testing_purposes);
> + cps.try_signal_ready_for(std::chrono::milliseconds{500});
> + while (true);
> + return core::posix::exit::Status::success;
> + },
> + core::posix::StandardStream::empty);
> +
> + cps.wait_for_signal_ready_for(std::chrono::milliseconds{500});
> +
> + EXPECT_EQ(for_testing::an_empty_profile_for_testing_purposes,
> + service::TrustStorePermissionManager::libapparmor_profile_resolver()(core::trust::Pid{child.pid()}));
> +}
>
--
https://code.launchpad.net/~thomas-voss/location-service/fix-1219164/+merge/228861
Your team Ubuntu Phablet Team is subscribed to branch lp:location-service.
More information about the Ubuntu-reviews
mailing list