Python, Linux, and the setuid bit

[Ethan Furman]

For anyone in the unenviable position of needing [1] to run Python scripts with the setuid bit on, there is an
suid-python wrapper [2] that makes this possible.

When I compiled it I was given a couple warnings. Can any one shed light on what they mean?

==================================================================
suid-python.c: In function ‘malloc_abort’:
suid-python.c:119:17: warning: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘size_t’ [-Wformat]
suid-python.c: In function ‘remove_env_prefix’:
suid-python.c:200:32: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
suid-python.c:201:32: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
==================================================================

and the code segments in question:

==================================================================
void *
malloc_abort(size_t size)
{
void *buf;

buf = malloc(size);
if (!buf)
{
fprintf(stderr, "Could not allocate %d bytes. errno=%d\n",
size, errno);
exit(1);
}

return buf;
}
------------------------------------------------------------------
int
remove_env_prefix(char **envp, char *prefix)
{
char **envp_read;
char **envp_write;
int prefix_len = strlen(prefix);
int removed_count = 0;

envp_write = envp;
for (envp_read = envp; *envp_read; envp_read++)
{
if (!strncmp(*envp_read, prefix, prefix_len))
{
/* Step past the environment variable that we don't want. */
removed_count++;
continue;
}

if (envp_read != envp_write)
{
*envp_write = *envp_read;
}

envp_write++;
}

/* Set the remaining slots to NULL. */
if (envp_write < envp_read)
{
memset(envp_write, 0, ((unsigned int) envp_read -
(unsigned int) envp_write));
}

return removed_count;
}
==================================================================

Thanks!

 

[John Gordon]

fprintf(stderr, "Could not allocate %d bytes. errno=%d\n",
size, errno);

%d is not the correct specifier for printing objects of type size_t.

char **envp_read;
char **envp_write;

if (envp_write < envp_read)
{
memset(envp_write, 0, ((unsigned int) envp_read -
(unsigned int) envp_write));
}

I think it's complaining about casting the char ** objects to unsigned int.

 

[Grant Edwards]

%d is not the correct specifier for printing objects of type size_t.

I believe %zu is the correct format specifier for size_t values.

I think it's complaining about casting the char ** objects to unsigned int.

If we assume that the author is trying to clear memory between the
addresses pointed to by the two variables, then it's probably better
be cast to (char *) before the subtracted. That should result in an
integer value.

 

[Grant Edwards]

If we assume that the author is trying to clear memory between the
addresses pointed to by the two variables, then it's probably better
be cast to (char *) before the subtracted.

Wow, I mangled that sentence. It should have been something like:

then it's probably better to cast them to (char *) before the
subtraction.

memset(envp_write, 0, ((char*)envp_read)-((char*)envp_write));

 

[Richard Kettlewell]

memset(envp_write, 0, ((unsigned int) envp_read -
(unsigned int) envp_write));

That is a remarkable blunder for a security-critical program.

On a 64-bit platform, the best case outcome is that it will throw away
the top 32 bits of each pointer before doing the subtraction, yielding
the wrong answer if the discarded bits happen to differ.

(There is no limit to the worst case behavior; the effect of converting
a pointer value to an integer type which cannot represent the result is
undefined.)

I would write:

(envp_read - envp_write) * sizeof *envp_read

 

[Chris Angelico]

That is a remarkable blunder for a security-critical program.

On a 64-bit platform, the best case outcome is that it will throw away
the top 32 bits of each pointer before doing the subtraction, yielding
the wrong answer if the discarded bits happen to differ.

If the pointers are more than 4GB apart, then yes, it'll give the
wrong answer - just as if you'd subtracted and then cast down to an
integer too small for the result. But if they're two pointers inside
the same object (already a requirement for pointer arithmetic) and not
4GB apart, then two's complement arithmetic will give the right result
even if the discarded bits differ. So while you're correct in theory,
in practice it's unlikely to actually be a problem.

(There is no limit to the worst case behavior; the effect of converting
a pointer value to an integer type which cannot represent the result is
undefined.)

I would write:

(envp_read - envp_write) * sizeof *envp_read

I'd simply cast them to (char *), which will permit the arithmetic
quite happily and look cleaner.

ChrisA

 

[Chris Angelico]

then two's complement arithmetic will give the right result
even if the discarded bits differ.

Clarification: Two's complement isn't the only way this could be done,
but it is the most likely. So, in theory, there are several possible
causes of disaster, but in practice, on any IBM PC compatible
architecture, casting a pointer to an integer will usually take the
lowest N bits, and two's complement arithmetic will be used, and the
environment is unlikely to hit 4GB in size, so the program will
"happen to work" in >99.999% of cases.

ChrisA

 

[Richard Kettlewell]

If the pointers are more than 4GB apart, then yes, it'll give the
wrong answer - just as if you'd subtracted and then cast down to an
integer too small for the result. But if they're two pointers inside
the same object (already a requirement for pointer arithmetic) and not
4GB apart, then two's complement arithmetic will give the right result
even if the discarded bits differ. So while you're correct in theory,
in practice it's unlikely to actually be a problem.

This program is on a security boundary, the pathological cases are
precisely the ones the attacker looks for.

(It’s hard to see how an attacker could turn this into a useful attack.
But perhaps the attacker has more imagination than me.)

 

[Chris Angelico]

This program is on a security boundary, the pathological cases are
precisely the ones the attacker looks for.

(It’s hard to see how an attacker could turn this into a useful attack.
But perhaps the attacker has more imagination than me.)

Quite frankly, I don't even care It's easy enough to fix the bug.
The idiomatic code will compile without warnings *and* be secure, so
I'm not seeing any reason to use the existing form. All I'm saying is
that it's normally going to happen to work; sure, an attacker might
well be able to get into something (although if you can generate 4GB
of environment, the fact that it doesn't get zeroed is likely to be
less of a concern than the massive DOS potential of a huge env!!), but
casual usage will have it seeming to work. The obvious solution is
right in every possible way, so that's the thing to do moving forward.

ChrisA