Skip to content

Commit

Permalink
Prevent another user from reading someone else secret with exportPath.
Browse files Browse the repository at this point in the history
  • Loading branch information
nbp committed Aug 28, 2014
1 parent 993c099 commit d7a6268
Show file tree
Hide file tree
Showing 9 changed files with 117 additions and 14 deletions.
4 changes: 4 additions & 0 deletions release.nix
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,10 @@ let
nix = build.x86_64-linux; system = "x86_64-linux";
}).test;

tests.sec_exportPath = (import ./tests/sec-exportPath.nix rec {
nix = build.x86_64-linux; system = "x86_64-linux";
}).test;


# Aggregate job containing the release-critical jobs.
release = pkgs.releaseTools.aggregate {
Expand Down
6 changes: 5 additions & 1 deletion src/libstore/local-store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1486,7 +1486,7 @@ static void checkSecrecy(const Path & path)


void LocalStore::exportPath(const Path & path, bool sign,
Sink & sink)
const string & userName, Sink & sink)
{
assertStorePath(path);

Expand All @@ -1495,6 +1495,10 @@ void LocalStore::exportPath(const Path & path, bool sign,
if (!isValidPath(path))
throw Error(format("path ‘%1%’ is not valid") % path);

string owner = getOwnerOfSecretFile(path);
if (owner != publicUserName() && owner != userName)
throw Error(format("path ‘%1%’ is not accessible by %2%") % path % userName);

HashAndWriteSink hashAndWriteSink(sink);

dumpPath(path, hashAndWriteSink);
Expand Down
2 changes: 1 addition & 1 deletion src/libstore/local-store.hh
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public:
const PathSet & references, const string &userName, bool repair = false);

void exportPath(const Path & path, bool sign,
Sink & sink);
const string & userName, Sink & sink);

Paths importPaths(bool requireSignature, Source & source);

Expand Down
4 changes: 3 additions & 1 deletion src/libstore/remote-store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -442,12 +442,14 @@ Path RemoteStore::addTextToStore(const string & name, const string & s,


void RemoteStore::exportPath(const Path & path, bool sign,
Sink & sink)
const string & userName, Sink & sink)
{
openConnection();
writeInt(wopExportPath, to);
writeString(path, to);
writeInt(sign ? 1 : 0, to);
if (GET_PROTOCOL_MINOR(daemonVersion) >= 15)
writeString(userName, to);
processStderr(&sink); /* sink receives the actual data */
readInt(from);
}
Expand Down
2 changes: 1 addition & 1 deletion src/libstore/remote-store.hh
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public:
const PathSet & references, const string &userName, bool repair = false);

void exportPath(const Path & path, bool sign,
Sink & sink);
const string & userName, Sink & sink);

Paths importPaths(bool requireSignature, Source & source);

Expand Down
5 changes: 4 additions & 1 deletion src/libstore/store-api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -297,9 +297,12 @@ string showPaths(const PathSet & paths)
void exportPaths(StoreAPI & store, const Paths & paths,
bool sign, Sink & sink)
{
/* Ensure that the current user can read his own files */
string userName = getCurrentUserName();

foreach (Paths::const_iterator, i, paths) {
writeInt(1, sink);
store.exportPath(*i, sign, sink);
store.exportPath(*i, sign, userName, sink);
}
writeInt(0, sink);
}
Expand Down
2 changes: 1 addition & 1 deletion src/libstore/store-api.hh
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ public:
cryptographic signature (created by OpenSSL) of the preceding
data is attached. */
virtual void exportPath(const Path & path, bool sign,
Sink & sink) = 0;
const string & userName, Sink & sink) = 0;

/* Import a sequence of NAR dumps created by exportPaths() into
the Nix store. */
Expand Down
27 changes: 19 additions & 8 deletions src/nix-daemon/nix-daemon.cc
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,9 @@ struct SavingSourceAdapter : Source
};


