Item2529: Issues with beforeSaveAttachment plugin dispatch (release 1.0.x scope)

When a stream is provided to *wiki::Func::saveAttachement (in the options hash), enabling beforeAttachmentSaveHandler causes Store.pm to copy the stream to a temporary file.

This code has problems in TWiki and in Foswiki 1.0. (Not sure where the corresponding code is in trunk, but it should be checked).

1) The copy loop uses sysread/syswrite, but does not handle errors properly.

2) There are two race conditions that can cause attachment corruption and/or provide an attack vector. This is because the code (a) closes the temporary file before issuing the callback - which must open the file by name because the stream is not passed to the callback. Therefore, the callback can get different data from what was written. (b) re-opens the temporary file by name when the callback returns - this means that addRevisionFromStream is also susceptable to tempfile attacks.

3) closing a File::Temp file is specified to delete the file; the close/re-open sequence works (at the moment) in some environments, but is not guaranteed.

There is no good solution without changing the plugin API - we really should be passing the stream, not the temp filename.

However, I would expect that plugins read attachments (e.g. for virus scan or to create a related file or index), but don't write them.

For this case, there's a straightforward fix (this is against TWiki's Store.pm, but the code is identical in foswiki.)

--- /var/www/servers/twiki/lib/TWiki/Store.pm~  2008-09-11 23:41:58.000000000 -0400
+++ /var/www/servers/twiki/lib/TWiki/Store.pm   2009-12-26 17:49:30.000000000 -0500
@@ -974,26 +974,37 @@
                 # the uploaded file when you close it even though the doco
                 # says it is deleted when it goes out of scope.
                 # The code below has proven to work for all. See Item5307

                 use File::Temp;
+               use Errno qw/EINTR/;
+
                 my $fh;
-
+
                 ( $fh, $tmpFile ) = File::Temp::tempfile();
                 binmode( $fh );
                 # transfer 512KB blocks
                 my $transfer;
                 my $r;
                 while( $r = sysread( $opts->{stream}, $transfer, 0x80000 )) {
-                    syswrite( $fh, $transfer, $r );
+                   if( !defined $r ) {
+                       next if( $! == EINTR );
+                       die "system read error: $!\n";
+                   }
+                   my $offset = 0;
+                   while( $r ) {
+                       my $w = syswrite( $fh, $transfer, $r, $offset );
+                       die "system write error: $!\n" unless( defined $w );
+                       $offset += $w;
+                       $r -= $w;
+                   }
                 }
-                close( $fh );
+               select((select($fh), $| = 1)[0]);
+               seek( $fh, 0, 0 ) or die "Can't seek temp: $!\n";
+               $opts->{stream} = $fh;
                 $attrs->{tmpFilename} = $tmpFile;
                 $plugins->beforeAttachmentSaveHandler( $attrs, $topic, $web );
-                open( $opts->{stream}, "<$tmpFile" );
-                binmode( $opts->{stream} );
-
             }
             my $error;
             try {
                 $handler->addRevisionFromStream( $opts->{stream},
                                                  $opts->{comment},

-- TimotheLitt

Elevating to urgent

-- KennethLavrsen - 27 Dec 2009

i've commited this patch to the Release01x00 branch; trunk is radically different, and i suspect the patch is unneeded there.

-- WillNorris - 29 Dec 2009

I took a quick look at trunk (Meta::attach) in svn. It no longer copies the file, but still does a close on the supplied stream. This is not guaranteed to work - particularly if the the stream is attached to a temp file. close may (should) make the tempfile disappear. Further, opening a tempfile by name is ALWAYS a security risk. We should use the stream -- this prevents race conditions around the name. Or pass /dev/fd* as a name. For re-processing a file (e.g. by a before handler, then store, seek will generally work. But it won't work if the stream is on a pipe -- e.g. in some versions of CGI, the stdin stream. And that's why the old code copied the stream to a new tempfile -- albeit with bugs. Tempfiles are guaranteed to be seekable, which is why the v1 patch can reuse the stream...

There is a reasonable discussion of the tempfile issues in perldoc File::Temp. Although the whole page is worth reading, pay particular attention to the section headed WARNING. (It's unfortunate that tempfiles weren't architected properly in the beginning - the legacy is a trap that people keep falling into...)

-- TimotheLitt - 29 Dec 2009

Timothe. Great great analysis and great work.

Can I lure you into making a patch also for our trunk?

I assume this is now fine for our Release branch and 1.0.9 which I am preparing.

-- KennethLavrsen - 02 Jan 2010

Happy New Year! This is pretty straightforward analysis when you know what to look for...

I'm still running TWiki - so I can't provide a tested patch for trunk. I simply read the code when Will said that trunk didn't need one - curious to see how the issues were addressed. I think creating a trunk patch belongs to the person who changed the design. But in any case it needs to be tested. I'd be happy to do design/code review.

To a first order, the code wants to look similar to the patched 1.0 stream. But I called that a temporary solution because it's not architecturally correct/complete. To summarize the open issues:

• For performance, I'd suggest only doing the copy if the input stream isn't seekable. Rather than use -f or !-p guesswork, I think I'd try a seek before dispatching & copy only if it failed.
• We should pass the stream to the plugin. The /dev/fd hack doesn't work everywhere, and for debugging, the tempfile name is more useful in error messages. But we need to document that plugins should NOT use the filename in an open or close. To support writes/replacement, we should allow the plugin to replace the stream with a new one.
• We need a release note to the effect that any plugin that uses the callout needs to be updated. For a low-medium security risk if it just reads the attachment. But if it writes/replaces it, for correctness.
• Distributed plugins need to be checked for use of this callout & updated. For phase-in, they probably should check for the stream (needs a new name as we currently pass a useless parameter of that name) and fallback to the filename otherwise. This would be easier/safer if we update the patch for V1.0 to pass the stream (in both directions). We can't force a synchronous update across the customer base -- particularly with plugins that we don't know about.
• Since we support TWiki plugins, we need to coordinate with that community. I've raised this issue with them (and recommended that they monitor this topic & coordinate with us). Their tracking item is rather content-free as the conversation has been in e-mail.
• The plugin attachment API has other issues (Tasks.Item572, Tasks.Item2365)) -- implementing a real fix for the security issue should also take the time to address these, since one API change (bad as it is) would be much better than two. Or three.
• We need regression tests for this.
• And then there's the issue that got me into this code in the first place - I passed a memory tempfile handle (open( \$foo)) as a stream to Func::saveAttachment_'s hash, and discovered that sysread immediately returns EOF. Sigh. Note that since these files _are seekable, the "performance" note will fix this by not doing the copy.

As you can see, this apparently simple bit of code actually has some messy issues.

-- TimotheLitt - 02 Jan 2010

I'm the one who wrote the original code and also changed the design. Right now I have an awful lot on my plate, and would be delighted if someone else could pick this up - I think Timothe's points are all good.

Some notes:

1. the reason why streams are passed rather than string data is the sheer size of some uploads, which requires them to be on disc instead of in memory.
2. IME seek does not work on all platforms (IIRC Windows claims it works, but the result is a null stream)
3. IME /dev/fd doesn't work everywhere

-- CrawfordCurrie - 06 Jan 2010

I am setting this in Waiting For release FOR Release01x00 branch ONLY!!

This is to get a closed bug report as reference.

I am raising an urgent bug Item2612 for trunk that just refers to this one.

-- KennethLavrsen - 09 Jan 2010