Protect uploads by default

[chillu]

At the moment, this relies on the author setting up the form to select an uploads folder that has been explicitly configured with access restrictions. We warn them on the file field creation dialog, but that really just creates more uncertainty.

By default, uploaded files are stored as public files in /Uploads. While this still relies on the file being linked to unauthorised parties, or them guessing the exact URL, that's a bad default. In 4.x, we have per-file access restrictions, so don't need to rely on permission control of the folder containing those files.

In addition, folder protections might be inadvertently removed by other authors/admins because they're not aware that this folder contains sensitive user submissions (e.g. due to bad naming). While that could also happen to individual submission attachments, it's far less likely.

Conclusion: I think we should protect the individual file uploads by default, which always overrules their folder permissions. I don't think we need to make this configurable, it should be the new default. If you're building a system that allows unmoderated public file submissions, I don't think userforms is the right choice. Can anyone think of a use case that might explicitly rely on this?

Since this is an additional security measure, I don't think we need to create a task to retroactively apply this to existing submissions. That would be useful, but it's not essential.

Related: Clearly advise how to protect uploaded files

ACs: (todo)

• Set permissions directly on uploaded files based on parent folder permissions (or grandparent+ folder if parent folder is set to inherit)
• ...

Subtasks (todo)

• ...

[chillu]

Note that once this is implemented, we should either create an automated end-to-end test, or add a manual Hiptest scenario for it. We already have tests for asset protection itself, the important part here is that uploads are indeed getting this protection through userforms.

[chillu]

The frustrating part about this is that we had this in place for the 3.x compatible userforms, and then removed it based on wrong assumptions about what functionality is tied to the old securefiles module: https://github.com/silverstripe/silverstripe-userforms/blob/4.6/code/extensions/SecureEditableFileField.php. See 6836174.

I believe we should fix this in various ways (layered security):

• The 3.x compatible logic force secured folders through makeSecure() on saving the field (not the submission). I would prefer if we just throw a validation error if the folder isn't secure. Otherwise you risk e.g. cutting off access to a general purpose folder like assets/Uploads.
• The EditableFileField should throw an exception if the folder it's told to upload to doesn't exist, that's undefined behaviour we shouldn't make any assumptions on
• The uploaded file should explictly copy the permissions from the parent folder, rather than inheriting them. This means if an admin removes the protection from this folder, the file still isn't exposed. The downside of this approach is that any changes to the folder permissions (e.g. locking down from CMS Authors to Administrators) won't be reflected in the files, and there's no way to batch update permissions. But I think we should default to security over usability on this one. Alternatively, we could mark the folder as containing files with userform submissions, and prevent existing permissions from being removed through validation.
• The uploaded file should be clearly marked as associated to a userform submission in AssetAdmin to avoid mishandling of this file (@silverstripe/ux-contributors thoughts? Could be on the "used on" report)
• Track IPs of any uploads to aid later analysis (e.g. due to abuse). Note that IPs are considered PII, and this might trigger implementors of userforms to update their privacy policies. But it's pointless if we make it opt-in. Just needs to be called out in a changelog.

[clarkepaul]

Good idea to indicate that a file has come from a form submission. If each upload field has only one relation to a folder I think the folder should also have an indication it's only used by a form, although folders could contain both files from a form and files which are not from the form currently right?

I agree it makes sense to protect the files as a smart default in case the admin doesn't have security awareness, but couldn't the folder still be changed by an admin to un-protect it with its permission being copied down to files? Potential use case... uploading a file to a shared gallery or shared submissions where you want others to be able to view the files/submissions. There was a case a while ago where a council wanted photos uploaded by the public which were instantly public (so the person uploading could see their addition to a collage) and these were reviewed every few hours by admins for inappropriate content.

My thoughts:

• File should indicate it is either protect and/or related to a form
• Folder should indicate the relation to a form
• If we display info on the "Used on" tab to indicate where the file has come, it should navigate to the form submission and not the form.

[chillu]

If each upload field has only one relation to a folder I think the folder should also have an indication it's only used by a form

There is no way of ensuring that without non-trivial work around upload validation - we don't have the concept of "exclusive ownership" of a folder for one use case (like userform uploads).

couldn't the folder still be changed by an admin to un-protect it with its permission being copied down to files

That's why I said this in the ticket description: "I think we should protect the individual file uploads by default, which always overrules their folder permissions:

File should indicate it is either protect and/or related to a form

If we do what I recommend above, that indication is given through the "Who can view this file?" selection on the "Permissions" tab. I'm a bit hesitant adding a "lock" icon or something to the UI on a file-by-file basis, because those additional permission checks with further slow down the response. We already check "canView" for the current user, but not for anonymous users.

So there's a couple of things we could do:

