To: Albert Cahalan <acahalan@gmail.com>
Cc: Arjan van de Ven <arjan@infradead.org>,
       "Albert D. Cahalan" <acahalan@cs.uml.edu>,
       "Jakub Jelinek" <jakub@redhat.com>, Al Viro <viro@ftp.linux.org.uk>,
       linux-kernel@vger.kernel.org, akpm@osdl.org
Subject: Re: [PATCH 4/4] pmap: reduced permissions
References: <200601222219.k0MMJ3Qg209555@saturn.cs.uml.edu>
	<1137996654.2977.0.camel@laptopd505.fenrus.org>
	<787b0d920601230128o5a12513fjae3708e3fb552dca@mail.gmail.com>
	<1138009305.2977.28.camel@laptopd505.fenrus.org>
	<787b0d920601230220r5c7df60dk142d1d637ab4ed48@mail.gmail.com>
	<87r76vrhsj.fsf@amaterasu.srvr.nix>
	<787b0d920601251745n72811696p129396f1279a4a82@mail.gmail.com>
From: Nix <nix@esperi.org.uk>
Date: Thu, 26 Jan 2006 07:54:49 +0000
In-Reply-To: <787b0d920601251745n72811696p129396f1279a4a82@mail.gmail.com> (Albert
 Cahalan's message of "Wed, 25 Jan 2006 20:45:48 -0500")
Message-ID: <87r76vpgom.fsf@amaterasu.srvr.nix>
User-Agent: Gnus/5.1006 (Gnus v5.10.6) XEmacs/21.4 (Corporate Culture,
 linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4222
Lines: 97

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/