File uploads go to insecure location

[sussexrick]

First logged as http://issues.umbraco.org/issue/CON-1454.

What did you do?

Create a form in Umbraco Forms 6.0.5 on Umbraco 7.7.6 with a file upload type. Fill in the form by uploading a file and clicking submit.

What did you expect to happen?

The file could contain private and confidential information or personal data, so I would expect it to be accessible only to people with permissions to view data submitted to that form.

Most likely I would expect it to upload to another IFileSystem configured in FileSystemProviders.config with a different alias.

What actually happened?

It was uploaded to the standard media folders via the IFileSystem. This location is typically available to anyone who can view the entire website, which on a public website means anyone with an Internet connection. It has a URL that's difficult to guess, but that's not sufficient protection for personal or private data.

Workaround

I've published a NuGet package, Escc.Umbraco.Forms.Security, which includes an updated FileSystemProvider which routes forms uploads to a separate folder that can be secured properly.

https://github.com/east-sussex-county-council/Escc.Umbraco.Forms
https://www.nuget.org/packages?q=Escc.Umbraco.Forms

[ronaldbarendse]

It looks like Umbraco Forms has hard-coded all access to uploaded files to use FileSystemProviderManager.Current.GetFileSystemProvider<MediaFileSystem>();, this should indeed be made configurable to be able to split it from media and thereby also allow securing its storage.

Not only the FileUpload field type uses this file system provider: exports, the 'Send email with template (Razor)' workflow (add as attachments), XSLT files (used when posting as XML, save as file and send XSLT mail) and the RecordService (cleanup after deleting records) all use it.

The SendRazorEmail also only allows attaching the uploaded files from the built-in field type, without any way of subclassing/inheriting/override this (besides creating your own workflow from scratch or overriding the FileUpload with a custom one using the same FieldTypeId).

[umbrabot]

Hiya @sussexrick,

Just wanted to let you know that we noticed that this issue got a bit stale and might not be relevant any more.

We will close this issue for now but we're happy to open it up again if you think it's still relevant (for example: it's a feature request that's not yet implemented, or it's a bug that's not yet been fixed).

To open it this issue up again, you can write @umbrabot still relevant in a new comment as the first line. It would be super helpful for us if on the next line you could let us know why you think it's still relevant.

For example:

@umbrabot still relevant
This bug can still be reproduced in version 8.9.0

This will reopen the issue in the next few hours.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[sussexrick]

@umbrabot still relevant

This is still the case in Forms 8.6.0. The workaround I produced is no longer maintained and may not work.

[Matthew2016]

@nul800sebastiaan Bit disappointing to see something like this still not resolved, I would of thought security would be Umbraco's number 1 priority.

For a paid product such as forms, it seems to be always left behind....

[TimIOSH]

Still relevant.

Would like to see a more secure solution or at least a more easily configurable secure solution.

[StenersonB]

Does anyone have a solution for this? Even something like locking down 'media/forms/' to users who aren't logged in would be better than nothing.

[alanwthom]

+1 for secure form uploads by default. Given the nature of some of the files that users could be uploading, this should really be a core feature (especially for a licensed product IMO).

@StenersonB We've attempted to workaround this with a cutom HttpModule, specifically looking to intercept the incoming request, check if it's trying to access a form upload, if so check if the user is logged into the backoffice and return a forbidden error if not. This might be an approach to consider in the meantime until it's supported in the core product?

public class FormUploadModule : IHttpModule
{
   public void Init(HttpApplication context)
   {
      context.AuthorizeRequest += new EventHandler(AuthorizeRequest);
   }
 
   public void AuthorizeRequest(object sender, EventArgs e)
   {
      var file = ((HttpApplication)sender).Context.Request.FilePath;

       if (file.StartsWith("/media/forms/upload/"))
      {
         var user = new HttpContextWrapper(HttpContext.Current).GetUmbracoAuthTicket();
 
         if (user == null)
         {
            ((HttpApplication)sender).Context.Response.Status = "403 Forbidden";
            ((HttpApplication)sender).Context.Response.StatusCode = 403;
         }
      }
   }

   public void Dispose() { }
}

You can then add this to the <modules/> section of your web.config file, e.g. <add name="FormUploadModule" type="Namespace.FormUploadModule"/>.

[AndyButland]

We've added an module along the lines of that suggested by @alanwthom above - thanks very much for this contribution Alan. We've extended it a little to handle further cases - i.e. a back-office user that is logged in but doesn't have access to the "Forms" section, or doesn't have access to manage forms, or lastly, does have access to manage forms but not the one for which the uploaded file was submitted - for each of these a 403 response will be provided as well.

To be released in 8.10.0 and 9.2.0.

[bjarnef]

@AndyButland this works to me and I only have an UMB_UCONTEXT cookie set, but our customer can't access the file even logged into Umbraco backoffice.

She has the following cookies set, but get 403 Forbidden. She tried clearing the cookies and log back in, but same result.

It seems the chunks-2 value is set for a large authentication cookie:
umbraco/Umbraco-CMS#13402

[bjarnef]

@AndyButland @nikolajlauridsen does the changes in https://github.com/umbraco/Umbraco-CMS/pull/13404/files also need to apply to how this is restricted in Umbraco Forms?

[AndyButland]

Yes, it looks like it. I can see we are doing something similar to the original code in the middleware used to protect the media requests here. Will have a look for the next patch releases.

Thanks for raising and digging out the link to the fix.

[bjarnef]

Thanks @AndyButland 🙌😎

I am not sure in which scenario the UMB_UCONTEXT cookies is splitted into multiple cookies an value chunks-2 where 2 indicate two additional cookies.. I guess it potential also could be chunks-3 ... and why the customer get this, but I or Umbraco Cloud support don't get this splitted up?

[AndyButland]

Looking at the original issue it comes about when the amount of "claims" in the cookie exceeds a certain level, which in Umbraco terms is most likely number of user groups. So could be an administrator is fine, but an editor in several user groups would trigger the problem.

[bjarnef]

Yes, it makes sense as I and Umbraco support team member was administrator user, but customer only with editors user group assigned.