Skip to content

ZOOKEEPER-5124 (C client) Chroot path validation fails to isolate namespaces #2499

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
apurtell wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ZOOKEEPER-5124 (C client) Chroot path validation fails to isolate namespaces #2499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
105 changes: 81 additions & 24 deletions zookeeper-client/zookeeper-client-c/src/zookeeper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1705,42 +1705,73 @@ void free_duplicate_path(const char *free_path, const char* path) {
}

/**
prepend the chroot path if available else return the path
*/
static char* prepend_string(zhandle_t *zh, const char* client_path) {
* Map a client path to a server path under the chroot.
*
* A valid client path is absolute. The chroot prefix does not make an
* invalid path valid. A sequential create of the client root stays in
* the chroot namespace. A null result is an invalid path or a failed
* allocation.
*/
static char* prepend_chroot(zhandle_t *zh, const char* client_path, int mode) {
size_t chroot_len;
size_t path_len;
char *ret_str;
if (zh == NULL || zh->chroot == NULL)
return (char *) client_path;
// handle the chroot itself, client_path = "/"
if (strlen(client_path) == 1) {
if (client_path == NULL || client_path[0] != '/')
return NULL;
/*
* The client root is the chroot node. A sequential create of the root
* keeps a slash at the end of the server path. The server adds the
* sequence number as a child of the chroot, not as a sibling.
*/
if (client_path[1] == '\0' && !ZOOKEEPER_IS_SEQUENCE(mode)) {
return strdup(zh->chroot);
}
ret_str = (char *) malloc(strlen(zh->chroot) + strlen(client_path) + 1);
strcpy(ret_str, zh->chroot);
return strcat(ret_str, client_path);
chroot_len = strlen(zh->chroot);
path_len = strlen(client_path);
ret_str = (char *) malloc(chroot_len + path_len + 1);
if (ret_str == NULL)
return NULL;
memcpy(ret_str, zh->chroot, chroot_len);
memcpy(ret_str + chroot_len, client_path, path_len + 1);
return ret_str;
}

/**
* Map a client path to a server path when the request does not create a
* sequential node.
*/
static char* prepend_string(zhandle_t *zh, const char* client_path) {
return prepend_chroot(zh, client_path, 0);
}

/**
strip off the chroot string from the server path
if there is one else return the exact path
* Map a server path to a client path.
*
* The chroot prefix matches only at a path component boundary, f.e. a chroot
* of /app does not match the path /app2. Cluster configuration is not in
* the chroot namespace, so it is not affected by the chroot prefix.
*/
char* sub_string(zhandle_t *zh, const char* server_path) {
char *ret_str;
if (zh->chroot == NULL)
size_t chroot_len;
if (zh->chroot == NULL || server_path == NULL)
return (char *) server_path;
//ZOOKEEPER-1027
if (strncmp(server_path, zh->chroot, strlen(zh->chroot)) != 0) {
chroot_len = strlen(zh->chroot);
if (strncmp(server_path, zh->chroot, chroot_len) == 0) {
if (server_path[chroot_len] == '\0') {
return strdup("/");
}
if (server_path[chroot_len] == '/') {
return strdup(server_path + chroot_len);
}
}
if (strcmp(server_path, ZOO_CONFIG_NODE) != 0) {
LOG_ERROR(LOGCALLBACK(zh), "server path %s does not include chroot path %s",
server_path, zh->chroot);
return (char *) server_path;
}
if (strlen(server_path) == strlen(zh->chroot)) {
//return "/"
ret_str = strdup("/");
return ret_str;
}
ret_str = strdup(server_path + strlen(zh->chroot));
return ret_str;
return (char *) server_path;
}

static buffer_list_t *allocate_buffer(char *buff, int len)
Expand Down Expand Up @@ -4114,7 +4145,7 @@ static int Request_path_init(zhandle_t *zh, int mode,
{
assert(path_out);

*path_out = prepend_string(zh, path);
*path_out = prepend_chroot(zh, path, mode);
/*
* Restore *path_out to caller's original path pointer on validation
* failure so caller-side free_duplicate_path() remains safe.
Expand Down Expand Up @@ -5677,13 +5708,39 @@ int zoo_wget(zhandle_t *zh, const char *path,
int zoo_getconfig(zhandle_t *zh, int watch, char *buffer,
int* buffer_len, struct Stat *stat)
{
return zoo_wget(zh,ZOO_CONFIG_NODE,watch?zh->watcher:0,zh->context, buffer,buffer_len,stat);
return zoo_wgetconfig(zh,watch?zh->watcher:0,zh->context, buffer,buffer_len,stat);
}

/*
* Cluster configuration is not in the chroot namespace. This function
* uses the asynchronous configuration call so that the synchronous and
* asynchronous APIs will both read the same configuration node.
*/
int zoo_wgetconfig(zhandle_t *zh, watcher_fn watcher, void* watcherCtx,
char *buffer, int* buffer_len, struct Stat *stat)
{
return zoo_wget(zh, ZOO_CONFIG_NODE, watcher, watcherCtx, buffer, buffer_len, stat);
struct sync_completion *sc;
int rc=0;

if(buffer_len==NULL)
return ZBADARGUMENTS;
if((sc=alloc_sync_completion())==NULL)
return ZSYSTEMERROR;

sc->u.data.buffer = buffer;
sc->u.data.buff_len = *buffer_len;
rc=zoo_awgetconfig(zh, watcher, watcherCtx, SYNCHRONOUS_MARKER, sc);
if(rc==ZOK){
wait_sync_completion(sc);
rc = sc->rc;
if (rc == 0) {
if(stat)
*stat = sc->u.data.stat;
*buffer_len = sc->u.data.buff_len;
}
}
free_sync_completion(sc);
return rc;
}


Expand Down
166 changes: 166 additions & 0 deletions zookeeper-client/zookeeper-client-c/tests/TestClient.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ struct buff_struct_2 {
// it needs a overhaul to work properly with both threaded and single
// threaded (ZOOKEEPER-2640)
#ifdef THREADED
// The client uses this internal function to map a server path to a client path.
extern "C" char* sub_string(zhandle_t *zh, const char* server_path);

// For testing LogMessage Callback functionality
list<string> logMessages;
void logMessageHandler(const char* message) {
Expand Down Expand Up @@ -225,6 +228,9 @@ class Zookeeper_simpleSystem : public CPPUNIT_NS::TestFixture
CPPUNIT_TEST(testPing);
CPPUNIT_TEST(testAcl);
CPPUNIT_TEST(testChroot);
CPPUNIT_TEST(testChrootPathValidation);
CPPUNIT_TEST(testChrootGetConfig);
CPPUNIT_TEST(testChrootServerPathMapping);
CPPUNIT_TEST(testAuth);
CPPUNIT_TEST(testCloseInAuthCompletion);
CPPUNIT_TEST(testHangingClient);
Expand Down Expand Up @@ -1205,6 +1211,166 @@ class Zookeeper_simpleSystem : public CPPUNIT_NS::TestFixture
CPPUNIT_ASSERT(count == 0);
}

// A handle with a chroot and a handle with no chroot reject the same
// invalid client paths. Invalid paths do not reach nodes outside the
// chroot.
void testChrootPathValidation() {
watchctx_t ctx, ctx_ch;
struct Stat stat;
struct String_vector children;
char buf[64];
int len;
zhandle_t *zk = createClient(&ctx);
CPPUNIT_ASSERT(zk);
CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_create(zk, "/tenant", "", 0,
&ZOO_OPEN_ACL_UNSAFE, 0, 0, 0));
CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_create(zk, "/tenant2", "", 0,
&ZOO_OPEN_ACL_UNSAFE, 0, 0, 0));
CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_create(zk, "/tenantx", "", 0,
&ZOO_OPEN_ACL_UNSAFE, 0, 0, 0));
zhandle_t *zk_ch = createchClient(&ctx_ch, "127.0.0.1:22181/tenant");
CPPUNIT_ASSERT(zk_ch);

const char *bad[] = { NULL, "", "x", "2", "2/x", "x/y" };
for (size_t i = 0; i < sizeof(bad) / sizeof(bad[0]); i++) {
const char *path = bad[i];
zhandle_t *handles[] = { zk, zk_ch };
int multi_rc[2];
for (size_t j = 0; j < 2; j++) {
zhandle_t *zh = handles[j];
verifyCreateFails(path, zh);
verifyCreateFailsSeq(path, zh);
CPPUNIT_ASSERT_EQUAL((int)ZBADARGUMENTS,
zoo_exists(zh, path, 0, &stat));
len = sizeof(buf);
CPPUNIT_ASSERT_EQUAL((int)ZBADARGUMENTS,
zoo_get(zh, path, 0, buf, &len, &stat));
CPPUNIT_ASSERT_EQUAL((int)ZBADARGUMENTS,
zoo_set(zh, path, "x", 1, -1));
CPPUNIT_ASSERT_EQUAL((int)ZBADARGUMENTS,
zoo_delete(zh, path, -1));
CPPUNIT_ASSERT_EQUAL((int)ZBADARGUMENTS,
zoo_get_children(zh, path, 0, &children));
CPPUNIT_ASSERT_EQUAL((int)ZBADARGUMENTS,
zoo_remove_all_watches(zh, path, ZWATCHTYPE_ANY, 1));
zoo_op_t op;
zoo_op_result_t result;
zoo_delete_op_init(&op, path, -1);
multi_rc[j] = zoo_multi(zh, 1, &op, &result);
}
// zoo_multi reports a sub-request error as ZMARSHALLINGERROR.
// The two handles give the same error.
CPPUNIT_ASSERT(multi_rc[0] != ZOK);
CPPUNIT_ASSERT_EQUAL(multi_rc[0], multi_rc[1]);
}

// Invalid paths did not change the chroot node or its sibling nodes.
CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_exists(zk, "/tenant", 0, &stat));
CPPUNIT_ASSERT_EQUAL(0, stat.version);
CPPUNIT_ASSERT_EQUAL(0, stat.numChildren);
CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_exists(zk, "/tenant2", 0, &stat));
CPPUNIT_ASSERT_EQUAL(0, stat.numChildren);
CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_exists(zk, "/tenantx", 0, &stat));
CPPUNIT_ASSERT_EQUAL(0, stat.version);

// A sequential create of the client root makes a child of the chroot
// node. A sequential create of "/" on a handle with no chroot has
// the same result at the cluster root.
char path_buffer[64];
CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_create(zk_ch, "/", "", 0,
&ZOO_OPEN_ACL_UNSAFE, ZOO_SEQUENCE, path_buffer,
sizeof(path_buffer)));
CPPUNIT_ASSERT_EQUAL((size_t)11, strlen(path_buffer));
CPPUNIT_ASSERT_EQUAL('/', path_buffer[0]);
string server_path = string("/tenant") + path_buffer;
CPPUNIT_ASSERT_EQUAL((int)ZOK,
zoo_exists(zk, server_path.c_str(), 0, &stat));
CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_get_children(zk, "/", 0, &children));
for (int i = 0; i < children.count; i++) {
CPPUNIT_ASSERT(strncmp(children.data[i], "tenant0", 7) != 0);
}
deallocate_String_vector(&children);

