Mir

include/server/mir/frontend/session_credentials.h (+0/-3)
src/server/frontend/published_socket_connector.cpp (+6/-0)
src/server/frontend/session_credentials.cpp (+0/-18)
src/server/frontend/socket_messenger.cpp (+54/-3)
src/server/frontend/socket_messenger.h (+6/-0)
tests/acceptance-tests/test_trust_session_helper.cpp (+28/-0)

To merge this branch:

bzr merge lp:~alan-griffiths/mir/fix-1314574

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2014-05-20
Kevin DuBois (community)			Approve on 2014-05-19
Alexandros Frantzis (community)		2014-05-16	Approve on 2014-05-19
Review via email: mp+219814@code.launchpad.net

Commit message

frontend: get the credentials of the real client process when it connects

Description of the change

frontend: get the credentials of the real client process when it connects

Posix mandates that with previously used SO_PEERCRED "The returned credentials are those that were in effect at the time of the call to connect(2) or socketpair(2)."

Note that privileged processes *can* spoof their SCM_CREDENTIALS as I don't think it matters for our use cases. But this seems to be the best we can achieve without relying on non-portable techniques (e.g. apparmor security contexts).

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-16:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1647/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-16:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1649/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-05-16:

Fetched 19.1 MB in 33s (573 kB/s)
W: Failed to fetch bzip2:/var/lib/apt/lists/partial/ports.ubuntu.com_ubuntu-ports_dists_utopic_universe_source_Sources Hash Sum mismatch

W: Failed to fetch bzip2:/var/lib/apt/lists/partial/ports.ubuntu.com_ubuntu-ports_dists_utopic_universe_binary-armhf_Packages Hash Sum mismatch

Triggering rebuild

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-16:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1651/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1656/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-05-19:

153 + auto const ucredp = (struct ucred *) CMSG_DATA(CMSG_FIRSTHDR(&msgh));

C cast.

210 +// Disabled as we can't be sure the mir_demo_client_basic is about

Why depend on mir_demo_client_basic instead of creating a new process and running mir_connect() ourselves? We can also record the actual client PID this way, and compare it directly with what the server got.

review: Needs Fixing

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-05-19:

> 153 + auto const ucredp = (struct ucred *)
> CMSG_DATA(CMSG_FIRSTHDR(&msgh));
>
> C cast.

Done

> 210 +// Disabled as we can't be sure the mir_demo_client_basic is about
>
> Why depend on mir_demo_client_basic instead of creating a new process and
> running mir_connect() ourselves? We can also record the actual client PID this
> way, and compare it directly with what the server got.

I did consider this - but wasn't sure I wanted to get involved with forking a process that already has a mir server running in it (and a socket pair). (Or communicating with one forked earlier after obtaining the socket.) I've been thinking that a small scriptable client might be good for several tests. But including that work in this MP seemed disproportionate.

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-05-19:

> I did consider this - but wasn't sure I wanted to get involved with forking a process that already
> has a mir server running in it (and a socket pair). (Or communicating with one forked earlier after
> obtaining the socket.) I've been thinking that a small scriptable client might be good for several
> tests. But including that work in this MP seemed disproportionate.

The scriptable client would be a solution for some cases, but I prefer the flexebility of running completely custom code for clients if possible (especially if some synchronization is needed).

I think that an "exec_in_clean_process(std::function<>, std::vector<int> excluded_fds)" that does a fork followed by an FD cleaning pass (close all open fds, except the supplied list that contains fd numbers for the server socket and pipes used for synchronization (mtf::CrossProcessSync) etc) would be enough.

Anyway, I am happy as long as we have a plan for this for the short(-ish)-term

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-05-19:

> Anyway, I am happy as long as we have a plan for this for the short(-ish)-term

We have some convergent thoughts, not a plan. Marking with a "TODO" comment

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-19:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1661/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-19:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1665/rebuild

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-05-19:

okay by me

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-20:

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/678/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/261/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/262/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/261/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-autolanding/62/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-autolanding/62/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/677/console

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) on 2014-05-20:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Alan Griffiths

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'include/server/mir/frontend/session_credentials.h'
 --- include/server/mir/frontend/session_credentials.h	2014-05-19 02:55:29 +0000
 +++ include/server/mir/frontend/session_credentials.h	2014-05-20 14:06:48 +0000
@@ -27,8 +27,6 @@
  class SessionCredentials
+ {
  public:
--    static SessionCredentials from_socket_fd(int socket_fd);
--
      SessionCredentials(pid_t pid, uid_t uid, gid_t gid);
      pid_t pid() const;
@@ -41,7 +39,6 @@
      pid_t the_pid;
      uid_t the_uid;
      gid_t the_gid;
--
  };
