Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.python > #70234 > unrolled thread

Python, Linux, and the setuid bit

Back to article view | Back to comp.lang.python

Contents

  Python, Linux, and the setuid bit Ethan Furman <ethan@stoneleaf.us> - 2014-04-14 14:13 -0700
    Re: Python, Linux, and the setuid bit John Gordon <gordon@panix.com> - 2014-04-14 21:55 +0000
      Re: Python, Linux, and the setuid bit Grant Edwards <invalid@invalid.invalid> - 2014-04-14 22:04 +0000
        Re: Python, Linux, and the setuid bit Grant Edwards <invalid@invalid.invalid> - 2014-04-14 22:07 +0000
    Re: Python, Linux, and the setuid bit Richard Kettlewell <rjk@greenend.org.uk> - 2014-04-15 09:00 +0100
      Re: Python, Linux, and the setuid bit Chris Angelico <rosuav@gmail.com> - 2014-04-15 18:15 +1000
        Re: Python, Linux, and the setuid bit Richard Kettlewell <rjk@greenend.org.uk> - 2014-04-15 10:28 +0100
          Re: Python, Linux, and the setuid bit Chris Angelico <rosuav@gmail.com> - 2014-04-15 19:35 +1000
      Re: Python, Linux, and the setuid bit Chris Angelico <rosuav@gmail.com> - 2014-04-15 18:18 +1000

#70234 — Python, Linux, and the setuid bit

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!

--
~Ethan~

[1] Need, or really really really convenient to have. ;)
[2] http://selliott.org/python/

[toc] | [next] | [standalone]

#70235

In <mailman.9260.1397511440.18130.python-list@python.org> Ethan Furman <ethan@stoneleaf.us> writes:

>          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.

-- 
John Gordon         Imagine what it must be like for a real medical doctor to
gordon@panix.com    watch 'House', or a real serial killer to watch 'Dexter'.

[toc] | [prev] | [next] | [standalone]

#70236

On 2014-04-14, John Gordon <gordon@panix.com> wrote:
> In <mailman.9260.1397511440.18130.python-list@python.org> Ethan Furman <ethan@stoneleaf.us> writes:
>
>>          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.

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

>>      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.

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               grant.b.edwards        Yow! Please come home with
                                  at               me ... I have Tylenol!!
                              gmail.com            

[toc] | [prev] | [next] | [standalone]

#70237

On 2014-04-14, Grant Edwards <invalid@invalid.invalid> wrote:
> On 2014-04-14, John Gordon <gordon@panix.com> wrote:

>>>      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.
>
> 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));

-- 
Grant Edwards               grant.b.edwards        Yow! My mind is making
                                  at               ashtrays in Dayton ...
                              gmail.com            

[toc] | [prev] | [next] | [standalone]

#70253

Ethan Furman <ethan@stoneleaf.us> writes:
>         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

-- 
http://www.greenend.org.uk/rjk/

[toc] | [prev] | [next] | [standalone]

#70255

On Tue, Apr 15, 2014 at 6:00 PM, Richard Kettlewell <rjk@greenend.org.uk> wrote:
> Ethan Furman <ethan@stoneleaf.us> writes:
>>         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.

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

[toc] | [prev] | [next] | [standalone]

#70264

Chris Angelico <rosuav@gmail.com> writes:
> Richard Kettlewell <rjk@greenend.org.uk> wrote:
>> Ethan Furman <ethan@stoneleaf.us> writes:
>>>         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.
>
> 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.)

-- 
http://www.greenend.org.uk/rjk/

[toc] | [prev] | [next] | [standalone]

#70265

On Tue, Apr 15, 2014 at 7:28 PM, Richard Kettlewell <rjk@greenend.org.uk> wrote:
> 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

[toc] | [prev] | [next] | [standalone]

#70256

On Tue, Apr 15, 2014 at 6:15 PM, Chris Angelico <rosuav@gmail.com> wrote:
> 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

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.python

csiph-web