Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified src/libstore-tests/data/serve-protocol/handshake-to-client.bin
Comment thread
cole-h marked this conversation as resolved.
Binary file not shown.
4 changes: 2 additions & 2 deletions src/libstore-tests/serve-protocol.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ struct ServeProtoTest : VersionedProtoTest<ServeProto, serveProtoDir>
{
/**
* For serializers that don't care about the minimum version, we
* used the oldest one: 1.0.
* used the oldest one: 2.5.
*/
ServeProto::Version defaultVersion = 2 << 8 | 0;
ServeProto::Version defaultVersion = 2 << 8 | 5;
};

VERSIONED_CHARACTERIZATION_TEST(
Expand Down
76 changes: 21 additions & 55 deletions src/libstore/daemon.cc
Original file line number Diff line number Diff line change
Expand Up @@ -546,47 +546,22 @@ static void performOp(
break;
}

case WorkerProto::Op::ExportPath: {
auto path = store->parseStorePath(readString(conn.from));
readInt(conn.from); // obsolete
logger->startWork();
TunnelSink sink(conn.to);
store->exportPath(path, sink);
logger->stopWork();
conn.to << 1;
break;
}

case WorkerProto::Op::ImportPaths: {
logger->startWork();
TunnelSource source(conn.from, conn.to);
auto paths = store->importPaths(source, trusted ? NoCheckSigs : CheckSigs);
logger->stopWork();
Strings paths2;
for (auto & i : paths)
paths2.push_back(store->printStorePath(i));
conn.to << paths2;
break;
}

case WorkerProto::Op::BuildPaths: {
auto drvs = WorkerProto::Serialise<DerivedPaths>::read(*store, rconn);
BuildMode mode = bmNormal;
if (GET_PROTOCOL_MINOR(conn.protoVersion) >= 15) {
mode = WorkerProto::Serialise<BuildMode>::read(*store, rconn);

/* Repairing is not atomic, so disallowed for "untrusted"
clients.

FIXME: layer violation in this message: the daemon code (i.e.
this file) knows whether a client/connection is trusted, but it
does not how how the client was authenticated. The mechanism
need not be getting the UID of the other end of a Unix Domain
Socket.
*/
if (mode == bmRepair && !trusted)
throw Error("repairing is not allowed because you are not in 'trusted-users'");
}
mode = WorkerProto::Serialise<BuildMode>::read(*store, rconn);

/* Repairing is not atomic, so disallowed for "untrusted"
clients.

FIXME: layer violation in this message: the daemon code (i.e.
this file) knows whether a client/connection is trusted, but it
does not how how the client was authenticated. The mechanism
need not be getting the UID of the other end of a Unix Domain
Socket.
*/
if (mode == bmRepair && !trusted)
throw Error("repairing is not allowed because you are not in 'trusted-users'");
logger->startWork();
store->buildPaths(drvs, mode);
logger->stopWork();
Expand Down Expand Up @@ -806,13 +781,11 @@ static void performOp(
clientSettings.buildCores = readInt(conn.from);
clientSettings.useSubstitutes = readInt(conn.from);

if (GET_PROTOCOL_MINOR(conn.protoVersion) >= 12) {
unsigned int n = readInt(conn.from);
for (unsigned int i = 0; i < n; i++) {
auto name = readString(conn.from);
auto value = readString(conn.from);
clientSettings.overrides.emplace(name, value);
}
unsigned int n = readInt(conn.from);
for (unsigned int i = 0; i < n; i++) {
auto name = readString(conn.from);
auto value = readString(conn.from);
clientSettings.overrides.emplace(name, value);
}
Comment on lines +784 to 789

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Validate override count to prevent DoS

readInt can return negative and the loop is unbounded. Add range checks and a sane cap to avoid large allocations from untrusted clients.

Apply:

-        unsigned int n = readInt(conn.from);
-        for (unsigned int i = 0; i < n; i++) {
+        int n = readInt(conn.from);
+        if (n < 0) throw Error("invalid overrides count");
+        constexpr int kMaxOverrides = 4096;
+        if (n > kMaxOverrides)
+            throw Error("too many overrides in SetOptions (max: %d)", kMaxOverrides);
+        for (int i = 0; i < n; i++) {
             auto name = readString(conn.from);
             auto value = readString(conn.from);
             clientSettings.overrides.emplace(name, value);
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
unsigned int n = readInt(conn.from);
for (unsigned int i = 0; i < n; i++) {
auto name = readString(conn.from);
auto value = readString(conn.from);
clientSettings.overrides.emplace(name, value);
}
int n = readInt(conn.from);
if (n < 0)
throw Error("invalid overrides count");
constexpr int kMaxOverrides = 4096;
if (n > kMaxOverrides)
throw Error("too many overrides in SetOptions (max: %d)", kMaxOverrides);
for (int i = 0; i < n; i++) {
auto name = readString(conn.from);
auto value = readString(conn.from);
clientSettings.overrides.emplace(name, value);
}


logger->startWork();
Expand Down Expand Up @@ -877,19 +850,12 @@ static void performOp(
auto path = store->parseStorePath(readString(conn.from));
std::shared_ptr<const ValidPathInfo> info;
logger->startWork();
try {
info = store->queryPathInfo(path);
} catch (InvalidPath &) {
if (GET_PROTOCOL_MINOR(conn.protoVersion) < 17)
throw;
}
info = store->queryPathInfo(path);
logger->stopWork();
if (info) {
if (GET_PROTOCOL_MINOR(conn.protoVersion) >= 17)
conn.to << 1;
conn.to << 1;
WorkerProto::write(*store, wconn, static_cast<const UnkeyedValidPathInfo &>(*info));
} else {
assert(GET_PROTOCOL_MINOR(conn.protoVersion) >= 17);
conn.to << 0;
}
break;
Expand Down Expand Up @@ -1064,7 +1030,7 @@ void processConnection(ref<Store> store, FdSource && from, FdSink && to, Trusted
auto [protoVersion, features] =
WorkerProto::BasicServerConnection::handshake(to, from, PROTOCOL_VERSION, WorkerProto::allFeatures);

if (protoVersion < 0x10a)
if (protoVersion < MINIMUM_PROTOCOL_VERSION)
throw Error("the Nix client version is too old");

WorkerProto::BasicServerConnection conn;
Expand Down
6 changes: 3 additions & 3 deletions src/libstore/dummy-store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ struct DummyStore : virtual Store
/**
* The dummy store is incapable of *not* trusting! :)
*/
virtual std::optional<TrustedFlag> isTrustedClient() override
std::optional<TrustedFlag> isTrustedClient() override
{
return Trusted;
}
Expand All @@ -80,7 +80,7 @@ struct DummyStore : virtual Store
unsupported("addToStore");
}

virtual StorePath addToStoreFromDump(
StorePath addToStoreFromDump(
Source & dump,
std::string_view name,
FileSerialisationMethod dumpMethod = FileSerialisationMethod::NixArchive,
Expand All @@ -103,7 +103,7 @@ struct DummyStore : virtual Store
callback(nullptr);
}

virtual ref<SourceAccessor> getFSAccessor(bool requireValidPath) override
ref<SourceAccessor> getFSAccessor(bool requireValidPath) override
{
return makeEmptySourceAccessor();
}
Expand Down
51 changes: 26 additions & 25 deletions src/libstore/export-import.cc
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include "nix/store/export-import.hh"
#include "nix/util/serialise.hh"
#include "nix/store/store-api.hh"
#include "nix/util/archive.hh"
Expand All @@ -8,27 +9,14 @@

namespace nix {

void Store::exportPaths(const StorePathSet & paths, Sink & sink)
static void exportPath(Store & store, const StorePath & path, Sink & sink)
{
auto sorted = topoSortPaths(paths);
std::reverse(sorted.begin(), sorted.end());

for (auto & path : sorted) {
sink << 1;
exportPath(path, sink);
}

sink << 0;
}

void Store::exportPath(const StorePath & path, Sink & sink)
{
auto info = queryPathInfo(path);
auto info = store.queryPathInfo(path);

HashSink hashSink(HashAlgorithm::SHA256);
TeeSink teeSink(sink, hashSink);

narFromPath(path, teeSink);
store.narFromPath(path, teeSink);

/* Refuse to export paths that have changed. This prevents
filesystem corruption from spreading to other machines.
Expand All @@ -37,16 +25,29 @@ void Store::exportPath(const StorePath & path, Sink & sink)
if (hash != info->narHash && info->narHash != Hash(info->narHash.algo))
throw Error(
"hash of path '%s' has changed from '%s' to '%s'!",
printStorePath(path),
store.printStorePath(path),
info->narHash.to_string(HashFormat::Nix32, true),
hash.to_string(HashFormat::Nix32, true));

teeSink << exportMagic << printStorePath(path);
CommonProto::write(*this, CommonProto::WriteConn{.to = teeSink}, info->references);
teeSink << (info->deriver ? printStorePath(*info->deriver) : "") << 0;
teeSink << exportMagic << store.printStorePath(path);
CommonProto::write(store, CommonProto::WriteConn{.to = teeSink}, info->references);
teeSink << (info->deriver ? store.printStorePath(*info->deriver) : "") << 0;
}

void exportPaths(Store & store, const StorePathSet & paths, Sink & sink)
{
auto sorted = store.topoSortPaths(paths);
std::reverse(sorted.begin(), sorted.end());

for (auto & path : sorted) {
sink << 1;
exportPath(store, path, sink);
}

sink << 0;
}

StorePaths Store::importPaths(Source & source, CheckSigsFlag checkSigs)
StorePaths importPaths(Store & store, Source & source, CheckSigsFlag checkSigs)
{
StorePaths res;
while (true) {
Expand All @@ -66,17 +67,17 @@ StorePaths Store::importPaths(Source & source, CheckSigsFlag checkSigs)
if (magic != exportMagic)
throw Error("Nix archive cannot be imported; wrong format");

auto path = parseStorePath(readString(source));
auto path = store.parseStorePath(readString(source));

// Activity act(*logger, lvlInfo, "importing path '%s'", info.path);

auto references = CommonProto::Serialise<StorePathSet>::read(*this, CommonProto::ReadConn{.from = source});
auto references = CommonProto::Serialise<StorePathSet>::read(store, CommonProto::ReadConn{.from = source});
auto deriver = readString(source);
auto narHash = hashString(HashAlgorithm::SHA256, saved.s);

ValidPathInfo info{path, narHash};
if (deriver != "")
info.deriver = parseStorePath(deriver);
info.deriver = store.parseStorePath(deriver);
info.references = references;
info.narSize = saved.s.size();

Expand All @@ -86,7 +87,7 @@ StorePaths Store::importPaths(Source & source, CheckSigsFlag checkSigs)

// Can't use underlying source, which would have been exhausted
auto source = StringSource(saved.s);
addToStore(info, source, NoRepair, checkSigs);
store.addToStore(info, source, NoRepair, checkSigs);

res.push_back(info.path);
}
Expand Down
24 changes: 24 additions & 0 deletions src/libstore/include/nix/store/export-import.hh
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#pragma once

#include "nix/store/store-api.hh"

namespace nix {

/**
* Magic header of exportPath() output (obsolete).
*/
const uint32_t exportMagic = 0x4558494e;

/**
* Export multiple paths in the format expected by `nix-store
* --import`. The paths will be sorted topologically.
*/
void exportPaths(Store & store, const StorePathSet & paths, Sink & sink);

/**
* Import a sequence of NAR dumps created by `exportPaths()` into the
* Nix store.
*/
StorePaths importPaths(Store & store, Source & source, CheckSigsFlag checkSigs = CheckSigs);

} // namespace nix
6 changes: 0 additions & 6 deletions src/libstore/include/nix/store/legacy-ssh-store.hh
Original file line number Diff line number Diff line change
Expand Up @@ -179,12 +179,6 @@ public:
*/
StorePathSet queryValidPaths(const StorePathSet & paths, bool lock, SubstituteFlag maybeSubstitute = NoSubstitute);

/**
* Just exists because this is exactly what Hydra was doing, and we
* don't yet want an algorithmic change.
*/
void addMultipleToStoreLegacy(Store & srcStore, const StorePathSet & paths);

void connect() override;

unsigned int getProtocol() override;
Expand Down
1 change: 1 addition & 0 deletions src/libstore/include/nix/store/meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ headers = [ config_pub_h ] + files(
'derived-path-map.hh',
'derived-path.hh',
'downstream-placeholder.hh',
'export-import.hh',
'filetransfer.hh',
'gc-store.hh',
'globals.hh',
Expand Down
2 changes: 0 additions & 2 deletions src/libstore/include/nix/store/serve-protocol-connection.hh
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,6 @@ struct ServeProto::BasicClientConnection
BuildResult getBuildDerivationResponse(const StoreDirConfig & store);

void narFromPath(const StoreDirConfig & store, const StorePath & path, std::function<void(Source &)> fun);

void importPaths(const StoreDirConfig & store, std::function<void(Sink &)> fun);
};

struct ServeProto::BasicServerConnection
Expand Down
4 changes: 2 additions & 2 deletions src/libstore/include/nix/store/serve-protocol.hh
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,8 @@ enum struct ServeProto::Command : uint64_t {
QueryValidPaths = 1,
QueryPathInfos = 2,
DumpStorePath = 3,
ImportPaths = 4,
Comment thread
edolstra marked this conversation as resolved.
ExportPaths = 5,
// ImportPaths = 4, // removed
// ExportPaths = 5, // removed
BuildPaths = 6,
QueryClosure = 7,
BuildDerivation = 8,
Expand Down
20 changes: 0 additions & 20 deletions src/libstore/include/nix/store/store-api.hh
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,6 @@ enum CheckSigsFlag : bool { NoCheckSigs = false, CheckSigs = true };

enum SubstituteFlag : bool { NoSubstitute = false, Substitute = true };

/**
* Magic header of exportPath() output (obsolete).
*/
const uint32_t exportMagic = 0x4558494e;

enum BuildMode : uint8_t { bmNormal, bmRepair, bmCheck };

enum TrustedFlag : bool { NotTrusted = false, Trusted = true };
Expand Down Expand Up @@ -818,21 +813,6 @@ public:
*/
StorePaths topoSortPaths(const StorePathSet & paths);

/**
* Export multiple paths in the format expected by ‘nix-store
* --import’.
*/
void exportPaths(const StorePathSet & paths, Sink & sink);

void exportPath(const StorePath & path, Sink & sink);

/**
* Import a sequence of NAR dumps created by exportPaths() into the
* Nix store. Optionally, the contents of the NARs are preloaded
* into the specified FS accessor to speed up subsequent access.
*/
StorePaths importPaths(Source & source, CheckSigsFlag checkSigs = CheckSigs);

struct Stats
{
std::atomic<uint64_t> narInfoRead{0};
Expand Down
2 changes: 0 additions & 2 deletions src/libstore/include/nix/store/worker-protocol-connection.hh
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,6 @@ struct WorkerProto::BasicClientConnection : WorkerProto::BasicConnection
bool * daemonException,
const StorePath & path,
std::function<void(Source &)> fun);

void importPaths(const StoreDirConfig & store, bool * daemonException, Source & source);
};

struct WorkerProto::BasicServerConnection : WorkerProto::BasicConnection
Expand Down
5 changes: 3 additions & 2 deletions src/libstore/include/nix/store/worker-protocol.hh
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ namespace nix {
/* Note: you generally shouldn't change the protocol version. Define a
new `WorkerProto::Feature` instead. */
#define PROTOCOL_VERSION (1 << 8 | 38)
#define MINIMUM_PROTOCOL_VERSION (1 << 8 | 18)
#define GET_PROTOCOL_MAJOR(x) ((x) & 0xff00)
#define GET_PROTOCOL_MINOR(x) ((x) & 0x00ff)

Expand Down Expand Up @@ -152,7 +153,7 @@ enum struct WorkerProto::Op : uint64_t {
AddIndirectRoot = 12,
SyncWithGC = 13,
FindRoots = 14,
ExportPath = 16, // obsolete
Comment thread
edolstra marked this conversation as resolved.
// ExportPath = 16, // removed
QueryDeriver = 18, // obsolete
SetOptions = 19,
CollectGarbage = 20,
Expand All @@ -162,7 +163,7 @@ enum struct WorkerProto::Op : uint64_t {
QueryFailedPaths = 24,
ClearFailedPaths = 25,
QueryPathInfo = 26,
ImportPaths = 27, // obsolete
// ImportPaths = 27, // removed
QueryDerivationOutputNames = 28, // obsolete
QueryPathFromHashPart = 29,
QuerySubstitutablePathInfos = 30,
Expand Down
Loading