Bug 20445
| Summary: | restore(8) uses non-varying filenames for /tmp files when restoring given filesystem repeatedly | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | jkuroda <jkuroda> | ||||
| Component: | bin | Assignee: | Sheldon Hearn <sheldonh> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | ||||||
| Priority: | Normal | ||||||
| Version: | 3.3-STABLE | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
State Changed From-To: open->feedback Does this problem exist in OpenBSD as well? I know that we stole some security fixes from them a while back, and wonder if perhaps they haven't sorted this out since then. On Mon, 07 Aug 2000 09:37:22 MST, Jon Masami Kuroda wrote: > This "bug" should not have any security impact if I understand open(2) > and the "O_EXCL" flag correctly. It is, so far, just a productivity > annoyance. While that's certainly the way it looks to me, I'm very uncomfortable with proceding without finding out _why_ OpenBSD's rev 1.3 was required. Essentially, your patch proposes that we revert NetBSD's rev 1.3. The CVS log for that delta implies that the patch you propose will somehow break the -r and -R options. I'm not sure whether <lukem@supp.cpr.itg.telecom.com.au> is still a deliverable address. Perhaps you could chat to Theo and ask him why the revision was introduced in the first place? Ciao, Sheldon. In looking at the OpeBSD 1.1->1.2 diff and the 1.2->1.3 diffs for dirs.c,
it looks like that mkstemp(3) was applied in 1.1->1.2 in extractdirs(), but
that setdirmodes() still recreated the value of modefile on its own instead
of using the value of modefile created by mkstemp(3) back in extractdirs().
OpenBSD, src/sbin/dirs.c, r1.2
setdirmodes(flags)
int flags;
{
FILE *mf;
struct modeinfo node;
struct entry *ep;
char *cp;
vprintf(stdout, "Set directory mode, owner, and times.\n");
(void) sprintf(modefile, "%s/rstmode%d", _PATH_TMP, dumpdate);
mf = fopen(modefile, "r");
if (mf == NULL) {
fprintf(stderr, "fopen: %s\n", strerror(errno));
fprintf(stderr, "cannot open mode file %s\n", modefile);
fprintf(stderr, "directory mode, owner, and times not set\n");
return;
}
...
Since extractdirs() in 1.2 used mkstemp(3) to create unique filenames,
setdirmodes() would fail on opening the file since it expected a filename
of the form "/tmp/rstmodeDECIMAL", instead of "/tmp/rstmodeDECIMAL-MKSTEMP"
which extractdirs created via mkstemp(3).
the 1.2->1.3 patch fixed this by having restore use "/tmp/rstmodeDECIMAL"
for the "R" and "r" cases in both extractdirs() and setdirmodes() so that
they were using the same filename. It is not clear to me why setdirmodes()
was not modified to just rely on the mkstemp(3) value of modefile used
back in extractdirs() instead of the special casing of "R" and "r" or why
the value of dirfile is not created via mkstemp(3) in all cases for that
matter. I guess a better question should be "why did setdirmodes() ever
try to recreate the value of modefile on its own?" None of the other
functions that rely on dirfile, for instance, ever try to recreate that
value from scratch, and instead rely on extractdirs() to do its thing
to the value dirfile.
In any case, it seems like a good idea to talk to the author of the original
patch for this since we all seem to have a bunch of questions about this now
Cheers,
Jon
Sheldon Hearn <sheldonh@uunet.co.za> writes:
: On Mon, 07 Aug 2000 09:37:22 MST, Jon Masami Kuroda wrote:
:
: > This "bug" should not have any security impact if I understand open(2)
: > and the "O_EXCL" flag correctly. It is, so far, just a productivity
: > annoyance.
:
: While that's certainly the way it looks to me, I'm very uncomfortable
: with proceding without finding out _why_ OpenBSD's rev 1.3 was required.
: Essentially, your patch proposes that we revert NetBSD's rev 1.3. The
: CVS log for that delta implies that the patch you propose will somehow
: break the -r and -R options.
:
: I'm not sure whether <lukem@supp.cpr.itg.telecom.com.au> is still a
: deliverable address. Perhaps you could chat to Theo and ask him why the
: revision was introduced in the first place?
Aha, I found out why and I didnt even have to talk to Luke yet! =)
From the NetBSD restore(8) manpage:
The temporary files /tmp/rstdir* and /tmp/rstmode* are generated with a
unique name based on the date of the dump and the process ID (see
mktemp(3)), except for when -r or -R is used. Because -R allows you to
restart a -r operation that may have been interrupted, the temporary
files should be the same across different processes. In all other cases,
the files are unique because it is possible to have two different dumps
started at the same time, and separate operations shouldn't conflict with
each other.
I found this while going through the NetBSD cvsweb stuff where I found
revision 1.20 of dirs.c and traced my way to the man page.
http://cvsweb.netbsd.org/cgi-bin/cvsweb.cgi/basesrc/sbin/restore/dirs.c
Now this makes sense. It's still annoying, but it makes sense. Perhaps
restore(8) needs a flag to say "screw this, use mkstemp!" if the operator
so desires. Or a way to save the names of the files for later use.
--Jon
Sheldon Hearn wrote on Tue, 08 Aug 2000 12:16:39 +0200:
:
: On Mon, 07 Aug 2000 09:37:22 MST, Jon Masami Kuroda wrote:
:
: > This "bug" should not have any security impact if I understand open(2)
: > and the "O_EXCL" flag correctly. It is, so far, just a productivity
: > annoyance.
:
: While that's certainly the way it looks to me, I'm very uncomfortable
: with proceding without finding out _why_ OpenBSD's rev 1.3 was required.
: Essentially, your patch proposes that we revert NetBSD's rev 1.3. The
: CVS log for that delta implies that the patch you propose will somehow
: break the -r and -R options.
:
: I'm not sure whether <lukem@supp.cpr.itg.telecom.com.au> is still a
: deliverable address. Perhaps you could chat to Theo and ask him why the
: revision was introduced in the first place?
Someone in the office asked why I didnt just use "restore xf dumpfile"
and it seems the question and answer is relevant enough to put in here
as explanation for others:
from FreeBSD restore(8)
If a backup was made using more than one tape volume, restore will notify
the user when it is time to mount the next volume. If the -x or -i flag
has been specified, restore will also ask which volume the user wishes to
mount. The fastest way to extract a few files is to start with the last
volume, and work towards the first volume.
For the purpose for which I used restore, this is annoying since I dont
really want to have to type "1" or anything interactively to restore
when I have a dozen or more fs's being cloned. Now granted I could use
expect as an interactive wrapper, but I should be able to do this all
with restore on it's own. Or maybe I'm just ultra-lazy.
--Jon
On Tue, 08 Aug 2000 11:01:10 MST, Jon Masami Kuroda wrote:
> Now this makes sense. It's still annoying, but it makes sense. Perhaps
> restore(8) needs a flag to say "screw this, use mkstemp!" if the operator
> so desires. Or a way to save the names of the files for later use.
I can't think of an elegant solution. I think we're approaching the
limits of the software, beyond which we'd stub our toes trying to make
it all things to all men.
I'd like to close the PR, but if you have any great ideas that you think
we won't stub our toes on, I'll leave it open.
What say you?
Ciao,
Sheldon.
I think the only thing that could be done is to add that bit from the NetBSD
restore(8) manpage to the FreeBSD one so future young bright file system
cloners wont repeat my steps.
BUGS
restore can get confused when doing incremental restores from dumps that
were made on active file systems.
A level zero dump must be done after a full restore. Because restore
runs in user code, it has no control over inode allocation; thus a full
dump must be done to get a new set of directories reflecting the new in-
ode numbering, even though the content of the files is unchanged.
> The temporary files /tmp/rstdir* and /tmp/rstmode* are generated with a
> unique name based on the date of the dump and the process ID (see
> mktemp(3)), except for when -r or -R is used. Because -R allows you to
> restart a -r operation that may have been interrupted, the temporary
> files should be the same across different processes. In all other cases,
> the files are unique because it is possible to have two different dumps
> started at the same time, and separate operations shouldn't conflict with
> each other.
I would add one more note to that manpage change:
This will prevent cloning of filesystems from a single dumpfile to
multiple filesystems at one time via "restore -r" or "restore -R".
Given time, I could see implementing a -c[lone] flag or the NetBSD TMPDIR
environ(7) behaviour, but I think that is well beyond the scope of this PR.
ENVIRONMENT
If the following environment variable exists it will be utilized by
restore:
TMPDIR The directory given in TMPDIR will be used instead of /tmp to
store temporary files. Refer to environ(7) for more information.
Thanks for your time on this one, lets close it.
Cheers,
Jon
State Changed From-To: feedback->closed AProposed change committed to HEAD as rev 1.22 and merged onto the RELENG_4 and RELENG_3 branches. Responsible Changed From-To: freebsd-bugs->sheldonh He did it. |
restore(8) uses the dumpdate of the filesystem being restored to create the filenames for the files in /tmp used to store the names of directories -- /tmp/rst* -- and of their ownership/mode/time stamps -- /tmp/rstmode*. The first file, dirfile is used to store the list of dirs to be created before files are restored into them, the second, modefile, to restore the ownership,mode, and time stamps on those dirs after file restores are finished. The dumpdate is the number of seconds since the Unix Epoch (tm) till the time the filesystem in question was dumped. example: dirs.c: 1 /* * Extract directory contents, building up a directory structure * on disk for extraction by name. * If genmode is requested, save mode, owner, and times for all 5 * directories on the tape. */ void extractdirs(genmode) int genmode; 10 { register int i; register struct dinode *ip; struct inotab *itp; struct direct nulldir; 15 int fd; vprintf(stdout, "Extract directories from tape\n"); (void) sprintf(dirfile, "%srstdir%d", _PATH_TMP, dumpdate); 20 if (command != 'r' && command != 'R') { (void *) strcat(dirfile, "-XXXXXX"); fd = mkstemp(dirfile); } else fd = open(dirfile, O_RDWR|O_CREAT|O_EXCL, 0666); 25 if (fd == -1 || (df = fdopen(fd, "w")) == NULL) { if (fd != -1) close(fd); warn("%s - cannot create directory temporary\nfopen", dirfile); done(1); 30 } if (genmode != 0) { (void) sprintf(modefile, "%srstmode%d", _PATH_TMP, dumpdate); if (command != 'r' && command != 'R') { (void *) strcat(modefile, "-XXXXXX"); 35 fd = mkstemp(modefile); } else fd = open(modefile, O_RDWR|O_CREAT|O_EXCL, 0666); if (fd == -1 || (mf = fdopen(fd, "w")) == NULL) { if (fd != -1) 40 close(fd); warn("%s - cannot create modefile\nfopen", modefile); done(1); } 44 } The dumpdate made of the filenames prior to their creation in lines 19 and 32. Note that after that, a call to mkstemp(3) is made but *only* if neither the "R" or "r" flags are provided to restore(8). Else, the file is created via open(2) with mode 0666, and umask 077 obtained from main.c:93. This introduces one very annoying problem. The name of restore(8)'s temporary files when restore(8) is given the "R" or "r" flags are very predictable -- in fact, they never change. While the use of the O_CREAT and O_EXCL flags to open(2) should prevent a non-privileged user from exploiting an otherwise possible race condition (right?), the problem I found is that this prevents one from using ``restore rf'' or ``restore Rf'' to clone a filesystem to several blank filesystems in parallel from a single dumpfile created earlier by dump(8) as each restore will attempt to create the same filenames in /tmp and only the first one will succeed. (given the example shown above) The rest will fail out like so: restore: /tmp/rstdir965378984 - cannot create directory temporary fopen: File exists restore: /tmp/rstmode965378984 - cannot create modefile fopen: File exists and so prevent one from restoring from one dumpfile to multiple filesystems (such as in FS cloning) simultaneously. It seems that mkstemp(3) was added in revision 1.6.2.1, but mkstemp(3) was used only in certain places, presumably only those places that would introduce nasty race conditions. Fix: Use mkstemp(3) instead of open(2) by itself in extractdirs(), when creating "dirfile" and "modefile". Do not recreate the value of "modefile" from "dumpdate" in setdirmodes(), and instead rely on mkstemp to modify the value of "modefile", back in extractdirs(), which is in an array of "static char"'s. This can easily be accomplished by removing the special casing for the "R" and "r" flags and letting these cases reap the benefits of mkstemp(3) as in other case such as when the "x" flag is used. I scoured the code and the CVS logs and couldn't find a reason for why mkstemp shouldn't be used here. I think it makes sense to on its own merit and also because it makes the code slightly simpler for not having to special case "R" and "r". But, I could be mistaken in this and my patch below could be terribly naive, so take it all with a grain of salt and the 10 coke's I had prior to writing this up. Thanks to Mike Smith for helping me remember that I had been meaning to submit a PR for this for sometime now. A first pass at patch on ${SRC}/sbin/restore/dirs.c This patch tested on 3.4-STABLE and 4.1-RELEASE. How-To-Repeat: * given a number of newfs'd fs's mounted in /mnt/altroot * and a dump of an example root fs to be cloned in /scratch/... # for newroot in 1 2 3 4 5; do cd /mnt/altroot/${newroot}/ && restore rf /scratch/root.dump & done watch the 2nd restore and onward complain about not being to open temporary files correctly.