Item2128: Item2115 (upload CSRF) needs merging to trunk

pencil
Priority: Urgent
Current State: Closed
Released In: 1.1.0
Target Release: minor
Applies To: Engine
Component:
Branches:
Reported By: CrawfordCurrie
Waiting For:
Last Change By: CrawfordCurrie
Also the unit tests are currently banjaxed. The workaround we have in 1.0.7 is OK, but not a long term solution IMHO.

The problem is this:
  1. User visits Attach screen, enters name of file and hits Upload
  2. Request is sent with the token from the Attach screen
  3. Server processes request, expires the token, and uploads the file
  4. User hits "back" in browser to get back to the Attach screen.
  5. User enters a new filename and hits Upload
  6. Request is sent with the original (now expired) token from the Attach screen
  7. Server sees the key is invalid, and redirects to the confirmation screen by 302
    • The redirect caches the request, but does not cache the uploaded file
    • when the upload request finishes, the file is removed during DESTROY
  8. User gets the OK/Cancel confirmation dialog and hits OK
  9. The file has gone from the server (in step 7) so the upload fails
In step 7 we need to be able to cache the uploaded file, but there are a couple of problems with doing that:
  1. Uploaded files can be very big, and caching the already cached upload could push the server to its limits
  2. Expiring redirect caches becomes more complex than simply deleting the files; now we would have to locate and remove any cached uploads.
Obviously you can get around this by clearing {Validation}{ExpireKeyOnUse}, but that's not a great idea as it weakens the protection considerably.

-- CrawfordCurrie - 21 Sep 2009

Thinking about it more. If you cache the file on the server, then you are opening a potential hole that could be exploited by filling up the disk with cache files to DoS or otherwise kill the server. So it might be better to reject the request on a validation failure. If you do that, then no upload can happen without a minty fresh validation code.

Merged and closed.

-- CrawfordCurrie - 16 Nov 2009

Re-opened to fix a bug in the fix. Testing a hashref using if( $hashref ) will always return true, one has to explicitely bind it to a hash to test its content.

Also cleaned up the useless(?) global $uploads, and the default loading of Data::Dumper (to save performance and memory).

-- OlivierRaginel - 18 Nov 2009

ItemTemplate edit

Summary Item2115 (upload CSRF) needs merging to trunk
ReportedBy CrawfordCurrie
Codebase trunk
SVN Range SVN 5065: Foswiki-1.1.0-dev, Thu, 03 Sep 2009, build 4764
AppliesTo Engine
Component
Priority Urgent
CurrentState Closed
WaitingFor
Checkins distro:0c8cb548e8a4 distro:46928b8f9d03
TargetRelease minor
ReleasedIn 1.1.0
Topic revision: r7 - 06 Sep 2010, CrawfordCurrie - This page was cached on 29 May 2017 - 02:14.

The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License