Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 159556 - sys-block/partimage: insecure temporary file creation
Summary: sys-block/partimage: insecure temporary file creation
Status: RESOLVED FIXED
Alias: None
Product: Gentoo Security
Classification: Unclassified
Component: Vulnerabilities (show other bugs)
Hardware: All Linux
: High minor (vote)
Assignee: Gentoo Security
URL:
Whiteboard: B3 [glsa]
Keywords:
Depends on:
Blocks:
 
Reported: 2006-12-31 03:09 UTC by Tavis Ormandy (RETIRED)
Modified: 2022-03-18 13:20 UTC (History)
3 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
Patch to fix hardcoded "FINISH_LAST_COUNTFILE" in /tmp (partimage-0.6.4-mkstemp-fix.patch,3.54 KB, patch)
2007-03-16 19:21 UTC, Pierre-Yves Rofes (RETIRED)
no flags Details | Diff
Patch to fix "FINISH_LAST_COUTNFILE" in /tmp (partimage-mkstemp-fix-v2.patch,3.77 KB, patch)
2007-04-05 18:32 UTC, Pierre-Yves Rofes (RETIRED)
no flags Details | Diff
final? fix for the mkstemp problem (partimage-mkstemp-fix-v3.patch,1.37 KB, patch)
2008-12-26 10:31 UTC, Matti Bickel (RETIRED)
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tavis Ormandy (RETIRED) gentoo-dev 2006-12-31 03:09:21 UTC
partimage creates the file /tmp/partimage.count insecurely (FINISH_LAST_COUNTFILE) (note: it creates the lockfile securely, although using a hardcoded name like this obviously prevents multiple simulatneous uses)

Ideally the countfile code should be using mkstemp.

Strangely, there appears to be an undocumented `--runshell` option that executes a file in /tmp, obviously this is a terrible idea and support for this option should be #if 0'ed out.

Ideally the countfile code should be using mkstemp.

patch will be attached shortly.
Comment 1 Sune Kloppenborg Jeppesen (RETIRED) gentoo-dev 2007-01-06 12:35:42 UTC
Tavis do you have a patch for this?
Comment 2 Matthias Geerdsen (RETIRED) gentoo-dev 2007-03-16 15:22:22 UTC
anyone got a patch for this?
Comment 3 Pierre-Yves Rofes (RETIRED) gentoo-dev 2007-03-16 19:21:56 UTC
Created attachment 113491 [details, diff]
Patch to fix hardcoded "FINISH_LAST_COUNTFILE" in /tmp

I think this patch should do the trick.
Comment 4 Sune Kloppenborg Jeppesen (RETIRED) gentoo-dev 2007-03-25 10:43:29 UTC
Pulling in herd for advise.
Comment 5 SpanKY gentoo-dev 2007-03-25 11:12:33 UTC
that patch is weird

mkstemp() fails yet you return EXIT_SUCCESS ?

mkstemp() returns the file descriptor, yet you turn around and run open() on the tempfile a second time ?

you probably want:
nLockFile = mkstemp(...);
if (nLockFile == -1) ...

and then:
int fd = mkstemp(...);
if (fd == -1) ...
nCountFile = fdopen(fd, ...);
Comment 6 Pierre-Yves Rofes (RETIRED) gentoo-dev 2007-04-05 18:32:16 UTC
Created attachment 115523 [details, diff]
Patch to fix "FINISH_LAST_COUTNFILE" in /tmp

This one should work much better (note to self: never try to write code when you haven't slept enough).
Vapier: you're right about EXIT_SUCCESS, but if you look at the original code, the author also used it even in case of failure... Taviso told me not to touch it, so now it returns EXIT_FAILURE just in cases related to the patch, but not on other cases.I don't know if it's really better, but at least it fixes the issue.
Comment 7 Pierre-Yves Rofes (RETIRED) gentoo-dev 2007-05-03 17:39:52 UTC
any news here? someone tested the new patch?
Comment 8 SpanKY gentoo-dev 2007-05-05 06:53:50 UTC
still not there yet ;)

the first do{}while() loop can leak file descriptors ...

