ipc: use O_EXCL on SHM files #339

chrissie-c · 2019-03-12T10:16:52Z

Signed-off-by: Christine Caulfield [email protected]

Signed-off-by: Christine Caulfield <[email protected]>

topimiettinen · 2019-03-12T10:34:55Z

lib/ipc_setup.c

@@ -657,9 +659,17 @@ handle_new_connection(struct qb_ipcs_service *s,
 	qb_util_log(LOG_DEBUG, "IPC credentials authenticated (%s)",
 		    c->description);

+retry_description:
+	snprintf(c->description, CONNECTION_DESCRIPTION,
+		 "%d-%d-%lu", s->pid, ugp->pid, (unsigned long)(random()%65536));


Unfortunately 16 bits of randomness is way too little, it's easy to create 64k files quickly. I'd use 64 bits as minimum. Does the protocol require a decimal number here, could it be hex or Base64 formatted instead? I'm not sure random() is a good generator of secure random numbers.

topimiettinen · 2019-03-12T10:42:10Z

lib/ipcs.c

+	if (fd == -1) {
+		seed = (time_t)time(NULL);
+	} else {
+		if (read(fd, &seed, sizeof(seed)) != 4) {


This effectively gives 32 bits of randomness to the generator. Theoretically it's not impossible to break by brute forcing this kind of number space, though in practice it might be impossible for the attacker to generate 2^32 files in /dev/shm. I'd still try to find out a way to get at least 64 bits of randomness to the file names to be sure.

srand() only takes an int! Should I not be using random - even with a seed from /dev/urandom?

Here's a good implementation in systemd src/basic/random-util.c, though it may need some porting to run on non-Linux. It's also a bit complex.

chrissie-c · 2019-03-12T12:26:52Z

Thanks. It does look a bit over the top. My idea for an alternative was to read 64 bits from /dev/urandom for each connection. If that fails (which is shouldn't of course) then fall back to 2 calls to random()

jnpkrn · 2019-03-12T13:36:16Z

Hm, and what about using POSIX 2008 endorsed mkdtemp
(EDIT: where missing, gnulib surrogate can be used) and leave any
sort of uniqueness-guessing to the outter system (which can make it
truly race-free when backed by the operating system), idea
I tangentially touched in the comment to the original report?

Cons are an extra system resource to consume (inode per the directory
EDIT:
also, depending on whether to keep the opened directory handle of the
temporary directory around for a slightly safer disposal of the
contained files where available, 1 open fd to fill up the limit,
but see, e.g.,
systemd/systemd@a8b627a
and libqb clients can elevate the respective limits to the hard
ones should they suspect issues of this kind
)
and care about (removal) + the necessity of wider retesting/revalidation,
but at least doesn't naively reimplement what's there to work with
already.

I.e., turn

/dev/shm/qb-ipcserver-request-6555-6674-9-data

into something like

/dev/shm/qb-ipcserver-request-6555-6674-9-XXXXXX/data

where the trailing XXXXXX corresponds to the semantics of mkdtemp.

Moreover, /dev/shm/qb-ipcserver-request-6555-6674-9-XXXXXX/
from above would then become the main gate keeping point (plain
ACL based access control, rather than taking care of every and
each of the underlying files separately, of course, there would
be some umask cooperation needed for this to be the case).

jnpkrn · 2019-03-12T13:57:57Z

lib/log_blackbox.c

@@ -188,7 +188,7 @@ qb_log_blackbox_write_to_file(const char *filename)
 	ssize_t written_size = 0;
 	struct qb_log_target *t;
 	struct _blackbox_file_header header;
-	int fd = open(filename, O_CREAT | O_RDWR, 0700);
+	int fd = open(filename, O_CREAT | O_RDWR | O_EXCL, 0700);


O_RDWR shall likely be flipped to O_WRONLY.

O_EXCL is dubious, overwriting existing file is a valid use case
here and an established semantics of the function.

Perhaps devising qb_log_blackbox_write_to_file_2 with some expressly
noted flags passable as flags and making qb_log_blackbox_write_to_file
depend on that might be a better idea.

chrissie-c · 2019-03-19T09:30:22Z

I'm sympathetic to the idea of mkdtemp, but annoyingly it's not compatible with shm_open() as far as I can tell. FreeBSD's shm_open() call has no filesystem visibility at all. Although it does allow a '/' in filename I'm not sure if they count as a directory as such - or even what that would mean.

As we need this security patch for the 1.0.x stream are people happy with my patch as posted and modified for this branch, then we'll look at something a bit more sophisticated for the 2.0 branch?
if not let me know what needs fixing and we'll get on with it. (ish - I'm off for the rest of the week after today)

jnpkrn · 2019-03-19T13:23:38Z

Since shm_open doesn't solve the pre-existing consumption problem in
any way for us, and because libqb already has one fallback path to use
instead of /dev/shm, I don't think there's a clear incentive to use
that.

Also, shm_open is not compatible with a portable discoverability
for the hypothetical passive IPC tracing/debugging tool to use
as mentioned at
#222 (comment)
(ditto passive blackbox-backed realtime following)
-- this discoverability scheme exists today (for years already), and
while it surely will get slightly ambiguous after the fix for this very
issue, it's at least still solvable on some platforms (look at
/proc/PID/fds etc.).

Last but not least (quite the contrary!), with your current approach,
you totally forgot that the attackers can abuse the inherent need to
create two files in the same shared namespace in sequence in a way
they can observe creation of the first (*-header; e.g. using
inotify) and hurry up to win the race over the legitimate libqb
library client on creating the second one (*-data). This makes
the currently proposed solution at sea.

With properly guarded intermediate directory, this wouldn't happen,
since there's a just a single entity to try to win the race over,
and that's moreover actively guarded by the operating system on
some platforms when mkdtemp is used.

jnpkrn · 2019-03-22T19:37:29Z

There's WIP using mkdtemp and fixing thus the described shortcomings:
#343
O_EXCL would then be more optional, but perhaps good to use nonetheless.

Regarding the tracing tool, it'd be indeed helpful for remote debugging
of situations just before fencing event occurring (the tool run remotely
in the ssh session, it will freeze eventually, but perhaps already
capturing the most precious data) -- just recently I've observed
scenario it would certainly help with.

fabbione

looks sane to me

jnpkrn · 2019-03-26T12:44:13Z

Sane with respect to the primary goal, yes.
Sufficient in the light of the known attack surface, no.

jnpkrn

(just an explicit expression to accompany my previous comment)

Use O_EXCL on IPC files

chrissie-c · 2019-03-26T15:00:44Z

I'm going to shrink this patch so it only has the O_EXCL bits in. The other part can be looked at separately as it's just going to get too messy.

patch has changed since original submission and reduced in scope

ipc: use O_EXCL on SHM files, and randomize the names

aae3615

Signed-off-by: Christine Caulfield <[email protected]>

topimiettinen reviewed Mar 12, 2019

View reviewed changes

jnpkrn reviewed Mar 12, 2019

View reviewed changes

fabbione approved these changes Mar 26, 2019

View reviewed changes

jnpkrn previously requested changes Mar 26, 2019

View reviewed changes

ipc: fixes

df8b82e

Use O_EXCL on IPC files

chrissie-c changed the title ~~ipc: use O_EXCL on SHM files, and randomize the names~~ ipc: use O_EXCL on SHM files Mar 26, 2019

chrissie-c merged commit 7cd7b06 into ClusterLabs:master Apr 8, 2019

jnpkrn mentioned this pull request Jan 8, 2020

WIP: use mkdtemp for IPC paths #343

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ipc: use O_EXCL on SHM files #339

ipc: use O_EXCL on SHM files #339

chrissie-c commented Mar 12, 2019

topimiettinen Mar 12, 2019

chrissie-c Mar 12, 2019

topimiettinen Mar 12, 2019

chrissie-c Mar 12, 2019

topimiettinen Mar 12, 2019

chrissie-c commented Mar 12, 2019

jnpkrn commented Mar 12, 2019 •

edited

Loading

jnpkrn Mar 12, 2019

chrissie-c commented Mar 19, 2019

jnpkrn commented Mar 19, 2019 •

edited

Loading

jnpkrn commented Mar 22, 2019

fabbione left a comment

jnpkrn commented Mar 26, 2019

jnpkrn left a comment

chrissie-c commented Mar 26, 2019

ipc: use O_EXCL on SHM files #339

ipc: use O_EXCL on SHM files #339

Conversation

chrissie-c commented Mar 12, 2019

topimiettinen Mar 12, 2019

Choose a reason for hiding this comment

chrissie-c Mar 12, 2019

Choose a reason for hiding this comment

topimiettinen Mar 12, 2019

Choose a reason for hiding this comment

chrissie-c Mar 12, 2019

Choose a reason for hiding this comment

topimiettinen Mar 12, 2019

Choose a reason for hiding this comment

chrissie-c commented Mar 12, 2019

jnpkrn commented Mar 12, 2019 • edited Loading

jnpkrn Mar 12, 2019

Choose a reason for hiding this comment

chrissie-c commented Mar 19, 2019

jnpkrn commented Mar 19, 2019 • edited Loading

jnpkrn commented Mar 22, 2019

fabbione left a comment

Choose a reason for hiding this comment

jnpkrn commented Mar 26, 2019

jnpkrn left a comment

Choose a reason for hiding this comment

chrissie-c commented Mar 26, 2019

jnpkrn commented Mar 12, 2019 •

edited

Loading

jnpkrn commented Mar 19, 2019 •

edited

Loading