On Thu, 21 Aug 2003, Branden Robinson wrote:
> Out of curiosity, how can doing the chdir() break anything? A relative
> symlink has to be resolved relative to the directory in which the
Because now you're in a different directory than you expect. However,
since this is just exec()ing the X server, it won't affect xsession
scripts. If you're dumb enough to use relative paths in your XF86Config,
it might break that. It might also displace coredumps.
I should stress that I don't think the chdir is a big problem, but
merely that there is what I consider to be a more elegant way to resolve
bug 138195.
> symlink resides. If the symlink lived on a filesystem that were mounted
> noexec, the execv() should probably fail regardless of whether a chdir()
> precedes it or not.
Nope, any way you do the execv(), it will succeed. The permissions (and
mount status) of the symlink are irrelevant; only those of the resolved
file are used.
> Again, I had you confused with the submitter; sorry. Complaining about
> my solution after the bug is resolved does still feel like armchair
> quarterbacking, but I wouldn't have griped about it if it had occurred
I thought armchair programming was what Free Software was all about? At
least that's what they told me on Slashdot...;)
> If you can satisfy my curiosity above, I'd be happy to apply your
> patches. Please keep in mind that my goal with xserver-wrapper.c is to
> be very paranoid, as it runs setuid root. Any reasonable precautions we
> can take within this program, we should.
>From a security standpoint, I would prefer my solution because it does
less (not that I think the chdir() is actually dangerous, but that the
fewer things that happen, the less likely we are to cause a problem).
> If you (or anyone else reading this on debian-x) would like to audit the
> code for possible misuse of static buffers, I'd appreciate it. It would
> be a good thing to have done before sarge freezes.
Patch attached. Here's what's in it:
- the xserver buffer has an off-by-one from NUL-terminating readlink()
result (this is the one I sent before)
- var/value buffers had off-by-one from NUL-terminating read strings
- whitespace truncating could cause off-the-beginning problems; if the
entire value was spaces, then i would decrement to before the array
and place a NUL character
- I tried to remove sections where magic numbers had to line up; this
happened by:
- using sizeof(xserver) and sizeof(line) in the code instead of 1024
- defining X_WRAPPER_CONFIG_KEYLEN and X_WRAPPER_CONFIG_VALLEN and
using them (both for declarations and in the sscanf call).
- getting rid of strncasecmp in favor of strcasecmp; we're already
guaranteed that both strings are NUL-terminated, so it buys us
nothing
- got rid of useless comparison of strlen(value) to 256, even though
sscanf guarantees NUL-termination at 256
I don't think any of the overflows are exploitable, but it's good to
plug them just in case. The removal of magic numbers is purely
stylistic, but should prevent magic-number differences from causing any
future overflows. The drawback is that the code is slightly less
readable (see the sscanf call, which is forced to use macro stringify
trickery).
-Peff--- xserver-wrapper.c.orig 2003-08-22 00:45:08.000000000 -0400
+++ xserver-wrapper.c 2003-08-22 01:27:33.000000000 -0400
@@ -118,6 +118,11 @@
#define TRUE 1
#endif
+#define X_WRAPPER_CONFIG_KEYLEN 64
+#define X_WRAPPER_CONFIG_VALLEN 256
+#define STRINGIFY(x) #x
+#define STRINGIFY_VAL(x) STRINGIFY(x)
+
typedef enum {
RootOnly,
Console,
@@ -176,9 +181,8 @@
struct stat statbuf;
char xserver[1024];
char line[1024];
- char var[64];
- char value[256];
- int length;
+ char var[X_WRAPPER_CONFIG_KEYLEN+1];
+ char value[X_WRAPPER_CONFIG_VALLEN+1];
int i;
int intval;
char *val;
@@ -192,33 +196,30 @@
if (cf) {
/* parse it */
- val = fgets(line, 1024, cf);
+ val = fgets(line, sizeof(line), cf);
while (val != NULL) {
var[0] = '\0';
value[0] = '\0';
- if (sscanf(line, " %64[A-Za-z0-9_] = %256[A-Za-z0-9_ -] ",
+ if (sscanf(line,
+ " %" STRINGIFY_VAL(X_WRAPPER_CONFIG_KEYLEN) "[A-Za-z0-9_]"
+ " = %" STRINGIFY_VAL(X_WRAPPER_CONFIG_VALLEN) "[A-Za-z0-9_ -] ",
var, value) > 0) {
/* truncate extra spaces at end of value */
- length = strlen(value);
- if (length > 256) {
- length = 256;
- }
- for (i = (length - 1); (value[i] == ' '); i--) {
+ for(i = strlen(value) - 1; i >= 0 && value[i] == ' '; i--)
value[i] = '\0';
- }
/* DEBUG (void) fprintf(stderr, "var: %s, value: %s.\n", var, value);
*/
- if (strncasecmp(var, "allowed_users", 64) == 0) {
+ if (strcasecmp(var, "allowed_users") == 0) {
level = getSecLevel(value);
/* DEBUG (void) fprintf(stderr, "security level set to %d\n",
level); */
}
- if (strncasecmp(var, "nice_value", 64) == 0) {
+ if (strcasecmp(var, "nice_value") == 0) {
niceval = atoi(value);
if ((niceval < -20) || (niceval > 19)) niceval = 0;
/* DEBUG (void) fprintf(stderr, "nice value set to %d\n", niceval);
*/
}
}
- val = fgets(line, 1024, cf);
+ val = fgets(line, sizeof(line), cf);
}
(void) fclose(cf);
@@ -233,7 +234,7 @@
exit(1);
}
- i = readlink(X_SERVER_SYMLINK, xserver, 1024);
+ i = readlink(X_SERVER_SYMLINK, xserver, sizeof(xserver) - 1);
if (i < 0) {
(void) fprintf(stderr, "X: cannot read %s symbolic link (%s), aborting.\n",
@@ -243,8 +244,8 @@
xserver[i] = '\0'; /* readlink() does not null-terminate the string */
- if ((strncmp(xserver, "/usr/bin/X11/X", 1024) == 0) ||
- (strncmp(xserver, "/usr/X11R6/bin/X", 1024) == 0)) {
+ if ((strcmp(xserver, "/usr/bin/X11/X") == 0) ||
+ (strcmp(xserver, "/usr/X11R6/bin/X") == 0)) {
(void) fprintf(stderr, "X: %s points back to X wrapper executable, "
"aborting.\n", X_SERVER_SYMLINK);
exit(1);