1. List userform submission in "Used on" for a file
2. Visual indicator for protected file on file selection and browsing
3. Visual indicator for protected folder on folder selection (e.g. visible on dropdown in userforms upload field)
4. List userform itself in "Used on" for a folder (we don't have that tab there at the moment)
5. Custom messaging on folder edit screen with link to userform
6. Custom messaging on file edit screen with link to userform submission

I think the first item is MVP, everything else is a bonus where we have to evaluate tradeoffs. @clarkepaul @brynwhyman Thoughts?

[clarkepaul]

Looking good.

For point 1. it might be better if the file linked to the actual form submission and not the page/form it was uploaded to, otherwise it might be hard to figure out which submission it comes from (1000's submissions).

[brynwhyman]

Alright, thanks for the early investigation here! I'm seeing the following ACs surfacing:

• Files uploaded through a userform default to a protected folder if no folder is set in the userform admin (this will be similar to the SecureUploads folder in SS3)
• The EditableFileField should throw an exception if the default protected folder doesn't exist
• Files uploaded through a userform are always protected on a per-file basis
• The uploaded file should be clearly marked as associated to a userform submission in AssetAdmin to avoid mishandling of this file
• The CMS user understands through the userforms admin that the uploaded files will be 'protected'

Plus some more implied by the DoD:

• The change in upload security is noted in the module documentation (dev and user)
• The change in upload security is noted in the expected change log
• CucumberStudio tests are recorded to cover new scenarios

Perhaps we could also include something like the following to help with the management of files:

• When selecting a new upload destination, the parent folder must always be the new default protected folder (i.e SecureUploads in SS3)
• It is still possible to choose a new upload destination folder, but the uploaded files will not inherit the permissions of that folder

Feedback welcome @chillu @clarkepaul

[chillu]

Files uploaded through a userform are always protected on a per-file basis

"Files uploaded through a userform are always protected on a per-file basis if they are placed in a protected folder"

When selecting a new upload destination, the parent folder must always be the new default protected folder (i.e SecureUploads in SS3)

No, I think that's too restrictive, we just have to make sure that it's very clear if authors pick an unprotected folder there.

It is still possible to choose a new upload destination folder, but the uploaded files will not inherit the permissions of that folder

Agreed.

New AC:

In order to allow authors to set up Userform uploads with an unprotected folde, a developer needs to specifically enable this through configuration, alongside big fat warning flags.

The alternative is that we just remove this ability, but there are cases where userforms are used for anonymous uploads. For example, a council asking for tombstone pictures with a custom moderation queue. This example would continue to work, but it'd require an explicit opt-in. Alternatively, we could just provide event hooks, and it's up to devs to customise the implementation with a File->unprotect() call there.

[brynwhyman]

Re your comment:

"Files uploaded through a userform are always protected on a per-file basis if they are placed in a protected folder"

So, you're saying this case should only apply if a file is uploaded to a protected folder? Shouldn't we be looking to protect all files by default regardless of the whether the folder is protected? This would go well alongside:

In order to allow authors to set up Userform uploads with an unprotected folde, a developer needs to specifically enable this through configuration, alongside big fat warning flags.

side note: we'll be discussing this issue in a bit more detail with the @silverstripeux team in the next couple of days. I expect we'll probably split out a few of these ACs into different cards too - so we can go a little deeper in to suggested solutions for each.

[chillu]

Shouldn't we be looking to protect all files by default regardless of the whether the folder is protected?

No, that would make management of files harder, particularly without good batch abilities in AssetAdmin (only a handful of files at a time). By default, files should inherit from their folder. But userform uploads are particularly "toxic", and given our current CMS UI they might be mistaken.

For example, a news author wants to embed the annual report into a news article, and searches for "report". The first result is assets/reviewed/report.pdf. But in fact this is a medical report uploaded through userforms. Turns out a different group of authors in charge of userform submissions has created a file review pipeline that moves files from assets/SecureUploads to assets/reviewed, and forgot to secure this new folder. Now the news author mistakes this file, and attaches it to the news article. Unless the file is specifically protected (rather than inheriting permissions), this just caused information disclosure. Admittedly, this relies on a chain of human error, but this is a measure of layered security. We should make it as hard as possible to do the wrong thing with userform uploads.

[bergice]

Moving back to New Issues for now.

[chillu]

@bergice @brynwhyman I think this points to a protected channel, can you please outline the rationale here?

[brynwhyman]

We've moved this issue out of our internal ZenHub backlog while there's focus on defining ACs and digging deeper on related issues:

• Indication if upload destination is secure with FileUpload field #948 <- Indication if upload destination is secure with FileUpload field
• Icon showing if file/folder is restricted + tied to a userform submission silverstripe-asset-admin#1066 <- show icon on file/folder is protected or public
• Provide a recommended folder for the FileUpload field #949 <- Change the destination for the FileUpload field when none is chosen (related to this issue)
• Provide a clear indicator that the file/folder belongs to a userform submission #950 <- show a form submission icon
• Add the option to delete a related file(s) when a submission is deleted #952 <- ability to delete a form submission file when you delete the form submission

[brynwhyman]

We've recently spent a considerable amount of time on a 'Harden form submission security' epic. Through opportunity and solution investigation this opportunity was not chosen to be developed in favour of the issues noted in my previous comment.

With the release of CMS 4.6 and v5.3 of the userforms module the following functionality will be available:

• All new forms with a file upload field will default to storing uploaded files in a new, restricted access folder titled 'Form-submissions'
• There is still the option to chose another destination folder, but the permission settings for that folder are made clear before the user confirms the action (highlighting if they're choosing an insecure destination folder)
• The 'secure by default' folder dedicated to form submissions will also help mitigate the expected issue of: "folder protections might be inadvertently removed by other authors/admins because they're not aware that this folder contains sensitive user submissions (e.g. due to bad naming)."
• Uploaded files will still default to a permission level of 'Inherit from parent folder' but this was a conscious decision to keep as this allows for bulk updating of file permissions by updating the parent folder settings
• There's a new icon indicator for files to depict whether it was received via a userform submission upload
• There's a new icon indicator for files and folders to highlight if the file or folder has restricted permissions applied. This should also mitigate the risk of a user mistakenly changing the permission structure and exposing files to the public.

This will be summarised nicely in the upcoming CMS 4.6 release announcement, but ultimately we chose a different path to what's been outlined in this issue description. @chillu are you happy to close this issue in favour of the features soon to be shipped?

[brynwhyman]

Given the above comment and support for closing from @clarkepaul in this comment, I'm closing this.