fd_lock = mkstemp(...);
...
nLockFile = fdopen(fd_lock, "r");
if (nLockFile == NULL) {
  ... retry process ...

really, the "if (fd_lock)" should be causing the retry and the "if (nLockFile == NULL)" should close(fd_lock)

also, in the case where fd_countfile fails, you leak fd_lock and nLockFile ... and in the case where nCountFile fails, you leak those two plus fd_countfile
Comment 9 Robert Buchholz (RETIRED) gentoo-dev 2007-09-09 12:15:04 UTC
From the 0.6.6 ChangeLog:
 o applied gentoo patch that fixes the insecure temporary file creation (Thanks to Pierre-Yves Rofes) 

There's a version bump bug with ebuilds, bug #117882.
Comment 10 Pierre-Yves Rofes (RETIRED) gentoo-dev 2007-09-09 13:12:59 UTC
I had forgotten this one :/  Currently the patch is still not okay, I'll have to correct the things mentioned in comment #8. I'll try to do it soon.
Comment 11 Robert Buchholz (RETIRED) gentoo-dev 2007-11-02 03:30:55 UTC
Pierre-Yves, any update here?
Comment 12 Christian Zoffoli (RETIRED) gentoo-dev 2008-01-15 12:09:21 UTC
already in cvs, thanks.
Comment 13 Robert Buchholz (RETIRED) gentoo-dev 2008-01-15 12:44:48 UTC
Please do not close security bugs.


Christian, are the issues mentioned in comment 8 taken care of?
Comment 14 Pierre-Yves Rofes (RETIRED) gentoo-dev 2008-01-15 13:00:18 UTC
(In reply to comment #13)
> Please do not close security bugs.
> 
> 
> Christian, are the issues mentioned in comment 8 taken care of?
> 

Nope, they're not.
Comment 15 Christian Zoffoli (RETIRED) gentoo-dev 2008-01-15 15:09:37 UTC
ops, I've missed #8 ...sorry
Comment 16 Matthias Geerdsen (RETIRED) gentoo-dev 2008-07-07 18:39:57 UTC
What is the status here? Any news?
0.6.7 is the current version now... is it still affected?
Comment 17 Matti Bickel (RETIRED) gentoo-dev 2008-12-07 14:52:54 UTC
so here's the thing: partimage included p-y's patch, but as vapier pointed out, this is not entirely correct. 

p-y: can you come up with a fixed patch?
Comment 18 Matti Bickel (RETIRED) gentoo-dev 2008-12-26 10:31:08 UTC
Created attachment 176426 [details, diff]
final? fix for the mkstemp problem

I was just scratching my head on this one once more.

Attached is a patch fixing the issue in comment #8.
This would be a HELL of a lot easier if i'd be allowed to drop the debug printout and do

if (mkstemp == -1 || fdopen == NULL)
  cleanup();

but i hope the patch is as clear as possible and i haven't missed a variable in the process.
Comment 19 Robert Buchholz (RETIRED) gentoo-dev 2008-12-26 21:09:01 UTC
Matti, are you posting this for review on this bug or did you propose it upstream?
Comment 20 Matti Bickel (RETIRED) gentoo-dev 2008-12-26 21:26:23 UTC
the former. i'll propose it to upstream, if either we agree that it's correct or nobody responds :)
Comment 21 Robert Buchholz (RETIRED) gentoo-dev 2008-12-27 10:52:33 UTC
Reviewed, looks sane to me. Please propose it upstream.
Comment 22 Samuli Suominen (RETIRED) gentoo-dev 2010-07-07 23:01:17 UTC
(In reply to comment #21)
> Reviewed, looks sane to me. Please propose it upstream.
> 

Upstream fixed this in 0.6.8, now in Portage.
Comment 23 Stefan Behte (RETIRED) gentoo-dev Security 2010-08-01 12:47:24 UTC
Arches, please test and mark stable:
=sys-block/partimage-0.6.8
Target keywords : "amd64 ppc x86"
Already stabled : "amd64 x86"
Missing keywords: "ppc"
Comment 24 Joe Jezak (RETIRED) gentoo-dev 2010-08-11 21:00:23 UTC
Marked ppc stable.
Comment 25 Stefan Behte (RETIRED) gentoo-dev Security 2010-10-06 13:19:43 UTC
For some reason (target-tool bug?), sparc was missing.

Sparc, please test and mark stable:
=sys-block/partimage-0.6.8
Comment 26 Raúl Porcel (RETIRED) gentoo-dev 2010-10-10 17:10:42 UTC
sparc is not stable
Comment 27 Tim Sammut (RETIRED) gentoo-dev 2010-11-20 23:35:54 UTC
GLSA Vote: Yes.
Comment 28 Stefan Behte (RETIRED) gentoo-dev Security 2010-11-21 16:25:42 UTC
Yes, too, GLSA request filed.
Comment 29 GLSAMaker/CVETool Bot gentoo-dev 2014-12-12 00:20:10 UTC
This issue was resolved and addressed in
 GLSA 201412-08 at http://security.gentoo.org/glsa/glsa-201412-08.xml
by GLSA coordinator Sean Amoss (ackle).
Comment 30 Roman Jay Almaza 2021-08-17 23:26:28 UTC Comment hidden (spam)
Comment 33 Jenny 2022-03-18 13:20:02 UTC
Greetings! Very useful advice in this particular article! It's the little changes that make the most significant changes. Thanks a lot for sharing!