Bug #82720 for File-Temp: NFS failure

Tue Jan 15 09:58:37 2013 NHORNE [...] cpan.org - Ticket created

Subject:

NFS failure

This is a big restriction: "Finally, on NFS file systems the link count of the file handle does not always go to zero immediately after unlinking. Currently, this command is expected to fail on NFS disks." When will it be fixed? It causes many (most?) CPAN modules to fail on their make tst.

Tue Jan 15 10:04:44 2013 TJENNESS [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #82720] NFS failure
Date:	Tue, 15 Jan 2013 08:04:33 -0700
To:	Per Friberg <bug-File-Temp [...] rt.cpan.org>
From:	Tim Jenness <tjenness [...] cpan.org>

What fix do you suggest? The stat call is unreliable and no-one could come up with a bullet proof way of spotting that the unreliability was due to the disk being NFS mounted rather than someone doing something odd to a trusted file. There didn't seem to be a cross-platform way to reliably detect that a file was being written over NFS. On Tue, Jan 15, 2013 at 7:58 AM, Nigel Horne via RT < bug-File-Temp@rt.cpan.org> wrote: Show quoted text

> Tue Jan 15 09:58:37 2013: Request 82720 was acted upon. > Transaction: Ticket created by NHORNE > Queue: File-Temp > Subject: NFS failure > Broken in: 0.22 > Severity: Critical > Owner: Nobody > Requestors: NHORNE@cpan.org > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=82720 > > > > This is a big restriction: > > "Finally, on NFS file systems the link count of the file handle does not > always go to zero > immediately after unlinking. Currently, this command is expected to fail > on NFS disks." > > When will it be fixed? It causes many (most?) CPAN modules to fail on > their make tst. >

Tue Jan 15 10:04:45 2013 The RT System itself - Status changed from 'new' to 'open'

Sat Feb 02 13:01:59 2013 dagolden [...] cpan.org - Correspondence added

On Tue Jan 15 10:04:44 2013, TJENNESS wrote: Show quoted text

> What fix do you suggest?

The practical problem is that tempfile() in scalar context can croak on NFS because unlink0 failing is immediately fatal: https://metacpan.org/source/TJENNESS/File-Temp-0.22/Temp.pm#L1435 I suggest that if unlink0 fails in tempfile(), the file be scheduled for deferred unlinking instead, which only issues warnings rather than fatal errors. That has very minor security implications and only for a platform/filesystem combo on which the code would otherwise fail entirely. David

Sat Feb 02 13:02:19 2013 dagolden [...] cpan.org - Requestor DAGOLDEN added

Sat Feb 02 14:22:53 2013 ether [...] cpan.org - Cc ETHER added

Wed Feb 06 17:09:23 2013 dagolden [...] cpan.org - Correspondence added

Fixed in the new Perl-Toolchain-Gang repository: https://github.com/Perl-Toolchain-Gang/File-Temp/commit/3f836f8f55541572711ea0431485d78f000fe55c

Wed Feb 06 17:09:24 2013 dagolden [...] cpan.org - Status changed from 'open' to 'patched'

Wed Feb 06 17:50:01 2013 njh [...] bandsman.co.uk - Correspondence added

Subject:	Re: [rt.cpan.org #82720] NFS failure
Date:	Wed, 06 Feb 2013 17:49:37 -0500
To:	bug-File-Temp [...] rt.cpan.org
From:	Nigel Horne <njh [...] bandsman.co.uk>

On 02/06/2013 05:09 PM, David Golden via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=82720> > > Fixed in the new Perl-Toolchain-Gang repository: > > https://github.com/Perl-Toolchain-Gang/File-Temp/commit/3f836f8f55541572711ea0431485d78f000fe55c

Super, David - well done.

Download smime.p7s
application/pkcs7-signature 4.1k

Message body not shown because it is not plain text.

Wed Feb 06 17:50:03 2013 The RT System itself - Status changed from 'patched' to 'open'

Wed Feb 06 20:11:16 2013 dagolden [...] cpan.org - Severity Critical changed to Normal

Wed Feb 06 20:46:35 2013 dagolden [...] cpan.org - Reference by ticket #70224 added

Wed Feb 06 20:51:03 2013 dagolden [...] cpan.org - Status changed from 'open' to 'patched'

Wed Feb 13 21:33:00 2013 TJENNESS [...] cpan.org - Correspondence added

On Sat Feb 02 13:01:59 2013, DAGOLDEN wrote: Show quoted text

> On Tue Jan 15 10:04:44 2013, TJENNESS wrote:

> > What fix do you suggest?

> > The practical problem is that tempfile() in scalar context can croak on > NFS because unlink0 failing is immediately fatal: > > https://metacpan.org/source/TJENNESS/File-Temp-0.22/Temp.pm#L1435 > > I suggest that if unlink0 fails in tempfile(), the file be scheduled for > deferred unlinking instead, which only issues warnings rather than fatal > errors. > > That has very minor security implications and only for a > platform/filesystem combo on which the code would otherwise fail entirely. >

I do worry about the security implications. We are effectively saying that unlink0 might as well never try to be clever as we can always fallback to deferred unlink if it fails. Tom C was insistent that unlink0 behave like it does because he was extremely concerned about something weird happening to temp files (and didn't really care about "broken" operating systems that could not unlink an open file). Ideally you want to use deferred unlink only if you know you are on NFS. I assume that if you ramp up the security level the unlink0 can't be bypassed?

Wed Feb 13 21:33:01 2013 The RT System itself - Status changed from 'patched' to 'open'

Wed Feb 13 21:42:52 2013 dagolden [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #82720] NFS failure
Date:	Wed, 13 Feb 2013 21:42:07 -0500
To:	bug-File-Temp [...] rt.cpan.org
From:	David Golden <dagolden [...] cpan.org>

On Wed, Feb 13, 2013 at 9:33 PM, TJENNESS via RT <bug-File-Temp@rt.cpan.org> wrote: Show quoted text

> I do worry about the security implications. We are effectively saying that unlink0 might as well > never try to be clever as we can always fallback to deferred unlink if it fails. Tom C was insistent > that unlink0 behave like it does because he was extremely concerned about something weird > happening to temp files (and didn't really care about "broken" operating systems that could not > unlink an open file). Ideally you want to use deferred unlink only if you know you are on NFS.

Not caring about "broken" is not a very portable mindset, which I don't think is the right thing for a core module to be doing. My preferred model is: "We'll try really hard to be secure, but if you can't be really secure for whatever reason, we'll do something else reasonable instead." Show quoted text

> I assume that if you ramp up the security level the unlink0 can't be bypassed?

I don't have any objection to that. I suspect the number of people running under high is darn small and it won't bite most of the common "do tests in a tempfile" bugs that people hit. David -- David Golden <dagolden@cpan.org> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg

Wed Feb 13 21:54:41 2013 TJENNESS [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #82720] NFS failure
Date:	Wed, 13 Feb 2013 21:54:26 -0500
To:	Per Friberg <bug-File-Temp [...] rt.cpan.org>
From:	Tim Jenness <tjenness [...] cpan.org>

On Wed, Feb 13, 2013 at 9:42 PM, David Golden via RT < bug-File-Temp@rt.cpan.org> wrote: Show quoted text

> Queue: File-Temp > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=82720 > > > On Wed, Feb 13, 2013 at 9:33 PM, TJENNESS via RT > <bug-File-Temp@rt.cpan.org> wrote:

> > I do worry about the security implications. We are effectively saying

> that unlink0 might as well

> > never try to be clever as we can always fallback to deferred unlink if

> it fails. Tom C was insistent

> > that unlink0 behave like it does because he was extremely concerned

> about something weird

> > happening to temp files (and didn't really care about "broken" operating

> systems that could not

> > unlink an open file). Ideally you want to use deferred unlink only if

> you know you are on NFS. > > Not caring about "broken" is not a very portable mindset, which I > don't think is the right thing for a core module to be doing. > >

Sure. Not my word, but it is still the case that if something happened that causes unlink0 to fail unexpectedly in a manner that might be a security problem, then we simply hide the problem and defer the unlink. If we can defer the unlink any time that unlink0 fails then its not obvious why we are doing the unlink0 "bend over backwards to make sure" tests at all. Show quoted text

> My preferred model is: "We'll try really hard to be secure, but if you > can't be really secure for whatever reason, we'll do something else > reasonable instead." > >

but Tom would say that File::Temp is giving you an expectation of security and "try to be secure but ignore any problems" is not really being secure. You don't have an expectation for the same level of security if you are on a OS that can't unlink an open file so the deferred unlink is the best you can do. If you are on NFS you also can't have an expectation of security but the problem is we can't work out whether a directory is mounted remotely or not. Show quoted text

> > I assume that if you ramp up the security level the unlink0 can't be

> bypassed? > > I don't have any objection to that. I suspect the number of people > running under high is darn small and it won't bite most of the common > "do tests in a tempfile" bugs that people hit. > >

You can't expect the HIGH security level to work on NFS so we shouldn't be disabling the test because of NFS issues in this case. -- Tim Jenness

Wed Feb 13 22:05:58 2013 dagolden [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #82720] NFS failure
Date:	Wed, 13 Feb 2013 22:05:14 -0500
To:	bug-File-Temp [...] rt.cpan.org
From:	David Golden <dagolden [...] cpan.org>

On Wed, Feb 13, 2013 at 9:54 PM, TJENNESS via RT <bug-File-Temp@rt.cpan.org> wrote: Show quoted text

> Sure. Not my word, but it is still the case that if something happened that > causes unlink0 to fail unexpectedly in a manner that might be a security > problem, then we simply hide the problem and defer the unlink. If we can > defer the unlink any time that unlink0 fails then its not obvious why we > are doing the unlink0 "bend over backwards to make sure" tests at all.

The docs merely say "On systems that can not unlink an open file"... that doesn't say "operating systems" or "platforms" (the Perl euphemism for OS) so I think it's entirely reasonable to read "systems" as the generic combination of hardware, software, filesystem, etc.. Thus "if it can't be unlinked, it gets deferred". We could certainly amend the docs to indicate the weaker guarantee. Show quoted text

> but Tom would say that File::Temp is giving you an expectation of security > and "try to be secure but ignore any problems" is not really being secure.

There's safe and there's paranoid. It's only described as "preferred" to avoid race conditions using the filename. Since we don't give the filename, they can't do that anyway, the only difference is when the unlink happens. If you think attackers can open or replace arbitrary files, the battle is lost anyway. -- David Golden <dagolden@cpan.org> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg

Sat Apr 02 23:40:12 2016 dagolden [...] cpan.org - Correspondence added

Ticket migrated to github as http://example.com/issue/1234

Sat Apr 02 23:41:39 2016 dagolden [...] cpan.org - Correspondence added

Ticket migrated to github as https://github.com/Perl-Toolchain-Gang/File-Temp/issues/16