Open ybelenko opened 5 years ago
Hmm, there is the option that you can enable/disable public access to the files collection but I guess it was forgotten to add these rules to the thumbnailer too.
Also, isnt this the same issue as in #986 ? The app side loads these images using the thumbnailer which would be as described in the other issue.
Also, isnt this the same issue as in #986 ?
Yeah, kinda. I added this issue just because security department suggested different solutions("How do you think this should be implemented" section) from referenced one.
@benhaynes This issue is identical to #986, to resolve this we have to restrict files directly being accessed and implement a script in which files are restricted for logged in user only, and we have to place the script API side where the files are being accessed.
Makes sense. Will this effect caching, CDNs, latency, etc? Are there any drawbacks to this approach? If so, we should look into making it optional — especially since we'll be switching to UUID naming, which isn't "guessable" (part of the problem).
Any estimate on how long this will take to implement @itsmerhp?
ping @rijkvanzanten
@benhaynes Yes, It will effect on caching, CDNs, and latency.
According to approch provided by @itsmerhp We should give an option for each image for the access permissions.
For example,
the logo doesn't need any access permission. So one should set the public
permission for logo file.
If you set the private
permission then it will require token to token access the file.
We also need to increase (or make customizable) the ttl
time for access_token
as currently, we have 5 min which is very small for CDN and other caching services.
We may think for a special token which only uses for accessing the image.
According to me, we should implement the UUID first,
UUID
supported (give an option in the to use the UUID in the file name) @rijkvanzanten Let me know your thoughts. So we can finalize the approach.
Thanks @hemratna — that seems like a great approach. I agree that we should start with UUID since that partially solves the problem. After that we can write the permission gateway to secure files, which I think should be off by default (so files are public).
With the individual file permissions, files would still need to go through the permission gateway to check if each are public
/private
, right? Would we want a global option so we can completely ignore that check if the admin sets all files to public?
@benhaynes We can start with UUID
for the file name, which is also configurable by giving the options in the admin panel.
Regarding the file permission, We will give a global option for access permission.
@benhaynes
As @hemratna suggested, we will have a toggle button in APP
to ON / OFF
UUID
for the file name.
Question: What should be the default file naming option UUID
or Original name?
Instead of a toggle, let's use a Dropdown of options for file name. That will be more future-proof if we want to add more options in the future.
It looks like UUID
will be the file name convention... by a hair!!
https://twitter.com/directus/status/1139618994879696897
It will be partially fixed with the development of #337
@bjgajjar I think it fixes this fully actually 🙂 Having UUIDs for filenames fixes this problem. Changing the filenaming from UUID to a less-safe option is a user decision.
@rijkvanzanten But I think this will not resolve our private permission issue. Am I right?
Ah good point. I got confused by the other issue that references the "guessability" of the names
@benhaynes @hemratna If we set global permission regarding file permission, then a user can either permit all the files to the public or none.
I have a few questions in it: Question 1: Are we going to facilitate global permission user specific or it will be applicable for the entire system? For example: If a user has set global files permission to private then only that user is going to access, not even admins? Or if global permission set to private then all the users of that system can access all the files but not public? Question 2: What if admin wants to give files permissions to the specific role's all users, but not public or other users?
Regarding file permission, we can give role-based permissions also, but as per me, there seem two problems in it. Problem 1: All files are storing in the directus_files table and I think for core collections we don't have "mine" option and due to that we can't restrict that roles can see and modify their files only.
Problem 2: For example File interface has been used in a collection as a field and for that collection, a role has not permission but for File Library(directus_files) that role has permission, then in that case how to restrict the listing of files of that collection in file directory?
We could make this a global setting, but I think it make more sense to have this be a configuration option for the storage adapter. That way you could have all local
files be private, but have all s3
file be public. That seems like a good amount of granularity... especially when we allow for choosing the adapter per upload (later).
https://example.com/uploads/[project]/files/[id]/data
uuid
for the directus_files.filename` so it's different from the item's primary key. That way we can still refer to the File in the App/API by the primary key, but you can't guess its URL from that. https://example.com/uploads/[project]/originals/image.png
Set for the each storage adapter, regardless of the role/user. Either way permissions would decide if you can see the asset through the App... but if set to public then you could still view the asset after logging out (since you know the actual asset URL).
You can still do this for public and private files... but public files can still be accessed when the user is not authenticated (if they know the exact asset URL).
We should use the existing directus_files.uploaded_by
field for this mine
permission. This seems like a different ticket... so let's wait on this. The focus here is allowing files to be private at the storage adapter level.
To keep things simple, I think we keep everything the same... but if you do not have access to Directus Files, then you're "Choose Existing Files" modal would be empty, and any relationships to files you don't have permission to would be NULL
(or however this works for other collections/data now).
Does all this make sense?
TL;DR: We should add the ability to make files private at the storage adapter level, and those files get routed through a gateway that checks auth/permission. Any other features should be in a new ticket.
Global vs Storage Adapter
We could make this a global setting, but I think it make more sense to have this be a configuration option for the storage adapter. That way you could have all
local
files be private, but have alls3
file be public. That seems like a good amount of granularity... especially when we allow for choosing the adapter per upload (later).
Just to make things more clear, We also need to give support for the multiple adapters of the same storage type.
For example, One can have two S3 bucket
one used for public files and one for private files, or S3 bucket
for private files and local
for public files.
How this works...
- Private — If a storage adapter is set to private, then all asset requests are routed through a gateway that checks auth/permission and the true asset URL/UUID is never seen. Perhaps something that extends the API files endpoint:
Instead of writing gateway
, can we use AWS S3 ACL
Benefits.
Writing our own gateway
has some downsides,
Implementing the ONLY AWS S3 ACL will not support private file mode for local
adapter.
Question 1
Set for the each storage adapter, regardless of the role/user. Either way permissions would decide if you can see the asset through the App... but if set to public then you could still view the asset after logging out (since you know the actual asset URL).
Agreed, In the future, we might require an option for role wise permission, For example, admin
can see all the media, but user
can only see their's media.
If the media is uploaded to storge which is set to private
, and the current user has permission to access the file,
append_storage_information
function https://github.com/directus/api/blob/master/src/helpers/file.php#L99 will generate S3 URL with append extra query params like X-Amz-Security-Token
, X-Amz-Expires
and other required information.
In case the user doesn't have permission this will return a blank string.
We also use the same concept for all the other file adapter which support inbuilt ACL like DigitalOcean Spaces, Google Cloud Storage, etc.
Thanks @hemratna! It seems like there are some great benefits to using the S3 ACL. Maybe we should have a core (local
) gateway as a fallback, but progressive enhancements for other storage adapters (eg: S3)?
We could start with whichever is easier (S3 ACL or local gateway) and to the other later. The important thing is having support for private files somewhere.
@rijkvanzanten — thoughts on this approach?
We've discussed serving files through the API instead of relying on the webserver before (in that case to figure out the CORS config). Doing that would also allow us to make files actually private by relying on the Directus API ACL.
Do you have a sense of time needed to integrate each of these options? @hemratna
Perhaps we should start with the global/local/fallback/default with an API endpoint that returns actual files (based on auth/acl)? Once we have that we could look into progressive enhancements for S3, etc.
Thoughts??
@itsmerhp Any thoughts on the timeline?
@directus/api-team
Hello @benhaynes @hemratna As discussed in the above comments,
Please let me know if I am missing anything for initial implementation.
There will be two settings options for a storage adaptor
- Private: Only user who has uploaded file can only access the file, including logo image.
- Public: Files will be accessible publicly, not even login needed for it.
Can we also support role
for private
? So it's more in line with regular permissions none
/ mine
/ role
/ full
@itsmerhp
I like that @rijkvanzanten — but then would we also want some sort of none
? Or does that not make sense for files?
Also, remember that this is a setting for the whole Storage Adapter! :)
That makes sense to me. A user role can have permission to create files, but not read them. This is the same with the ability to create new items in the CMS, but not read them (useful for user submissions)
Ahh, true! Forgot about those external apps. 😉
Hello @benhaynes I agree with @rijkvanzanten, we should set role-wise permissions for files and existing permission logic should be used to allow/restrict file access.
If we use role-wise permission(App > Roles & Permissions) for files accessibility, then public/private can't be applied storage adaptor wise(for eg. can't set one local storage public and another private), permissions will be applied for all the files of all the local storage adaptors.
Approx Timeline: 40 hours
Perfect — thank you @itsmerhp!
This all sounds good, and the timeline works... but let's wait on starting this until the App is "stable". We have a much higher ticket count on the App and we should get most of those resolved before starting in on a bigger overhaul like this one.
In the meantime, if an external dev wants to give it a try we can help guide them and review the PR! But internally we need to triage the whole platform first.
Sound good?
Currently, the AWS S3 ACL for the private file is also not working. Please consider adding the support for this.
Feature Request
About audited Directus version. It has been cloned from suite repo. Latest commit https://github.com/directus/directus/commit/1d151a9034514e3f2ec1c80001e7c5fffdef2d4e
Description:
Unauthenticated attackers can bypass the application's authentication enforcement and directly access internal components, effectively circumventing the authentication credentials validation phase.
Business risk:
An attacker can get an access to the possible sensitive files.
Technical details:
Forceful Browsing attacks (direct access to internal components) can be used to exploit incomplete authentication enforcement in the server-side application. The forceful browsing technique enables attackers to access internal sensitive components, without undergoing the application authentication credentials validation phase.
Through the following link it is possible to get an access to application images that should be accessible after the authentication.
https://xxxxxxxx/uploads/_/originals/photo2_fullsize.jpg
What problem does this feature solve?
Fixes security hole.
How do you think this should be implemented?
Would you be willing to work on this?
Maybe, with help/guidance from Directus team.