Proper /proc/pid/cmdline behavior when command line is corrupt?

Proper /proc/pid/cmdline behavior when command line is corrupt?

[Edward Falk]

Hi all; there's a few lines of code in fs/proc/base.c:proc_pid_cmdline() 
that I'm unable to make sense of.  There are a few lines that check the 
returned buffer to see if it's properly nul-terminated.  If not, the 
code assumes the user has overwritten and corrupted the command line buffer.

The next step is to search for the first embedded nul, and truncate the 
buffer at that point.

If no embedded nul is found, enough data is copied from the user's 
environment to fill the buffer.  Another search for an embedded nul is 
then made.

Does anybody know what on earth this code is trying to accomplish?  Is 
this the intended behavior?  The best I can guess is that the user is 
assumed to have overwritten the end of the command line buffer and that 
the environment buffer is assumed to immediately follow the command line 
buffer.

I'm currently working on a patch that removes the one page limit on the 
returned command line buffer but I'm not convinced I should retain this 
behavior.  Is it possible that there's any code out there that depends 
on this behavior.  It would help if I knew why it was done this way.

TIA,
	-ed falk, efalk@google.com

Re: Proper /proc/pid/cmdline behavior when command line is corrupt?

[Jan Engelhardt]

Hi Edward,


> there's a few lines of code in fs/proc/base.c:proc_pid_cmdline() that
> I'm unable to make sense of.  There are a few lines that check the
> returned buffer to see if it's properly nul-terminated.  If not, the
> code assumes the user has overwritten and corrupted the command line
> buffer.
>
> The next step is to search for the first embedded nul, and truncate
> the buffer at that point.
>
> If no embedded nul is found, enough data is copied from the user's
> environment to fill the buffer.  Another search for an embedded nul
> is then made.
>
> Does anybody know what on earth this code is trying to accomplish? 
> Is this the intended behavior?  The best I can guess is that the user
> is assumed to have overwritten the end of the command line buffer and
> that the environment buffer is assumed to immediately follow the
> command line buffer.

The environment buffer is not assumed to be there, it is _known_ to come right
after the argument string, because that is how the kernel sets it up on execve
(for x86 at least). GDB verifies this:

(gdb) b main
(gdb) r 123
(gdb) p *argv[0]@3072
$3 = "/dev/shm/a.out\000123\000LESSKEY=/etc/lesskey.bin\000GREP_COLOR=1
\000MANPATH=/usr/local/man:/usr/share/man:/usr/X11R6/man:/opt/gnome/sha
re/man\000INFODIR=/usr/local/info:/usr/share/info:/usr/info\000NNTPSERV
ER=news\000SSH"...

$3 = "/dev/shm/a.out\000123\000LESSKEY=/etc/lesskey.bin\000GREP_COLOR=1
      ^                 ^      ^                           ^
      argv[0]           argv[1]envp[0]                     envp[1]
\000MANPATH=/usr/local/man:/usr/share/man:/usr/X11R6/man:/opt/gnome/sha
re/man\000INFODIR=/usr/local/info:/usr/share/info:/usr/info\000NNTPSERV
ER=news\000SSH"...

As you can see above, libc will set up the ARGV and ENVP arrays with
pointers to the respective strings. Writing over the end of the ENVP
string will cause

  - a segfault in the user program
  - a "cannot access that memory" in GDB
  - more bad things, if done in kernelspace


> I'm currently working on a patch that removes the one page limit on
> the returned command line buffer but I'm not convinced I should
> retain this behavior.

I think yes. proc_pid_cmdline() has these lines:

	len = mm->arg_end - mm->arg_start
  *	if (len > PAGE_SIZE)
  *		len = PAGE_SIZE;
	res = access_process_vm(task, mm->arg_start, buffer, len, 0);


and @buffer is allocated in the caller as only one page:


static ssize_t proc_info_read(struct file * file, char __user * buf,            
                          size_t count, loff_t *ppos)                           
{                                                                               
	...
        if (!(page = __get_free_page(GFP_KERNEL)))
                return -ENOMEM;
	length = PROC_I(inode)->op.proc_read(task, (char*)page);                
	...
}


> Is it possible that there's any code out there
> that depends on this behavior.  It would help if I knew why it was
> done this way.




Jan Engelhardt
-- 

Re: Proper /proc/pid/cmdline behavior when command line is corrupt?

[Edward Falk]

Jan Engelhardt wrote:
> Hi Edward,

>>that the environment buffer is assumed to immediately follow the
>>command line buffer.
> 
> 
> The environment buffer is not assumed to be there, it is _known_ to come right
> after the argument string, because that is how the kernel sets it up on execve
> (for x86 at least).

Is that in a spec somewhere?  Otherwise, I would argue that it isn't 
_known_ to come right after the argument string, it just _happens_ to 
come right after the argument string.  This could change in future kernels.



>>I'm currently working on a patch that removes the one page limit on
>>the returned command line buffer but I'm not convinced I should
>>retain this behavior.
> 
> 
> I think yes. proc_pid_cmdline() has these lines:
> 
> 	len = mm->arg_end - mm->arg_start
>   *	if (len > PAGE_SIZE)
>   *		len = PAGE_SIZE;
> 	res = access_process_vm(task, mm->arg_start, buffer, len, 0);
> 
> 
> and @buffer is allocated in the caller as only one page:

True, but that's an arbitrary limitation which I'm in the process of 
removing.  I have a new version of proc_pid_cmdline() which will return 
the entire commandline buffer no matter what its length.  If the 
grab-more-data-from-environment-buffer behavior is actually broken, I'd 
rather not propagate it to the new code.

	-ed falk

Re: Proper /proc/pid/cmdline behavior when command line is corrupt?

[Jan Engelhardt]

Hi Edward,


>>> that the environment buffer is assumed to immediately follow the
>>> command line buffer.
>> 
>> The environment buffer is not assumed to be there, it is _known_ to
>> come right after the argument string, because that is how the kernel
>> sets it up on execve (for x86 at least).
>
> Is that in a spec somewhere?

http://www.x86-64.org/documentation/abi.pdf#search=%22argv%20envp%20x86%20ABI%22
page 29 figure 3.9 lists the data block in question as "Information
block, including argument strings, environment strings, auxiliary
information", but does not specify it further, like how it is laid out.

What it does mention is "argument pointers" (aka argv[N]) and their
exact position. In fact, right below the figure is the explanation:
"Argument strings, environment strings, and the auxiliary information
appear in no specific order within the information block and they need
not be compactly allocated."

What this means: we are guaranteed that argv[N] is valid, but that not
that the memory range [&argv[0][0] .. &argv[X][Y]] is contiguous. In
fact, if the argument string was not contiguous, but, let's say, have a
map hole, using setproctitle() will inevidently lead to a SIGSEGV.
Probably that is the reason why Linux/glibc do not come with a
setproctitle(3) function. I have taken a look at sendmail, which ships
its own setproctitle (for this very reason), and a source code comment
says it will only use contiguous argv pointers, but I have not looked
at it more closely.

> Otherwise, I would argue that it isn't _known_ to come right after
> the argument string, it just _happens_ to come right after the
> argument string.  This could change in future kernels.

Yes you are right. It may happen that the argument strings come _after_
the environment. Or something even different. In practice that would 
mean that writing over the end of the argument string will write into 
other vital structures, possibly crashing the userspace program. Or if 
not that, it will display garbage in the command line.

What that means for your future patch:

The way how the arg and env strings are laid out are, as far as I can 
tell, defined in fs/binfmt_elf.c:create_elf_tables(). And 
proc_pid_cmdline() depends on this layout, yes. However, since the 
layout is not used anywhere outside the kernel (so says the PDF), there 
should not be a problem. If you modify the layout, make sure it is 
consistent within the kernel. It is unspecified for userspace, and a 
user program accessing this area nonetheless is doing so at its own risk.

Hope this helps.


Regards,
Jan Engelhardt
-- 

Re: Proper /proc/pid/cmdline behavior when command line is corrupt?

[Edward Falk]

Jan Engelhardt wrote:
> 
> http://www.x86-64.org/documentation/abi.pdf#search=%22argv%20envp%20x86%20ABI%22
> page 29 figure 3.9 lists the data block in question as "Information
> block, including argument strings, environment strings, auxiliary
> information", but does not specify it further, like how it is laid out.
> 
> What it does mention is "argument pointers" (aka argv[N]) and their
> exact position. In fact, right below the figure is the explanation:
> "Argument strings, environment strings, and the auxiliary information
> appear in no specific order within the information block and they need
> not be compactly allocated."

...

Thank you for sending me that document.  It seems that the bottom line 
is that the environment-follows-the-commandline assumption is *not* 
valid for future kernels, and may well not be valid for other 
architectures either.

I would suggest that for an application to overwrite the end of its own 
commandline buffer is undefined behavior.*

I'd also further suggest that the current implementation of 
proc_pid_cmdline() is essentially _guessing_ that the user has 
overwritten the end of the buffer and also guessing that the extra data 
can be found in the environment buffer.

Further, if a terminating nul still can't be found in the leading part 
of the environment buffer, proc_pid_cmdline() arbitrarily truncates the 
return value at the one-page mark with no attempt to insert a 
terminating nul.  It seems to me that if we accept that it's ok to 
arbitrarily truncate the return value, then a better choice of 
truncation point would be the end of the commandline buffer.

In addition, since the code is looking for missing data in the 
environment buffer, we can reasonably assume that the user has 
inadvertantly and hopelessly corrupted their own environment.  I submit 
that in this case, all bets are off and it doesn't really matter *what* 
we do at this point -- the results are undefined and can't possibly be 
correct.


> What that means for your future patch:
> 
> The way how the arg and env strings are laid out are, as far as I can 
> tell, defined in fs/binfmt_elf.c:create_elf_tables(). And 
> proc_pid_cmdline() depends on this layout, yes. However, since the 
> layout is not used anywhere outside the kernel (so says the PDF), there 
> should not be a problem. If you modify the layout, make sure it is 
> consistent within the kernel. It is unspecified for userspace, and a 
> user program accessing this area nonetheless is doing so at its own risk.

Thank you for pointing this out.  I'm not planning to change the layout, 
but perhaps a comment should be added to create_elf_tables() warning 
that proc_pid_cmdline() will need to be modified if the layout is ever 
modified.

Anyway; does anybody know why the original code was done this way, or of 
any applications that depend on that behavior?


> Hope this helps.

Immensely.  Thank you.

	-ed falk


*Actually, by the looks of things, for a process to write to its own 
commandline buffer *at all* is undefined behavior, since the spec makes 
no guarantees of the layout of the information block.

Re: Proper /proc/pid/cmdline behavior when command line is corrupt?

[Jan Engelhardt]

Hi Edward,


> Thank you for sending me that document.  It seems that the bottom
> line is that the environment-follows-the-commandline assumption is
> *not* valid for future kernels, and may well not be valid for other
> architectures either.

Yes, one cannot rely on that across architectures either. But, in
practice, binfmt_elf.c is used by all architectures that support ELF,
so it is likely that the env-follows-arg is consistent across ELF
programs at least. And that is a Good Thing(tm), since otherwise
proc_pid_cmdline() would need to consider every possible layout.

On the other side, binfmt_aout.c for example might do something
completely different. Luckily it does the same compact placing as
binfmt_elf.c, so proc_pid_cmdline() should also work with AOUT
programs.

> I would suggest that for an application to overwrite the end of its
> own commandline buffer is undefined behavior.*

On Linux, yes. Since the setproctitle(3) call has originated from BSD,
things might be a little different there.

> I'd also further suggest that the current implementation of
> proc_pid_cmdline() is essentially _guessing_ that the user has
> overwritten the end of the buffer and also guessing that the extra
> data can be found in the environment buffer.

Well, the environment string is the only place where extra data can go
at the moment. If you place you arguments somewhere else, then they are
not part of the argument string, and would not show up in `ps`.

> Further, if a terminating nul still can't be found in the leading
> part of the environment buffer, proc_pid_cmdline() arbitrarily
> truncates the return value at the one-page mark with no attempt to
> insert a terminating nul.  It seems to me that if we accept that it's
> ok to arbitrarily truncate the return value, then a better choice of
> truncation point would be the end of the commandline buffer.
>
> In addition, since the code is looking for missing data in the
> environment buffer, we can reasonably assume that the user has
> inadvertantly and hopelessly corrupted their own environment.

setproctitle(), as provided by sendmail, copies the environment to the
heap before attempting to overwrite the argument and (possibly) the
environment string. Other implementations might not be so careful,
though.

> I submit that in this case, all bets are off and it doesn't really
> matter *what* we do at this point -- the results are undefined and
> can't possibly be correct.
>
>
>> What that means for your future patch:
>> 
>> The way how the arg and env strings are laid out are, as far as I can
>> tell, defined in fs/binfmt_elf.c:create_elf_tables(). And
>> proc_pid_cmdline() depends on this layout, yes. However, since the layout
>> is not used anywhere outside the kernel (so says the PDF), there should
>> not be a problem. If you modify the layout, make sure it is consistent
>> within the kernel. It is unspecified for userspace, and a user program
>> accessing this area nonetheless is doing so at its own risk.
>
> Thank you for pointing this out.  I'm not planning to change the layout, but
> perhaps a comment should be added to create_elf_tables() warning that
> proc_pid_cmdline() will need to be modified if the layout is ever modified.
>
> Anyway; does anybody know why the original code was done this way, or of any
> applications that depend on that behavior?

Nothing should break if you make the PAGE_SIZE restriction go away,
since it is only procfs that limits the output to PAGE_SIZE. A program
can still access its full argv -- configure scripts sometimes check for
the maximum length of the argument string, and show that it can be up
to 128KB, a lot more than just one page.



Jan Engelhardt
--