static void performOp(bool trusted, unsigned int clientVersion,
Source & from, Sink & to, unsigned int op)
static void performOp(bool trusted, const string & user,
unsigned int clientVersion, Source & from, Sink & to,
unsigned int op)
{
switch (op) {

Expand Down Expand Up @@ -285,8 +286,11 @@ static void performOp(bool trusted, unsigned int clientVersion,

case wopAddTextToStore: {
string userName = publicUserName();
if (GET_PROTOCOL_MINOR(clientVersion) >= 15)
if (GET_PROTOCOL_MINOR(clientVersion) >= 15) {
userName = readString(from);
if (userName != publicUserName() && userName != user)
throw Error(format("missmatch between user names; connection: %1%, claimed: %2%") % user % userName);
}
string suffix = readString(from);
string s = readString(from);
PathSet refs = readStorePaths<PathSet>(from);
Expand All @@ -300,9 +304,15 @@ static void performOp(bool trusted, unsigned int clientVersion,
case wopExportPath: {
Path path = readStorePath(from);
bool sign = readInt(from) == 1;
string userName = publicUserName();
if (GET_PROTOCOL_MINOR(clientVersion) >= 15) {
userName = readString(from);
if (userName != publicUserName() && userName != user)
throw Error(format("missmatch between user names; connection: %1%, claimed: %2%") % user % userName);
}
startWork();
TunnelSink sink(to);
store->exportPath(path, sign, sink);
store->exportPath(path, sign, userName, sink);
stopWork();
writeInt(1, to);
break;
Expand Down Expand Up @@ -517,7 +527,7 @@ static void performOp(bool trusted, unsigned int clientVersion,
}


static void processConnection(bool trusted)
static void processConnection(bool trusted, const string & user)
{
MonitorFdHup monitor(from.fd);

Expand Down Expand Up @@ -582,7 +592,7 @@ static void processConnection(bool trusted)
opCount++;

try {
performOp(trusted, clientVersion, from, to, op);
performOp(trusted, user, clientVersion, from, to, op);
} catch (Error & e) {
/* If we're not in a state where we can send replies, then
something went wrong processing the input of the
Expand Down Expand Up @@ -732,6 +742,7 @@ static void daemonLoop(char * * argv)

bool trusted = false;
pid_t clientPid = -1;
string user = publicUserName();

#if defined(SO_PEERCRED)
/* Get the identity of the caller, if possible. */
Expand All @@ -743,7 +754,7 @@ static void daemonLoop(char * * argv)
clientPid = cred.pid;

struct passwd * pw = getpwuid(cred.uid);
string user = pw ? pw->pw_name : int2String(cred.uid);
user = pw ? pw->pw_name : int2String(cred.uid);

struct group * gr = getgrgid(cred.gid);
string group = gr ? gr->gr_name : int2String(cred.gid);
Expand Down Expand Up @@ -779,7 +790,7 @@ static void daemonLoop(char * * argv)
/* Handle the connection. */
from.fd = remote;
to.fd = remote;
processConnection(trusted);
processConnection(trusted, user);

_exit(0);
}, false, "unexpected Nix daemon error: ");
Expand Down
79 changes: 79 additions & 0 deletions tests/sec-exportPath.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
# Test Nix's remote build feature.

{ system, nix }:

with import <nixpkgs/nixos/lib/testing.nix> { inherit system; };

makeTest (

let

# The configuration of the build slaves.
slave =
{ config, pkgs, ... }:
{ services.openssh.enable = true;
virtualisation.writableStore = true;
environment.nix = nix;
};

# Trivial Nix expression to build remotely.
expr = config: nr: pkgs.writeText "expr.nix"
''
let utils = builtins.storePath ${config.system.build.extraUtils}; in
derivation {
name = "hello-${toString nr}";
inherit system;
PATH = "''${utils}/bin";
builder = "''${utils}/bin/sh";
args = [ "-c" "mkdir $out; echo Hello-${toString nr} > $out/host" ];
outputs = [ "out" ];
secret = true;
}
'';

in

{

nodes =
{ client =
{ config, pkgs, ... }:
{ virtualisation.writableStore = true;
virtualisation.pathsInNixDB = [ config.system.build.extraUtils ];
nix.package = nix;
nix.binaryCaches = [ ];

users.extraUsers.alice =
{ createHome = true;
home = "/home/alice";
description = "Alice Foobar";
extraGroups = [ "wheel" ];
};

users.extraUsers.bob =
{ createHome = true;
home = "/home/bob";
description = "Bob Foobar";
extraGroups = [ "wheel" ];
};
};
};

testScript = { nodes }:
''
startAll;
$client->waitForFile("/home/alice");
$client->waitForUnit("nix-daemon.service");
# Perform a build and check that it was performed on the slave.
my $out = $client->succeed("su alice -c 'nix-build ${expr nodes.client.config 1}'");
client->succeed("test -e $out");
$client->succeed("su alice -c 'nix-store --export $out'");
$client->waitForFile("/home/bob");
$client->fail("su bob -c 'nix-store --export $out'");
'';

})

0 comments on commit d7a6268

Please sign in to comment.