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

ActiveStorage's signed keys not unique, e.g. can be reused for other variants #31662

Closed
koenpunt opened this Issue Jan 9, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@koenpunt
Contributor

koenpunt commented Jan 9, 2018

The comments in BlobsController and VariantsController mention security-through-obscurity, altho I would argue that this is no security at all, since the signed keys returned from ActiveStorage::Variation.encode are not unique, and thus the variant key of one stored file, can be reused for another file.

For example; a website provides you thumbnails, where upon purchase you get access to the download page. The signed key then being used, can be reused for any other image, by replacing the signed key part in the thumbnail URL.

I might be misunderstanding the "security-through-obscurity" case here, and it could be that this is the designed behavior, but I would say it would be better to generate these signed keys with an additional variable parameter (filename maybe), so that the signed key for 1000x1000 is not for every file the same.

Rails version: 5.2.0.beta2

Ruby version: 2.4.1p111

@georgeclaghorn

This comment has been minimized.

Show comment
Hide comment
@georgeclaghorn

georgeclaghorn Jan 9, 2018

Member

Security-through-obscurity refers more to the signed blob ID than the signed variation key, even though the comment on VariantsController suggests otherwise. (It should be edited not to do that.)

As I see it, variations are generally harmless. The point of the key is primarily to have a convenient way to reference one in a URL.

Member

georgeclaghorn commented Jan 9, 2018

Security-through-obscurity refers more to the signed blob ID than the signed variation key, even though the comment on VariantsController suggests otherwise. (It should be edited not to do that.)

As I see it, variations are generally harmless. The point of the key is primarily to have a convenient way to reference one in a URL.

@koenpunt

This comment has been minimized.

Show comment
Hide comment
@koenpunt

koenpunt Jan 9, 2018

Contributor

As I see it, variations are generally harmless.

Maybe, but in some occasions they're not; like the example I gave above. And I can give more examples like it, but it all boils down to that sometimes not all variants are equal.

The point of the key is primarily to have a convenient way to reference one in a URL.

I figured it was for preventing users to request any variant they want, but it is just for convenience, then why does it has to be encoded at all?

Then also, what is the static part doing in the variant key?

eyJfcmFpbHMiOnsibWVzc2FnZSI6IkJBaHBFZz09IiwiZXhwIjpudWxsLCJwdXIiOiJibG9iX2lkIn19--d8898284494e573d3806c5d773a4f831b8cd862a
^ static hash                                                                     ^ encoded 1000x1000

Anyway; if the file (digest) would be a variable in generating the signature, the issue would not exist. I can try and open a PR with this proposed change?

Contributor

koenpunt commented Jan 9, 2018

As I see it, variations are generally harmless.

Maybe, but in some occasions they're not; like the example I gave above. And I can give more examples like it, but it all boils down to that sometimes not all variants are equal.

The point of the key is primarily to have a convenient way to reference one in a URL.

I figured it was for preventing users to request any variant they want, but it is just for convenience, then why does it has to be encoded at all?

Then also, what is the static part doing in the variant key?

eyJfcmFpbHMiOnsibWVzc2FnZSI6IkJBaHBFZz09IiwiZXhwIjpudWxsLCJwdXIiOiJibG9iX2lkIn19--d8898284494e573d3806c5d773a4f831b8cd862a
^ static hash                                                                     ^ encoded 1000x1000

Anyway; if the file (digest) would be a variable in generating the signature, the issue would not exist. I can try and open a PR with this proposed change?

@rails-bot rails-bot bot added the stale label Apr 10, 2018

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot bot Apr 10, 2018

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-1-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

rails-bot bot commented Apr 10, 2018

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-1-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot closed this Apr 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment