[PATCH 4/4] pmap: reduced permissions

[PATCH 4/4] pmap: reduced permissions

[Albert D. Cahalan]

This patch changes all 3 remaining maps files to be readable
only for the file owner. There have been privacy concerns.

Fedora Core 4 has been shipping with such permissions on
the /proc/*/maps file already. General system monitoring
tools seldom use these files.

Signed-off-by: Albert Cahalan <acahalan@gmail.com>

---

This applies to -git4, grabbed Saturday night.


diff -Naurd 3/fs/proc/base.c 4/fs/proc/base.c
--- 3/fs/proc/base.c	2006-01-22 15:23:13.000000000 -0500
+++ 4/fs/proc/base.c	2006-01-22 15:44:16.000000000 -0500
@@ -202,7 +202,7 @@
 	E(PROC_TGID_EXE,       "exe",     S_IFLNK|S_IRWXUGO),
 	E(PROC_TGID_MOUNTS,    "mounts",  S_IFREG|S_IRUGO),
 #ifdef CONFIG_MMU
-	E(PROC_TGID_PMAP,      "pmap",   S_IFREG|S_IRUGO),
+	E(PROC_TGID_PMAP,      "pmap",   S_IFREG|S_IRUSR),
 #endif
 #ifdef CONFIG_SECURITY
 	E(PROC_TGID_ATTR,      "attr",    S_IFDIR|S_IRUGO|S_IXUGO),
@@ -231,9 +231,9 @@
 	E(PROC_TID_CMDLINE,    "cmdline", S_IFREG|S_IRUGO),
 	E(PROC_TID_STAT,       "stat",    S_IFREG|S_IRUGO),
 	E(PROC_TID_STATM,      "statm",   S_IFREG|S_IRUGO),
-	E(PROC_TID_MAPS,       "maps",    S_IFREG|S_IRUGO),
+	E(PROC_TID_MAPS,       "maps",    S_IFREG|S_IRUSR),
 #ifdef CONFIG_NUMA
-	E(PROC_TID_NUMA_MAPS,  "numa_maps",    S_IFREG|S_IRUGO),
+	E(PROC_TID_NUMA_MAPS,  "numa_maps",    S_IFREG|S_IRUSR),
 #endif
 	E(PROC_TID_MEM,        "mem",     S_IFREG|S_IRUSR|S_IWUSR),
 #ifdef CONFIG_SECCOMP

Re: [PATCH 4/4] pmap: reduced permissions

[Arjan van de Ven]

On Sun, 2006-01-22 at 17:19 -0500, Albert D. Cahalan wrote:
> This patch changes all 3 remaining maps files to be readable
> only for the file owner. There have been privacy concerns.
> 
> Fedora Core 4 has been shipping with such permissions on
> the /proc/*/maps file already. General system monitoring
> tools seldom use these files.

changing /maps to 0400 breaks glibc; there are cases where this would
lead to /proc/self/maps to be not readable (setuid like apps) so this
needs a more elaborate fix.

Re: [PATCH 4/4] pmap: reduced permissions

[Albert Cahalan]

On 1/23/06, Arjan van de Ven <arjan@infradead.org> wrote:
> On Sun, 2006-01-22 at 17:19 -0500, Albert D. Cahalan wrote:
> > This patch changes all 3 remaining maps files to be readable
> > only for the file owner. There have been privacy concerns.
> >
> > Fedora Core 4 has been shipping with such permissions on
> > the /proc/*/maps file already. General system monitoring
> > tools seldom use these files.
>
> changing /maps to 0400 breaks glibc; there are cases where this would
> lead to /proc/self/maps to be not readable (setuid like apps) so this
> needs a more elaborate fix.

Wow. Well, that's why I put the patch last in the series.
The other 3 don't depend on it at all.

I tend to think that glibc should not be reading this file.
What excuse is there?

In any case, the many existing statically linked executables
do cause trouble. Setuid apps are the ones you'd most want
to protect. Allowing them to read their own files can cause
plenty of trouble; perhaps you remember the XFree86 config
file bug that exposed the content of files that were not meant
to be readable.

On the other hand, these apps are the ones you'd most want
to recompile with modern tools. (PIE executable, stack canary,
no-execute stack flag, etc.)

I'm actually surprised that processes don't always get to read
their own files. If somebody hacks this up, be sure to base it
on the tgid. (or, better, on the struct mm)

Re: [PATCH 4/4] pmap: reduced permissions

[Arjan van de Ven]

On Mon, 2006-01-23 at 04:28 -0500, Albert Cahalan wrote:
> On 1/23/06, Arjan van de Ven <arjan@infradead.org> wrote:
> > On Sun, 2006-01-22 at 17:19 -0500, Albert D. Cahalan wrote:
> > > This patch changes all 3 remaining maps files to be readable
> > > only for the file owner. There have been privacy concerns.
> > >
> > > Fedora Core 4 has been shipping with such permissions on
> > > the /proc/*/maps file already. General system monitoring
> > > tools seldom use these files.
> >
> > changing /maps to 0400 breaks glibc; there are cases where this would
> > lead to /proc/self/maps to be not readable (setuid like apps) so this
> > needs a more elaborate fix.
> 
> Wow. Well, that's why I put the patch last in the series.
> The other 3 don't depend on it at all.
> 
> I tend to think that glibc should not be reading this file.
> What excuse is there?

glibc needs to be able to find out if a certain address is writable. (eg
mapped "w"). The only way available for that is... reading the maps
file.


> In any case, the many existing statically linked executables
> do cause trouble. Setuid apps are the ones you'd most want
> to protect.

for this 0400 isn't enough; because you can open this file, send the fd
over a unix socket, and then exec. The process you sent the fd to can
then read the setuid's program maps file. 

This thing is all a bit more complex than just the file mode ;(

Re: [PATCH 4/4] pmap: reduced permissions

[Albert Cahalan]

On 1/23/06, Arjan van de Ven <arjan@infradead.org> wrote:
> On Mon, 2006-01-23 at 04:28 -0500, Albert Cahalan wrote:

> > I tend to think that glibc should not be reading this file.
> > What excuse is there?
>
> glibc needs to be able to find out if a certain address is writable. (eg
> mapped "w"). The only way available for that is... reading the maps
> file.

What the heck for? That's gross.

If glibc is just providing this info for apps, there should be a
system call for it. Otherwise, being the C library, glibc can
damn well remember what it did.

> > In any case, the many existing statically linked executables
> > do cause trouble. Setuid apps are the ones you'd most want
> > to protect.
>
> for this 0400 isn't enough;

because you might fool the app into reading it

> because you can open this file, send the fd
> over a unix socket, and then exec. The process you sent the fd to can
> then read the setuid's program maps file.

You exec what, the setuid executable? For other reasons,
that needs to sever all file descriptors to the /proc files.
They should be returning EBADF for all operations.

Oh dear. I think I see why /proc/*/mem reads are far too
restricted. The file descripters are NOT getting severed???
Hmmm, I'm not finding code to sever them.

Well, that's part of a general problem then, including lack
of the revoke() system call that BSD introduced. This hits
hard with device files. Memory mappings get interesting,
though /dev/zero might make a nice substitute mapping.

Re: [PATCH 4/4] pmap: reduced permissions

[Nix]

On 23 Jan 2006, Albert Cahalan said:
> On 1/23/06, Arjan van de Ven <arjan@infradead.org> wrote:
>> On Mon, 2006-01-23 at 04:28 -0500, Albert Cahalan wrote:
> 
>> > I tend to think that glibc should not be reading this file.
>> > What excuse is there?
>>
>> glibc needs to be able to find out if a certain address is writable. (eg
>> mapped "w"). The only way available for that is... reading the maps
>> file.
> 
> What the heck for? That's gross.

Ironically enough, it's security. :)

> If glibc is just providing this info for apps, there should be a
> system call for it. Otherwise, being the C library, glibc can
> damn well remember what it did.

Nah, it's used for vfprintf() argument-area checking.

Specifically, it's the Linux implementation of __readonly_area(),
located in sysdeps/unix/sysv/linux/readonly-area.c in glibc-3.4-to-be,
and used by vfprintf() on behalf of __vfprintf_chk(). Calls to this
function (and the other __*_chk() functions) are expanded by glibc's
string headers and generated by GCC 4.1+ automatically when possible
(and by GCCs out there in the field: this patch is shipped by RH
already, known as FORTIFY_SOURCE).

FORTIFY_SOURCE zaps a whole class of security vulnerabilities stone
dead. Breaking it would be a bad idea. Therefore, /proc/self/maps has to
remain readable, even in setuid programs, because setuid programs are
one class of programs for which FORTIFY_SOURCE is crucial.

[Jakub added to Cc:, he can defend his own code much better than I can]

-- 
`Everyone has skeletons in the closet.  The US has the skeletons
 driving living folks into the closet.' --- Rebecca Ore

Re: [PATCH 4/4] pmap: reduced permissions

[Albert Cahalan]

On 1/25/06, Nix <nix@esperi.org.uk> wrote:
> On 23 Jan 2006, Albert Cahalan said:
> > On 1/23/06, Arjan van de Ven <arjan@infradead.org> wrote:
> >> On Mon, 2006-01-23 at 04:28 -0500, Albert Cahalan wrote:
> >
> >> > I tend to think that glibc should not be reading this file.
> >> > What excuse is there?
> >>
> >> glibc needs to be able to find out if a certain address is writable. (eg
> >> mapped "w"). The only way available for that is... reading the maps
> >> file.
> >
> > What the heck for? That's gross.
>
> Ironically enough, it's security. :)
>
> > If glibc is just providing this info for apps, there should be a
> > system call for it. Otherwise, being the C library, glibc can
> > damn well remember what it did.
>
> Nah, it's used for vfprintf() argument-area checking.
>
> Specifically, it's the Linux implementation of __readonly_area(),
> located in sysdeps/unix/sysv/linux/readonly-area.c in glibc-3.4-to-be,
> and used by vfprintf() on behalf of __vfprintf_chk(). Calls to this
> function (and the other __*_chk() functions) are expanded by glibc's
> string headers and generated by GCC 4.1+ automatically when possible
> (and by GCCs out there in the field: this patch is shipped by RH
> already, known as FORTIFY_SOURCE).
>
> FORTIFY_SOURCE zaps a whole class of security vulnerabilities stone
> dead. Breaking it would be a bad idea. Therefore, /proc/self/maps has to
> remain readable, even in setuid programs, because setuid programs are
> one class of programs for which FORTIFY_SOURCE is crucial.
>
> [Jakub added to Cc:, he can defend his own code much better than I can]

OK, Jakub, how would you like the system call to look? :-)
It looks like the mincore() system call has reserved bits
available in the output vector.

It's just vfprintf? Not vsprintf too? I'll take a guess that the
performance hit was considered tolerable only if doing IO
anyway. A proper system call would help both cases.

It's bad enough that procps has to suffer the overhead of
parsing all that nasty text. The thought of every app doing
that, automatically via gcc+glibc, is truly horrifying.

Re: [PATCH 4/4] pmap: reduced permissions

[Arjan van de Ven]

> It's bad enough that procps has to suffer the overhead of
> parsing all that nasty text. The thought of every app doing
> that, automatically via gcc+glibc, is truly horrifying.

well it's rare. The %n printf argument is... almost never used. 

Re: [PATCH 4/4] pmap: reduced permissions

[Nix]

FOn Wed, 25 Jan 2006, Albert Cahalan murmured woefully:
> On 1/25/06, Nix <nix@esperi.org.uk> wrote:
>> On 23 Jan 2006, Albert Cahalan said:
>> > On 1/23/06, Arjan van de Ven <arjan@infradead.org> wrote:
>> >> On Mon, 2006-01-23 at 04:28 -0500, Albert Cahalan wrote:
>> >
>> >> > I tend to think that glibc should not be reading this file.
>> >> > What excuse is there?
>> >>
>> >> glibc needs to be able to find out if a certain address is writable. (eg
>> >> mapped "w"). The only way available for that is... reading the maps
>> >> file.
>> >
>> > What the heck for? That's gross.
>>
>> Ironically enough, it's security. :)
>>
>> > If glibc is just providing this info for apps, there should be a
>> > system call for it. Otherwise, being the C library, glibc can
>> > damn well remember what it did.
>>
>> Nah, it's used for vfprintf() argument-area checking.
>>
>> Specifically, it's the Linux implementation of __readonly_area(),
>> located in sysdeps/unix/sysv/linux/readonly-area.c in glibc-3.4-to-be,
>> and used by vfprintf() on behalf of __vfprintf_chk(). Calls to this
>> function (and the other __*_chk() functions) are expanded by glibc's
>> string headers and generated by GCC 4.1+ automatically when possible
>> (and by GCCs out there in the field: this patch is shipped by RH
>> already, known as FORTIFY_SOURCE).
>>
>> FORTIFY_SOURCE zaps a whole class of security vulnerabilities stone
>> dead. Breaking it would be a bad idea. Therefore, /proc/self/maps has to
>> remain readable, even in setuid programs, because setuid programs are
>> one class of programs for which FORTIFY_SOURCE is crucial.
>>
>> [Jakub added to Cc:, he can defend his own code much better than I can]

I screwed up the From line, so Al got it and Jakub didn't. Fixed.
(Some extra quoting left in for context.)

> OK, Jakub, how would you like the system call to look? :-)
> It looks like the mincore() system call has reserved bits
> available in the output vector.
> 
> It's just vfprintf? Not vsprintf too? I'll take a guess that the
> performance hit was considered tolerable only if doing IO
> anyway. A proper system call would help both cases.

It's everything that transitively calls glibc's vfprintf() call, which
is almost the whole printf() family. That is to say, vfprintf(),
printf(), vprintf(), vfwprintf()... and more, but the inclusion
of printf() is enough.

> It's bad enough that procps has to suffer the overhead of
> parsing all that nasty text. The thought of every app doing
> that, automatically via gcc+glibc, is truly horrifying.

It's only called for i18ned stuff using the positional parameter
specifiers, like %.2$s... it's to stop people passing something like
%.500000$s; glibc, of course, can't tell where the parameter list ends
without GCC support, and even *with* that support it can't tell for
every case (e.g. for direct calls to vfprintf()); so the heuristic
that's used is that valid values of those parameters must necessarily
fall into space which is not writable (as you can't write to a code
segment that you're currently executing). This stops format string
attacks from being *too* devastating, one hopes; attackers can't spy on
the program's data that way.

It does handle failure cases, viz

  if (fp == NULL)
    {
      /* It is the system administrator's choice to not have /proc
         available to this process (e.g., because it runs in a chroot
         environment.  Don't fail in this case.  */
      if (errno == ENOENT
          /* The kernel has a bug in that a process is denied access
             to the /proc filesystem if it is set[ug]id.  There has
             been no willingness to change this in the kernel so
             far.  */
          || errno == EACCES)
        return 1;
      return -1;
    }

but of course it would be better if that latter failure case wasn't
needed in future.

-- 
`Everyone has skeletons in the closet.  The US has the skeletons
 driving living folks into the closet.' --- Rebecca Ore