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.I strongly suspect that webform_file_download() should either approve or deny access to file URIs matching private://webform/..., and never return NULL.
At present, when a user (Alice) attaches a file to a webform submission but has yet to submit it, that file is accessible to any other user (Bob).
In that situation the file is not (yet) part of a submission, and it doesn't appear in Bob's session as a submission-in-progress, so webform_file_download() ignores it completely (effectively saying "I don't manage access to this file"). In my case, file_file_download() then provides headers and therefore Bob's download is approved.
This would commonly be a fairly temporary state of affairs, which mitigates the situation somewhat; but it seems like a Bad Thing to me in general, and if a user started a submission and then abandoned it without using the "remove" button (or suffered a crash, etc), I imagine that file remains accessible permanently?
As webform file components enforce the directory prefix of 'webform/', it seems reasonable to me to assume that any file under that directory is definitely controlled by the webform module, and that access can and should be denied in this situation.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | webform-private_file_access-2035329-7.patch | 1.33 KB | jweowu |
| #3 | webform-private_file_access-2035329-3.patch | 1.51 KB | jweowu |
| #2 | webform-private_file_access-2035329-2.patch | 1.5 KB | jweowu |
| #1 | webform-private_file_access-2035329-1.patch | 741 bytes | jweowu |
Comments
Comment #1
jweowu CreditAttribution: jweowu commentedComment #2
jweowu CreditAttribution: jweowu commentedChanges to permit access to these files for submission admins, and also to deny access to files which are part of a submission if the submission access check failed.
Comment #3
jweowu CreditAttribution: jweowu commentedAnother tweak to stop presuming
private://, as other streams can also require private access checks.Comment #4
quicksketchTemporary files are deleted by system.module after 6 hours, but it does look like file.module grants access to files if they're temporary, even if it's not the original source of the file upload.
I think the additional check you've added here is a good thing, but the permission checked is probably incorrect. It should be "access all webform results" rather than the edit/delete permissions. Even that I'm not sure if it's necessary, since if a user didn't "submit" the file by saving the form, really no one should have access to it at all. It'd be a fairly artificial barrier for many administrators since they also have access to the file system, but for the greatest privacy I think we should explicitly deny access to all temporary files to everyone but the user who uploaded the file. Note this doesn't apply to "draft" submissions, since even draft submissions mark the file permanent.
The downside of this patch (regardless of the permission check), is that it will make #1408082: Change the upload path for file components to be outside of webform/ slightly more difficult, but I don't think this will add a significant challenge. And that issue isn't going anywhere anyway.
Comment #5
jweowu CreditAttribution: jweowu commentedAgreed. I hadn't noticed the auto-deletion, but that makes it easy.
I presume that creates a submission record, and therefore the submission access checks in this function will kick in?
Comment #6
quicksketchYep that's right. It's a real submission but it just has the "is_draft" flag set, kind of like an unpublished node. Administrators can view and edit draft submissions, they just have a sort of "auto resume" feature for end-users who start the submission.
Comment #7
jweowu CreditAttribution: jweowu commentedAs discussed, I've removed the
user_access()checks from the new code in favour of simply denying access to everyone in that instance.Comment #8
quicksketchThanks @jweowu! Looks good to me. I'll commit this after I get a chance to confirm the behavior.
Comment #9
quicksketchTested this out and everything works as described. Thanks @jweowu! Committed to 7.x-4.x branch only.
Comment #10.0
(not verified) CreditAttribution: commentedtweaked wording
Comment #11
DeveloperChris CreditAttribution: DeveloperChris as a volunteer commentedThis is a serious security issue and should be ported to the 7.x.3.x branch
A user can upload malicious content and link to it from external sites resulting in at the very minimum reputation damage to your site.
All uploads should be private until after the upload has been reviewed and accepted by the appropriate person.
After having this issue reported on one of the sites I manage (I must say it caused quite a stink) I find that although the problem is well known and has been resolved it was not marked as a security fix and is not in the "current" 7.3 branch which is the only branch pulled in by Drush when doing security patching.
Comment #16
DanChadwick CreditAttribution: DanChadwick commentedComment #17
mlhess CreditAttribution: mlhess as a volunteer commented