Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Attached files security restriction prevents some use cases.
Use case 1
A custom form to upload files using the managed_file render element. The files are not meant to be stored permanently, and are uploaded by untrusted anonymous users. Then the files are attached and sent to some type of Contact person. Drupal automatically garbage-collects the temporary files eventually.
The problem is that only public files paths are allowed to be attached, unless I grant "send arbitrary files", which apparently is a very big security problem.
How original issue was resolved - see comment #7.
For historical reasons, some suggestions were offered to extend the module:
- Have some way to tell mimemail that the file is allowed to be attached, directly when adding the attachments.
- Mimemail to invoke a hook to ask other modules if the file is allowed. But the modules may lack context to decide.
Use case 2
Attach images located in the site's theme directory - see comment #8. Proposed solution is in comment #5.
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | mimemail-additional_paths-UI-2624516-20.png | 48.95 KB | tobybellwood |
| #20 | mimemail-additional_paths-UI-2624516-20.patch | 2.42 KB | tobybellwood |
| #11 | mimemail-additional_paths-2624516-11.patch | 2.31 KB | m.lebedev |
| #5 | mimemail-additional_paths-2624516-5-7.x-1.0-beta4.patch | 879 bytes | grom358 |
Comments
Comment #2
MiroslavBanov CreditAttribution: MiroslavBanov commentedComment #3
MiroslavBanov CreditAttribution: MiroslavBanov commentedComment #4
grom358 CreditAttribution: grom358 commentedHere is one approach, using drupal variable to specify additional allowed paths.
Comment #5
grom358 CreditAttribution: grom358 commentedWhoops, had the wrong if condition.
Comment #6
maximpodorov CreditAttribution: maximpodorov commentedThank you for the patch!
Admin UI for the new variable is desirable.
Comment #7
MiroslavBanov CreditAttribution: MiroslavBanov commentedWell, #5 doesn't work for me, because temporary files path is very environment-specific.
I am also starting to wonder about the purpose of the security checks. The attack vector seems dubious.
I have changed my implementation to read the file contents, and directly add that as attachment. This solves my problem.
Comment #8
maximpodorov CreditAttribution: maximpodorov commentedYour solution is ideal for images located in the site's theme directory.
Comment #9
MiroslavBanov CreditAttribution: MiroslavBanov at FFW commentedUpdating issue summary to be about actual feature request.
Comment #10
MiroslavBanov CreditAttribution: MiroslavBanov at FFW commentedComment #11
m.lebedev CreditAttribution: m.lebedev commentedAdded admin UI for the new variable.
Comment #12
ndf CreditAttribution: ndf at Dx Experts commentedAdded a related issue with an alternative approach (with an alter hook).
Comment #13
MiroslavBanov CreditAttribution: MiroslavBanov at FFW commentedI continue to not see exactly what is the security concern. Why allow only public files? This is an API module. Other modules are attaching the files. They should do the security checks.
#5 is OK.
It does mitigate the issue. And contrary to what I said in #7, it works for allowing additional stream wrappers - for example adding "temporary://" to allowed paths.
About #12.
The problem with exposing a hook is that likely other modules won't have enough context to decide if it is safe to allow the file or not. The example usage checks for REQUEST_TIME. But what if you use queue_mail? It won't work. The place where you are likely to know if it is safe to send the file is the place where you call drupal_mail() and pass the attachments parameter.
Comment #14
ndf CreditAttribution: ndf at Dx Experts commented#13
You're right about "The problem with exposing a hook is that likely other modules won't have enough context to decide if it is safe to allow the file or not.".
I dropped the patch here and in the issue-queue more like a reference.
Regarding the security-issue.
The permission and public-file checks has been introduced and improved in 2 security commits:
- https://www.drupal.org/node/1719482
- https://www.drupal.org/node/2211419
Before those commit, you could attach everything (like settings.php).
Comment #15
tostinni CreditAttribution: tostinni at Agence Propal commentedIt should be mentioned that this security issue in #14 also prevent files to be attached if they're inside the private file system.
See #2183955: Private File System and image not being embedded - Could someone advice me..
Comment #16
Siggy CreditAttribution: Siggy commentedThanks for the work done with this issue.
Patch from comment #11 works for us to allow mailing webform uploads from a private folder without having to allow every non-public file to be mailed as an attachment.
Comment #17
MiroslavBanov CreditAttribution: MiroslavBanov at FFW commented#11 also converts the file paths to lowercase. I think this is incorrect, and will cause issues on case-sensitive file systems.
Comment #18
Ronino CreditAttribution: Ronino as a volunteer commentedWhile I don't really see a scenario where an attacker can attach any file (who has access via shell or something can access any file and circumvent any security measures anyways), I think that the solution with additional paths in #11 just shifts the problem to a different realm.
On one hand, granting a role "send arbitrary files" restricts potential attackers to users of this role.
On the other hand, configuring allowed paths opens these paths up for any user and potential attacker no matter what role he/she has. This is okay for theme files, but not for private files.
The questions is: What is worse or less bad?
I like the file module's approach with hook_file_download_access() with which access can be determined on file basis. Maybe something like hook_mimemail_attachment_access() with _mimemail_file() passing it the whole attachment as set in hook_mail() instead of just the URL. This way a module implementing hook_mail() can set the attachment including data required for having enough context for access determination if necessary.
Comment #19
tobybellwood CreditAttribution: tobybellwood at govCMS (Australian Government Department of Finance) for govCMS (Australian Government Department of Finance) commentedWith Webform now defaulting to Private File system storage (if available), this patch brings valuable utility when used in conjunction with webform_clear, or if you have a non-drupal document workflow.
I'll post a patch with updated UI wording to more clearly (hopefully) explain the private file option alongside the theme option. We'll look at putting the patch through it's paces.
For the record, Private file systems can be broadly specified as Private:// or directly ../acquia-files/files-private - this works outside the docroot too.
Happy for any alternate suggestions on wording or configuration.
If we end up rolling out a Webform+webform_clear+mimemail+PrivateFiles workflow, we'll also endeavour to post a how-to for noobs.
Comment #20
tobybellwood CreditAttribution: tobybellwood at govCMS (Australian Government Department of Finance) for govCMS (Australian Government Department of Finance) commented(this time including the title label change and a screencap).
Comment #21
nerdcore CreditAttribution: nerdcore at OpenConcept Consulting Inc. commented#20 Works as described.
Thanks so much for this!
Comment #22
nerdcore CreditAttribution: nerdcore at OpenConcept Consulting Inc. commentedComment #24
TR CreditAttribution: TR commentedThis patch really needs test cases.
But I'll leave the issue at RTBC because the Mime Mail module doesn't have working tests for anything yet, so it would be a bit unfair to hold this patch back.
Adding test coverage is going to take some time, and it should be done incrementally rather than going back and trying to figure out what should be tested. That's why I think that this patch, which adds a new feature, should have tests written for it NOW so that we can see the feature works and prevent it from being broken by other code changes. And frankly having tests helps show the maintainer that this feature does the right thing and should be committed.