// Other operations on the client root use the chroot node.
CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_exists(zk_ch, "/", 0, &stat));
CPPUNIT_ASSERT_EQUAL(1, stat.numChildren);
CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_get_children(zk_ch, "/", 0, &children));
CPPUNIT_ASSERT_EQUAL(1, (int)children.count);
CPPUNIT_ASSERT_EQUAL(string(path_buffer + 1), string(children.data[0]));
deallocate_String_vector(&children);
}

// Cluster configuration is not in the chroot namespace. Synchronous and
// asynchronous reads get the same node as a handle with no chroot.
void testChrootGetConfig() {
watchctx_t ctx, ctx_ch;
char buf[1024], buf_ch[1024];
int len = sizeof(buf);
int len_ch = sizeof(buf_ch);
struct Stat stat, stat_ch;
zhandle_t *zk = createClient(&ctx);
CPPUNIT_ASSERT(zk);
CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_create(zk, "/cfgchroot", "", 0,
&ZOO_OPEN_ACL_UNSAFE, 0, 0, 0));
zhandle_t *zk_ch = createchClient(&ctx_ch, "127.0.0.1:22181/cfgchroot");
CPPUNIT_ASSERT(zk_ch);

CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_getconfig(zk, 0, buf, &len, &stat));
CPPUNIT_ASSERT_EQUAL((int)ZOK,
zoo_getconfig(zk_ch, 0, buf_ch, &len_ch, &stat_ch));
CPPUNIT_ASSERT_EQUAL(len, len_ch);
CPPUNIT_ASSERT(len <= 0 || memcmp(buf, buf_ch, len) == 0);
CPPUNIT_ASSERT_EQUAL(stat.czxid, stat_ch.czxid);
CPPUNIT_ASSERT_EQUAL(stat.mzxid, stat_ch.mzxid);

