Menu
▾
▴
lcd4linux-devel
|
From: Michael R. <re...@eu...> - 2004-05-25 20:08:33
|
Hi there, I want lcd4linux not to contain any potential security holes. As we don't have any networking, no client-server-model, no other fancy stuff, this risc is quite low. But lcd4linux is running as root, and therefore potentially dangerous. I'm worrying about a symlink security hole: The Image driver (was: Raster) creates files, without checking if they exist. If a user places a symlink, he may use lcd4linux to overwrite arbitrary files. Which I consider not to be nice. The output file of the raster driver is passed with the '-o path/file' option. To enshure that a potential reader always gets a complete file, the image is first written to a temp file, which will be rename(2)'ed afterwards. I see two problems here: 1. the temp file is opened with O_CREAT. If a symlink is already present, it's target will be overwritten. Some security HOWTO's say one should use O_CREAT|O_EXCL, which means that the call will fail if the file already exists. OTOH, the open(2) man page states that thios doesn't work over NFS :-( 2. the rename() will overwrite a symlink, too. Anybody out there with the experience how to solve such issues? I didn't find too much documentation out there, especially not for exactly the cases we're dealing with here. thanks, Michael -- Michael Reinelt Tel: +43 676 3079941 Geisslergasse 4 Fax: +43 316 692343 A-8045 Graz, Austria e-mail: re...@eu... |
|
From: Xavier V. <xav...@fr...> - 2004-05-25 20:51:26
|
Hello Michael ! [snip] > 1. the temp file is opened with O_CREAT. If a symlink is already > present, it's target will be overwritten. Some security HOWTO's say one > should use O_CREAT|O_EXCL, which means that the call will fail if the > file already exists. OTOH, the open(2) man page states that thios > doesn't work over NFS :-( > 2. the rename() will overwrite a symlink, too. Maybe you should do it like I did in plugin_i2c_sensors to check we have a directory : it did "if ((dir->d_type!=DT_DIR && dir->d_type!=DT_LNK)" to check if it's a dir or a symlink. You should check if it isN'T a symlink but a true file. I don't know how to get this d_type for a special file, I did it with readdir. Bye ! -- Xavier VELLO <xav...@fr...> |
|
From: Michael R. <re...@eu...> - 2004-05-26 06:05:40
|
Hi Xavier,
>>1. the temp file is opened with O_CREAT. If a symlink is already
>>present, it's target will be overwritten. Some security HOWTO's say one
>>should use O_CREAT|O_EXCL, which means that the call will fail if the
>>file already exists. OTOH, the open(2) man page states that thios
>>doesn't work over NFS :-(
>>2. the rename() will overwrite a symlink, too.
>
>
> Maybe you should do it like I did in plugin_i2c_sensors to check we have
> a directory :
> it did "if ((dir->d_type!=DT_DIR && dir->d_type!=DT_LNK)" to check if
> it's a dir or a symlink. You should check if it isN'T a symlink but a
> true file. I don't know how to get this d_type for a special file, I did
> it with readdir.
Yes, that's easy, but contains a race conditions:
if (file exists and/or is a symlink) {
unlink (file)
}
open (file, O_CREAT)
This one looks clean, doesn't it? But there's a small window just before
the open() call, where a hacker could create the symlink, and it's
contents would be overwritten by lcd4linux.
You absolutely _have to_ use atomic operations here.
bye, Michael
--
Michael Reinelt Tel: +43 676 3079941
Geisslergasse 4 Fax: +43 316 692343
A-8045 Graz, Austria e-mail: re...@eu...
|
|
From: Xavier V. <xav...@fr...> - 2004-05-26 11:02:51
|
> Hello Michael !
> > Maybe you should do it like I did in plugin_i2c_sensors to check we have
> > a directory :
> > it did "if ((dir->d_type!=DT_DIR && dir->d_type!=DT_LNK)" to check if
> > it's a dir or a symlink. You should check if it isN'T a symlink but a
> > true file. I don't know how to get this d_type for a special file, I did
> > it with readdir.
>
> Yes, that's easy, but contains a race conditions:
>
> if (file exists and/or is a symlink) {
> unlink (file)
> }
> open (file, O_CREAT)
>
> This one looks clean, doesn't it? But there's a small window just before
> the open() call, where a hacker could create the symlink, and it's
> contents would be overwritten by lcd4linux.
> You absolutely _have to_ use atomic operations here.
Or you should do
if(file is a symlink) {
error and quit
}
before writing to the file. So, there's no problem anymore. Moreover, I
don't think the user is able to create a symlink within the small delay
between the if and the open.
Bye !
--
Xavier VELLO <xav...@fr...>
|
|
From: Michael R. <re...@eu...> - 2004-05-26 12:15:07
|
Hi,
>>Yes, that's easy, but contains a race conditions:
>>
>>if (file exists and/or is a symlink) {
>> unlink (file)
>>}
>>open (file, O_CREAT)
>>
>>This one looks clean, doesn't it? But there's a small window just before
>>the open() call, where a hacker could create the symlink, and it's
>>contents would be overwritten by lcd4linux.
>>You absolutely _have to_ use atomic operations here.
>
> Or you should do
> if(file is a symlink) {
> error and quit
> }
> before writing to the file. So, there's no problem anymore.
Well, this may be a clean solution. BUT the same race condition is here:
if(file is a symlink) {
error and quit
}
/* possible race condition here */
open (file, O_CREAT)
> Moreover, I
> don't think the user is able to create a symlink within the small delay
> between the if and the open.
Believe me, he is. He could try to do that i a loop, create some
tousands of symlinks a second. Then he just has to wait...
Your approach is one of the reasons for at least some of the security
holes out there.
http://en.tldp.org/HOWTO/Secure-Programs-HOWTO/avoid-race.html
bye, Michael
--
Michael Reinelt Tel: +43 676 3079941
Geisslergasse 4 Fax: +43 316 692343
A-8045 Graz, Austria e-mail: re...@eu...
|
|
From: Jerry S. <ye...@th...> - 2004-05-31 06:01:43
|
On Tue, May 25, 2004 at 10:08:18PM +0200, Michael Reinelt wrote: > Hi there, > > I want lcd4linux not to contain any potential security holes. As we > don't have any networking, no client-server-model, no other fancy stuff, > this risc is quite low. But lcd4linux is running as root, and therefore > potentially dangerous. > > I'm worrying about a symlink security hole: The Image driver (was: > Raster) creates files, without checking if they exist. If a user places > a symlink, he may use lcd4linux to overwrite arbitrary files. Which I > consider not to be nice. > > The output file of the raster driver is passed with the '-o path/file' > option. To enshure that a potential reader always gets a complete file, > the image is first written to a temp file, which will be rename(2)'ed > afterwards. > > I see two problems here: > > 1. the temp file is opened with O_CREAT. If a symlink is already > present, it's target will be overwritten. Some security HOWTO's say one > should use O_CREAT|O_EXCL, which means that the call will fail if the > file already exists. OTOH, the open(2) man page states that thios > doesn't work over NFS :-( > > 2. the rename() will overwrite a symlink, too. > > > Anybody out there with the experience how to solve such issues? I didn't > find too much documentation out there, especially not for exactly the > cases we're dealing with here. There are a few things you can try, each with their own limitations... 1. Use mkstemp() to create the filename. With a random filename, it becomes much harder for a hacker to misuse the file. I don't know if or how you can get the filename for the file created by mkstemp(). 2. Write the file to a directory owned by root. /tmp can be a bad place for secure files because anyone can write to it. Then the rename technique would work. However, I think that your solution above is reasonable. It is pretty much assumed that if you want a secure setup you won't use NFS. Furthermore, it is not common practice to use NFS for /tmp. As far as using a symlink to subvert the rename operation, there really isn't anything you can do about that. It is a sysadmin issue. The program is run by root and shouldn't be writing to a directory that other users also have write access to. Jerry -- Jerry Seutter Email: ye...@th... Web: http://www.thegeeks.net/~yello Gallery: www.thegeeks.net/~yello/gallery (email me for username and password). |
|
From: Michael R. <re...@eu...> - 2004-06-02 05:19:10
|
Hi Jerry, > There are a few things you can try, each with their own limitations... > > 1. Use mkstemp() to create the filename. With a random filename, it > becomes much harder for a hacker to misuse the file. I don't know if or > how you can get the filename for the file created by mkstemp(). I don't want to create the file in the temp directory, but in the same dir as the output file (so that the following rename() call must not copy any data) > 2. Write the file to a directory owned by root. /tmp can be a bad place > for secure files because anyone can write to it. Then the rename > technique would work. That's not that easy, too: I want to write the file the user specifies with the -o option. But I think I've already fixed this: I'm calling unlink() just before the open() to remove the file if it exists. The open() is called with the O_EXCL flag, which means it will fail if the file existed. As we've unlinked it just before, this should never happen. If it happens, I emit an error and abort. This looks secure enough to mee. bye, Michael -- Michael Reinelt Tel: +43 676 3079941 Geisslergasse 4 Fax: +43 316 692343 A-8045 Graz, Austria e-mail: re...@eu... |
|
From: Jerry S. <ye...@th...> - 2004-06-08 01:30:49
|
On Wed, Jun 02, 2004 at 07:19:02AM +0200, Michael Reinelt wrote: > Hi Jerry, > But I think I've already fixed this: I'm calling unlink() just before > the open() to remove the file if it exists. The open() is called with > the O_EXCL flag, which means it will fail if the file existed. As we've > unlinked it just before, this should never happen. If it happens, I emit > an error and abort. > > This looks secure enough to mee. Agreed. Failing out would prevent overwriting other files. Jerry -- Jerry Seutter Email: ye...@th... Web: http://www.thegeeks.net/~yello Gallery: www.thegeeks.net/~yello/gallery (email me for username and password). |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X