+ }
+ }
 === modified file 'src/server/frontend/published_socket_connector.cpp'
 --- src/server/frontend/published_socket_connector.cpp	2014-05-19 02:55:29 +0000
 +++ src/server/frontend/published_socket_connector.cpp	2014-05-20 14:06:48 +0000
@@ -205,6 +205,12 @@
      std::function<void(std::shared_ptr<Session> const& session)> const& connect_handler) const
+ {
      report->creating_session_for(server_socket->native_handle());
++
++    /* We set the SO_PASSCRED socket option in order to receive credentials */
++    auto const optval = 1;
++    if (setsockopt(server_socket->native_handle(), SOL_SOCKET, SO_PASSCRED, &optval, sizeof(optval)) == -1)
++        BOOST_THROW_EXCEPTION(std::runtime_error("Failed to set SO_PASSCRED"));
++
      connection_creator->create_connection_for(server_socket, {connect_handler, this});
+ }
 === modified file 'src/server/frontend/session_credentials.cpp'
 --- src/server/frontend/session_credentials.cpp	2014-05-19 02:55:29 +0000
 +++ src/server/frontend/session_credentials.cpp	2014-05-20 14:06:48 +0000
@@ -17,26 +17,8 @@
  #include "mir/frontend/session_credentials.h"
--#include <sys/socket.h>
--
--#include <stdexcept>
--#include <boost/throw_exception.hpp>
--
  namespace mf = mir::frontend;
--mf::SessionCredentials mf::SessionCredentials::from_socket_fd(int socket_fd)
--{
--    struct ucred cr;
--    socklen_t cl = sizeof(cr);
--
--    auto status = getsockopt(socket_fd, SOL_SOCKET, SO_PEERCRED, &cr, &cl);
--
--    if (status)
--        BOOST_THROW_EXCEPTION(std::runtime_error("Failed to query client socket credentials"));
--
--    return {cr.pid, cr.uid, cr.gid};
--}
--
  mf::SessionCredentials::SessionCredentials(pid_t pid, uid_t uid, gid_t gid) :
      the_pid{pid},
      the_uid{uid},
 === modified file 'src/server/frontend/socket_messenger.cpp'
 --- src/server/frontend/socket_messenger.cpp	2014-05-20 08:38:05 +0000
 +++ src/server/frontend/socket_messenger.cpp	2014-05-20 14:06:48 +0000
