Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SECURITY] Authentication Bypass by Forceful Browsing #987

Open
ybelenko opened this issue May 28, 2019 · 31 comments

Comments

@ybelenko
Copy link
Contributor

commented May 28, 2019

Feature Request

About audited Directus version.
It has been cloned from suite repo.
Latest commit directus/directus@1d151a9

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

screenshot

What problem does this feature solve?

Fixes security hole.

How do you think this should be implemented?

  • As a general rule, system components should not be accessible to unauthenticated users (excluding the authentication component, which is unique, as unauthenticated access to it is a functional requirement).
  • The authentication enforcement mechanism should be thoroughly and comprehensively implemented in the system. The authentication validation should be performed as the first validation in each access to the system. The accessing entity should be obligated to authenticate with the system as a pre-condition to use the system’s services. After a successful authentication process, the user server-side session storage should be "populated" with the permissions of authorized actions.
  • It is recommended to use authentication enforcement mechanisms that operate globally (effectively affecting all components), instead of implementing authentication validation within each separate component. Authentication validation implementation through applicative filters or infrastructure authentication declarations is highly recommended, since it is less likely to be prone to authentication flaws resulting from human error. Furthermore, these mechanisms are easier to manage and are usually well tested and widely used.

Would you be willing to work on this?

Maybe, with help/guidance from Directus team.

@Nitwel

This comment has been minimized.

Copy link
Member

commented May 28, 2019

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.

@Nitwel

This comment has been minimized.

Copy link
Member

commented May 28, 2019

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.

@ybelenko

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

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 benhaynes added this to Needs triage in Bug Triage via automation May 31, 2019

@bjgajjar bjgajjar moved this from Needs triage to Medium priority in Bug Triage Jun 6, 2019

@itsmerhp

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

@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.

@benhaynes

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

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

@hemratna

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

@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,

  1. The image name UUID supported (give an option in the to use the UUID in the file name)
  2. Add access permission for each file.

@rijkvanzanten Let me know your thoughts. So we can finalize the approach.

@benhaynes

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

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?

@hemratna

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

@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.

@itsmerhp

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

@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?

@benhaynes

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

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

@bjgajjar

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

It will be partially fixed with the development of #337

@rijkvanzanten

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

@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.

@bjgajjar

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

@rijkvanzanten But I think this will not resolve our private permission issue. Am I right?

@rijkvanzanten

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Ah good point. I got confused by the other issue that references the "guessability" of the names

@itsmerhp

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

@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?

@benhaynes

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

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 all s3 file be public. That seems like a good amount of granularity... especially when we allow for choosing the adapter per upload (later).

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:
    • https://example.com/uploads/[project]/files/[id]/data
    • For private files, we'll need to use a new 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.
  • Public — If the storage adapter is set to public then assets are just presented via the normal UUID link path. This would remain the same as now:
    • https://example.com/uploads/[project]/originals/image.png

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).

Question 2

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).

Problem 1

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.

Problem 2

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.

@hemratna

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

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 all s3 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,

  1. The file will be copied from S3 to our server temp directory and from temp directory, it will be served to the client(browser). In this case, it will consume (nearly) twice the bandwidth.
  2. Streaming the video may create some issue.
  3. Not sure above the effect on CDN and other caching services.

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.

@hemratna

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

How to use AWS S3 ACL?

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.

@benhaynes

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

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?

@rijkvanzanten

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

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.

@benhaynes

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

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??

@hemratna

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

@itsmerhp Any thoughts on the timeline?

@rijkvanzanten

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

@itsmerhp

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

Hello @benhaynes @hemratna
As discussed in the above comments,

  • We are going to start the implementation of privacy settings for local storage adaptor for now.
  • There will be two settings options for a storage adaptor
    1. Private: Only user who has uploaded file can only access the file, including logo image.
    2. Public: Files will be accessible publicly, not even login needed for it.

Please let me know if I am missing anything for initial implementation.

@rijkvanzanten

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

There will be two settings options for a storage adaptor

  1. Private: Only user who has uploaded file can only access the file, including logo image.
  2. 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

@benhaynes

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

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! :)

@rijkvanzanten

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

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)

@benhaynes

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Ahh, true! Forgot about those external apps. 😉

@itsmerhp

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2019

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

@benhaynes

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

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?

@hemratna

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

Currently, the AWS S3 ACL for the private file is also not working. Please consider adding the support for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.