len_ch = sizeof(buf_ch);
memset(&stat_ch, 0, sizeof(stat_ch));
CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_wgetconfig(zk_ch, watcher, &ctx_ch,
buf_ch, &len_ch, &stat_ch));
CPPUNIT_ASSERT_EQUAL(len, len_ch);
CPPUNIT_ASSERT_EQUAL(stat.mzxid, stat_ch.mzxid);
}

static bool hasChrootPathError() {
for (list<string>::iterator it = logMessages.begin();
it != logMessages.end(); ++it) {
if (it->find("does not include chroot path") != string::npos) {
return true;
}
}
return false;
}

// The chroot prefix of a server path ends at a path-component boundary.
// Cluster configuration that is not in the chroot does not cause an
// error.
void testChrootServerPathMapping() {
watchctx_t ctx;
logMessages.clear();
zhandle_t *zh = zookeeper_init2("127.0.0.1:22181/zoo", watcher, 10000,
0, &ctx, 0, logMessageHandler);
CPPUNIT_ASSERT(zh);
ctx.zh = zh;

char *client_path = sub_string(zh, "/zoo");
CPPUNIT_ASSERT_EQUAL(string("/"), string(client_path));
free(client_path);
client_path = sub_string(zh, "/zoo/a/b");
CPPUNIT_ASSERT_EQUAL(string("/a/b"), string(client_path));
free(client_path);
CPPUNIT_ASSERT(!hasChrootPathError());

const char *config_path = ZOO_CONFIG_NODE;
CPPUNIT_ASSERT(sub_string(zh, config_path) == config_path);
CPPUNIT_ASSERT(!hasChrootPathError());

const char *sibling_path = "/zoo2/a";
CPPUNIT_ASSERT(sub_string(zh, sibling_path) == sibling_path);
CPPUNIT_ASSERT(hasChrootPathError());

CPPUNIT_ASSERT(sub_string(zh, NULL) == NULL);
}

// Test creating normal handle via zookeeper_init then explicitly setting callback
void testLogCallbackSet()
{
Expand Down
Loading