@@ -1,5 +1,5 @@
  /*
-- * Copyright © 2013 Canonical Ltd.
++ * Copyright © 2013-2014 Canonical Ltd.
+  *
   * This program is free software: you can redistribute it and/or modify
   * it under the terms of the GNU General Public License version 3 as
@@ -21,6 +21,8 @@
  #include "mir/frontend/session_credentials.h"
  #include "mir/variable_length_array.h"
++#include <boost/throw_exception.hpp>
++
  #include <errno.h>
  #include <string.h>
@@ -36,9 +38,27 @@
+ {
+ }
++mf::SessionCredentials mfd::SocketMessenger::creator_creds() const
++{
++    struct ucred cr;
++    socklen_t cl = sizeof(cr);
++
++    auto status = getsockopt(socket->native_handle(), SOL_SOCKET, SO_PEERCRED, &cr, &cl);
++
++    if (status)
++        BOOST_THROW_EXCEPTION(std::runtime_error("Failed to query client socket credentials"));
++
++    return {cr.pid, cr.uid, cr.gid};
++}
++
  mf::SessionCredentials mfd::SocketMessenger::client_creds()
+ {
--    return mf::SessionCredentials::from_socket_fd(socket->native_handle());
++    if (session_creds.pid() != 0)
++        return session_creds;
++
++    // We've not got the credentials from client yet.
++    // Return the credentials that created the socket instead.
++    return creator_creds();
+ }
  void mfd::SocketMessenger::send(char const* data, size_t length, FdSets const& fd_set)
@@ -92,7 +112,7 @@
          message->cmsg_len = header.msg_controllen;
          message->cmsg_level = SOL_SOCKET;
          message->cmsg_type = SCM_RIGHTS;
--        int *data = (int *)CMSG_DATA(message);
++        int *data = reinterpret_cast<int*>(CMSG_DATA(message));
          int i = 0;
          for (auto &fd: fds)
              data[i++] = fd;
@@ -128,7 +148,38 @@
  size_t mfd::SocketMessenger::available_bytes()
+ {
++    // We call available_bytes() once the client is talking to us
++    // so this is a pragmatic place to grab the session credentials
++    if (session_creds.pid() == 0)
++        update_session_creds();
++
      boost::asio::socket_base::bytes_readable command{true};
      socket->io_control(command);
      return command.get();
+ }
++
++void mfd::SocketMessenger::update_session_creds()
++{
++    union {
++        struct cmsghdr cmh;
++        char   control[CMSG_SPACE(sizeof(ucred))];
++    } control_un;
++
++    control_un.cmh.cmsg_len = CMSG_LEN(sizeof(ucred));
++    control_un.cmh.cmsg_level = SOL_SOCKET;
++    control_un.cmh.cmsg_type = SCM_CREDENTIALS;
++
++    msghdr msgh;
++    msgh.msg_name = nullptr;
++    msgh.msg_namelen = 0;
++    msgh.msg_iov = nullptr;
++    msgh.msg_iovlen = 0;
++    msgh.msg_control = control_un.control;
++    msgh.msg_controllen = sizeof(control_un.control);
++
++    if (recvmsg(socket->native_handle(), &msgh, MSG_PEEK) != -1)
++    {
++        auto const ucredp = reinterpret_cast<ucred*>(CMSG_DATA(CMSG_FIRSTHDR(&msgh)));
++        session_creds = {ucredp->pid, ucredp->uid, ucredp->gid};
++    }
++}
 === modified file 'src/server/frontend/socket_messenger.h'
 --- src/server/frontend/socket_messenger.h	2014-05-16 16:46:13 +0000
 +++ src/server/frontend/socket_messenger.h	2014-05-20 14:06:48 +0000
@@ -20,6 +20,7 @@
  #define MIR_FRONTEND_SOCKET_MESSENGER_H_
  #include "message_sender.h"
  #include "message_receiver.h"
++#include "mir/frontend/session_credentials.h"
  #include <mutex>
  namespace mir
@@ -42,9 +43,14 @@
      SessionCredentials client_creds() override;
  private:
++    void update_session_creds();
++    SessionCredentials creator_creds() const;
++
      std::shared_ptr<boost::asio::local::stream_protocol::socket> socket;
      std::mutex message_lock;
++    SessionCredentials session_creds{0, 0, 0};
++
      void send_fds_locked(std::unique_lock<std::mutex> const& lock, std::vector<int32_t> const& fds);
  };
+ }
 === modified file 'tests/acceptance-tests/test_trust_session_helper.cpp'
 --- tests/acceptance-tests/test_trust_session_helper.cpp	2014-05-19 02:55:29 +0000
 +++ tests/acceptance-tests/test_trust_session_helper.cpp	2014-05-20 14:06:48 +0000
@@ -27,6 +27,7 @@
  #include "mir_test_framework/stubbed_server_configuration.h"
  #include "mir_test_framework/basic_client_server_fixture.h"
  #include "mir_test_doubles/fake_ipc_factory.h"
++#include "mir_test/popen.h"
  #include <gtest/gtest.h>
  #include <gmock/gmock.h>
@@ -149,6 +150,8 @@
          sprintf(client_connect_string, "fd://%d", fd);
          return client_connect_string;
+     }
++
++    MOCK_METHOD1(process_line, void(std::string const&));
  };
  void client_fd_callback(MirConnection*, size_t count, int const* fds, void* context)
@@ -185,3 +188,28 @@
      mir_connection_release(client_connection);
+ }
++
++// TODO we need a nice way to run this (and similar tests that require a separate client process) in CI
++// Disabled as we can't be sure the mir_demo_client_basic is about
++TEST_F(TrustSessionHelper, DISABLED_client_pid_is_associated_with_session)
++{
++    auto const server_pid = getpid();
++
++    mir_connection_new_fds_for_trusted_clients(connection, 1, &client_fd_callback, this);
++    wait_for_callback(std::chrono::milliseconds(500));
++
++    InSequence seq;
++    EXPECT_CALL(*this, process_line(StrEq("Starting")));
++    EXPECT_CALL(*this, process_line(StrEq("Connected")));
++    EXPECT_CALL(*this, process_line(StrEq("Surface created")));
++    EXPECT_CALL(*this, process_line(StrEq("Surface released")));
++    EXPECT_CALL(*this, process_line(StrEq("Connection released")));
++
++    mir::test::Popen output(std::string("bin/mir_demo_client_basic -m ") + fd_connect_string(actual_fds[0]));
++
++    std::string line;
++    while (output.get_line(line)) process_line(line);
++
++    EXPECT_THAT(trusted_helper_mediator->client_pid, Ne(0));
++    EXPECT_THAT(trusted_helper_mediator->client_pid, Ne(server_pid));
++}