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.

Sign up for GitHub

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

Jump to bottom

Keep null origins from masquarding as "file://" #134

Merged
merged 1 commit into from
Feb 15, 2017
Merged

Keep null origins from masquarding as "file://" #134

merged 1 commit into from
Feb 15, 2017

Conversation

bfad
Copy link
Contributor

@bfad bfad commented Feb 9, 2017

Browsers send null origins when an iframe contains html code for its
source instead of a URL. This means that websites who used rack-cors and
allowed "file://" were open to attacks from malicious pages that used
this fact to send "null" origins.

@bfad
Keep null origins from masquarding as "file://"
33fabd7 
Browsers send null origins when an iframe contains html code for its
source instead of a URL. This means that websites who used rack-cors and
allowed "file://" were open to attacks from malicious pages that used
this fact to send "null" origins.
@cyu
Copy link
Owner

cyu commented Feb 11, 2017 •
edited
Loading

@bfad I'm trying to figure out what is the best way to approach this.

As of 2/11/2017, of the browsers I tested, only Safari returns file:// as the origin. Both Chrome and FireFox returns a null origin.

I know people rely on null support for the file protocol. I'm not sure why, the best I can think of is for testing purposes.

Isn't allowing file:// just as bad with or without translating null origins to file://? It would be much easier to just run a file from my file system than to do some iframe trickery to send null origins. The point is that this patch is only slightly more secure in that you would only be exploited thru Safari.

Let me know what you think – I would like to figure out an acceptable solution for this. I'm leaning towards just throwing a stern warning right now.

@bfad
Copy link
Contributor Author

bfad commented Feb 11, 2017

@cyu I need the file:// protocol for mobile app purposes. The problem with treating null as file:// is that browsers also send null as the origin when there's an iframe that has the code source in the src attribute. Ex:

<html>
  <body>
    <iframe src="data:text/html,<script> var xhr = new XMLHttpRequest(); xhr.onreadystatechange = function() { if (xhr.readyState == 4) { alert(xhr.responseText); } }; xhr.open('GET', 'https://example.com/path', true); xhr.withCredentials = 'true'; xhr.send(null);</script>"></iframe>
  </body>
</html>

I can work around the problem by using a Proc for the origins attribute, but it seems to me that the gem's default should be for security — people who need to allow null origins could either add it to the array or make their own Proc as needed.

@bfad
Copy link
Contributor Author

bfad commented Feb 14, 2017

To reply to the edits you made in your original comment:

It would be much easier to just run a file from my file system than to do some iframe trickery to send null origins.

If I'm an attacker, I think it's much easier to get a person to visit a website with an iframe in it then it is to get a person to visit a webpage, download a malicious html file, and then get them open it up in a browser.

The other problem with treating a null origin as file:// is the complete unexpectedness of it. If I wanted to allow null origins, I would have put that in my array. How was I to know I was also allowing null origins when I allowed for file:// origins? (There's not even anything in the documentation.)

My suggestion is to accept my pull request, and then document the browser differences with respect to origins while warning people against allowing null origins due to other potential exploits. This way rack-cors isn't doing something unexpected and opening up a security whole for people like me, and people looking for documentation on why file:// isn't working for them can see they need to add null origins to get it to work but it also opens them up to malicious iframe attacks.

@cyu
Copy link
Owner

cyu commented Feb 14, 2017

@bfad I see your point – I was thinking about it from the perspective of an individual attack. And I agree about the unexpectedness problem – that to me is a more compelling reason than the former.

@cyu cyu merged commit 80450a0 into cyu:master Feb 15, 2017
@cyu
Copy link
Owner

cyu commented Feb 15, 2017

Thanks!

@ineffyble
Copy link

ineffyble commented Aug 26, 2018 •
edited
Loading

I think there's an inadvertent problem that's been created by this.

Right now, even if I put my allowed origins as "*", a request with an Origin header of "null" (e.g. from a Firefox extension content script) fails with a 422 Unprocessable Entity.

I believe that's because of this in Rails: https://github.com/rails/rails/pull/30780/files

Before, rack-cors was altering the Origin to be "file://", which stopped that 422 from occurring. Now, rack-cors doesn't change it from "null", so it gets blocked by Rails.

Ran into this problem a few months ago when our project upgraded rack-cors.

@itzsaga itzsaga mentioned this pull request May 12, 2020
Merged
@dependabot dependabot bot mentioned this pull request Mar 13, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bfad @cyu @ineffyble