Re: [hackers] [quark][PATCH] Add a config switch to enable/disable NPROC limit
On Mon, 25 Jan 2021 14:17:17 +0100 Giulio Picierro wrote: Dear Giulio, > sorry for the late reply, I had a really busy week (course to teach) > :/. don't worry about it! This is a mailing list and meant to be asynchronous. Don't feel pressured to response and rather take your time; it's a pastime after all. > I didn't have the chance to test the new code, which I will do soon, > hopefully. > > If I understand correctly your solution is to read the rlimit from > the system and then update accordingly. > > Now it seems to me a good solution, however I have to ask: do we > really need to set the rlimit on the number of processes (which are > fixed)? > > I mean, what is the rationale behind it? Security purposes? > > I'm just asking out of curiosity, to understand better the design > choices :D. There are two aspects at play here, the limit on open file descriptors of the current process (RLIMIT_NOFILE) and the limit on threads per user (RLIMIT_NPROC). The former is simple: It's a per-process-limit and we just apply a heuristic and find a coarse upper bound for file descriptors the quark process will consume. You see we set both cur and max, which is because a program usually gets a signal when it exceeds the "soft" cur-limit, but we don't want that. We want to either succeed or fail hard. The nice thing about setrlimit() is that if the process is not privileged (or has CAP_SYS_RESOURCE set) it can always set the soft limit and irrevocably reduce the hard limit, so in case the limits are already large enough, we don't even need CAP_SYS_RESOURCE. The latter case is much more difficult, because the thread-limit is not per-process but per-user. However, there isn't really a portable way to find out how many current threads a user has. Say we give quark the flag "-t 50" and are thus telling quark to spawn 50 serving-threads. However, if the user has a thread-limit of 1000 and already has 990 threads active, pthread_create() will fail after creating 10 threads. However, if the user has a thread-limit of 90 and only 20 active threads, the thread creation will succeed (we suppose in both cases that the number of "foreign" threads is constant). Usually the thread limit is very high and not an issue, but there might be configurations where that is not the case. The approach I worked out for quark is kind of a hack, because what it does is just ask the system to increase the thread-limit by the number of threads quark needs. If that fails, e.g. if we are already at the kernel limit, we just ignore that error, because that's as much as we can hope to do. The only case where quark now could possibly fail with this heuristic is on a system where a user is close to thread-saturation (in terms of its limits, which must be way below the kernel limits) and some user-program rapidly spawns threads in the few miliseconds between quark's thread-limit-increase (triggering a TOCTOU) and thread-allocation. However, even in this extreme case, quark would just error out on pthread_create() and this is no security issue or anything. One could get rid of setting the thread limit by spawning the worker threads before dropping root, but I just don't feel comfortable with that. The earlier you lobotomize yourself, the better. With best regards, hoping this was helpful to you Laslo
Re: [hackers] [quark][PATCH] Add a config switch to enable/disable NPROC limit
Dear Laslo, sorry for the late reply, I had a really busy week (course to teach) :/. I didn't have the chance to test the new code, which I will do soon, hopefully. If I understand correctly your solution is to read the rlimit from the system and then update accordingly. Now it seems to me a good solution, however I have to ask: do we really need to set the rlimit on the number of processes (which are fixed)? I mean, what is the rationale behind it? Security purposes? I'm just asking out of curiosity, to understand better the design choices :D. Thank you, Giulio. On 1/21/21 1:18 AM, Laslo Hunhold wrote: On Mon, 18 Jan 2021 23:03:11 +0100 Laslo Hunhold wrote: that is a really nice observation! Thanks for pointing it out and your elaborate explanation. I must honestly admit that I assumed the limit was per process and not per user. I'll think about how to approach this the best way; given your aforementioned fact, I only see two options: 1) Don't touch the rlimits and let it fail, giving a proper error message (might be problematic for open file descriptors that might get exhausted at runtime). One can also check the limits beforehand and error out (e.g. if we cannot guarantee 4 fds per slot). 2) Uncrement the rlimits by first reading them and setting the incremented value. possible problems here are TOCTOU (even though the risk here is not too high) and a possible interference in things that shouldn't be touched by convention. To give a followup, I went with option 2), because it allows the smoothest operation. Quark is run as root and thus can seize all the assets it needs. I'm sure many set their global resource limits to reasonable values, but if you run your server and give it a thread and slot count, you can estimate that it might exceed your resources. In that respect, it is forgivable by quark to just raise the bar instead of failing. Thanks again, Giulio, for your input and patch suggestion. I hope this fixes your issues in your case! With best regards Laslo
Re: [hackers] [quark][PATCH] Add a config switch to enable/disable NPROC limit
On Mon, 18 Jan 2021 23:03:11 +0100 Laslo Hunhold wrote: > that is a really nice observation! Thanks for pointing it out and your > elaborate explanation. I must honestly admit that I assumed the limit > was per process and not per user. I'll think about how to approach > this the best way; given your aforementioned fact, I only see two > options: > >1) Don't touch the rlimits and let it fail, giving a proper error > message (might be problematic for open file descriptors that > might get exhausted at runtime). One can also check the limits > beforehand and error out (e.g. if we cannot guarantee 4 fds per > slot). >2) Uncrement the rlimits by first reading them and setting the > incremented value. possible problems here are TOCTOU (even > though the risk here is not too high) and a possible > interference in things that shouldn't be touched by convention. To give a followup, I went with option 2), because it allows the smoothest operation. Quark is run as root and thus can seize all the assets it needs. I'm sure many set their global resource limits to reasonable values, but if you run your server and give it a thread and slot count, you can estimate that it might exceed your resources. In that respect, it is forgivable by quark to just raise the bar instead of failing. Thanks again, Giulio, for your input and patch suggestion. I hope this fixes your issues in your case! With best regards Laslo
Re: [hackers] [quark][PATCH] Add a config switch to enable/disable NPROC limit
On Sun, 17 Jan 2021 17:29:53 +0100 Giulio Picierro wrote: Dear Giulio, > Quoting the book "The Linux Programming Interface" from Micheal > Kerrisk: "the RLIMIT_NPROC limit, which places a limit on the number > of processes that can be created, is measured against not just that > process’s consumption of the corresponding resource, but also against > the sum of resources consumed by all processes with the same real > user ID." > > This leads quark to easily fail on Linux when launched with the same > userid of a logged user. > > For example if the user 'giulio' has an active desktop session, the > following command: > > $ sudo ./quark -p 8080 -u giulio -g giulio -l > > fails with the following error: > > $ ./quark: pthread_create: Resource temporarily unavailable > > No error occour if instead quark is launched with an userid that does > not have a session, such as the 'http' user, usually reserved for web > servers. > > I don't know if this is expected or this could be considered a bug: > in the end for production servers we could expect that the limit > works correctly. > > In any case, the least invasive way that I have found to solve the > issue is to introduce a config switch to disable the limit, retaining > it enabled by default. that is a really nice observation! Thanks for pointing it out and your elaborate explanation. I must honestly admit that I assumed the limit was per process and not per user. I'll think about how to approach this the best way; given your aforementioned fact, I only see two options: 1) Don't touch the rlimits and let it fail, giving a proper error message (might be problematic for open file descriptors that might get exhausted at runtime). One can also check the limits beforehand and error out (e.g. if we cannot guarantee 4 fds per slot). 2) Uncrement the rlimits by first reading them and setting the incremented value. possible problems here are TOCTOU (even though the risk here is not too high) and a possible interference in things that shouldn't be touched by convention. What do the others think? With best regards Laslo
[hackers] [quark][PATCH] Add a config switch to enable/disable NPROC limit
Quoting the book "The Linux Programming Interface" from Micheal Kerrisk:
"the RLIMIT_NPROC limit, which places a limit on the number of processes that
can be created, is measured against not just that process’s consumption of the
corresponding resource, but also against the sum of resources consumed by all
processes with the same real user ID."
This leads quark to easily fail on Linux when launched with the same userid of
a logged user.
For example if the user 'giulio' has an active desktop session, the following
command:
$ sudo ./quark -p 8080 -u giulio -g giulio -l
fails with the following error:
$ ./quark: pthread_create: Resource temporarily unavailable
No error occour if instead quark is launched with an userid that does not have
a session, such as the 'http' user, usually reserved for web servers.
I don't know if this is expected or this could be considered a bug: in the end
for production servers we could expect that the limit works correctly.
In any case, the least invasive way that I have found to solve the issue is to
introduce a config switch to disable the limit, retaining it enabled by default.
---
Makefile | 2 +-
config.def.h | 2 ++
main.c | 3 +++
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index da0e458..61da150 100644
--- a/Makefile
+++ b/Makefile
@@ -10,7 +10,7 @@ all: quark
data.o: data.c data.h http.h util.h config.mk
http.o: http.c config.h http.h util.h config.mk
-main.o: main.c arg.h data.h http.h queue.h sock.h util.h config.mk
+main.o: config.h main.c arg.h data.h http.h queue.h sock.h util.h config.mk
sock.o: sock.c sock.h util.h config.mk
util.o: util.c util.h config.mk
diff --git a/config.def.h b/config.def.h
index 56f62aa..7cccad2 100644
--- a/config.def.h
+++ b/config.def.h
@@ -4,6 +4,8 @@
#define BUFFER_SIZE 4096
#define FIELD_MAX 200
+#define ENABLE_NPROC_LIMIT 1
+
/* mime-types */
static const struct {
char *ext;
diff --git a/main.c b/main.c
index 86b2d0c..f09b5db 100644
--- a/main.c
+++ b/main.c
@@ -19,6 +19,7 @@
#include "arg.h"
#include "data.h"
+#include "config.h"
#include "http.h"
#include "queue.h"
#include "sock.h"
@@ -646,6 +647,7 @@ main(int argc, char *argv[])
}
/* set the thread limit (2 + nthreads) */
+#if ENABLE_NPROC_LIMIT
rlim.rlim_cur = rlim.rlim_max = 2 + nthreads;
if (setrlimit(RLIMIT_NPROC, ) < 0) {
if (errno == EPERM) {
@@ -655,6 +657,7 @@ main(int argc, char *argv[])
die("setrlimit:");
}
}
+#endif
/* limit ourselves to reading the servedir and block further
unveils */
eunveil(servedir, "r");
--
2.30.0
