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: Make JSONP responses optional. #6164
Conversation
Crap, didn't mean to include the first commit. If you want me to remove it and re-submit my pull request, please let me know. |
I like this, and we were discussing internally to potentially disable this by default for 1.3, but at the very least people should be able to disable it. Few comments:
|
Agreed, thanks for the comments and insight, I'll work on that getting As far as keeping it on by default, that's totally fair, I didn't think What is JSONP usually used for? Kibana? Fitblip On Wed, May 28, 2014 at 6:16 AM, Shay Banon notifications@github.comwrote:
|
Hey @kimchy, I've implemented your requested changes. Does everything look good? Also if you guys would like to discuss JSONP, CORS, or really anything security related, please feel free to reach out to me. I use elasticsearch enough that I'm happy to give back in whatever form I can. Fitblip |
@Fitblip thanks!, will definitely keep you in the loop and @ you on the issue we open. @spinscale looks good to me, what do you think? |
Awesome! Glad to help in any way I can 👍 |
|
||
################################## Security ################################ | ||
|
||
# If you want to enable JSONP as a valid return transport on the http server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick 1: as you changed the default behaviour, can you mention the disabling here explicitely and set the option to false
below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good call, I'll fix that!
This looks great. left very minor comments. I'd like to include it, and just saw that you haven't signed the CLA. As soon as you do that (under http://www.elasticsearch.org/contributor-agreement/), I am happy to get this PR in! |
Hey @spinscale, I think I've implemented the changes you requested (I had some confusion on the first one), and signed the CLA. Let me know if you need anything else from me 😃 |
Hey there, I have been thinking about this a bit, and have another question about this impl. If you disable sending of JSONP, do you really want to execute the search request and return the results without a callback or do you want to return an error message telling the user that JSONP is disabled (and also not wasting precious process time executing a query). What do you think makes more sense? I am leaning towards the option of returning an error, but maybe this is not considered best practice? |
Hey @spinscale, That's a good point actually. Returning a JSONP-formatted error response probably makes the most sense. I can re-factor this to make that happen, but it may be a couple days. Thanks! |
This should throw an error message when JSONP is disabled, but I'm not sure where the actually query is getting executed, so I 't know if by not actually passing back the buffer it's never read from/executed or what. By the time NettyHttpChannel.sendResponse() is invoked is the query already run? Or is this where it's executed? |
@Fitblip yeah the request has already been executed by then, if you have a RestResponse object I guess we need to take a look at I realize that you already put a considerable amount of time into this one, so I can just check by myself if you want, but I am as happy to help and assist, if you want to move forward (thanks a lot for all your work so far!) Also, we need a test here, when we are clear about the actual impl, to make sure, the setting works as expected :-) |
Awesome! Thanks for the info. For the most recent push do you have any preferences as to the parameters passed back through JSONP? Right now it's just I can certainly take a stab at checking the parameter and bailing out. We can hammer out impl details once I have something to propose :). I'll also be happy to write the test/s. |
Hey @spinscale, It looks like RestController.dispatchRequest is the place to do it (to me). I've essentially reverted all my changes to I'll make sure things are working as expected tomorrow (not sure if I'm calling settings the right way), and fix any issues that crop up. Thanks! |
looks good (sorry, travelling the rest of the week, so I dont have too much time to comment) |
No worries @spinscale. This should be good to go (minus the test). I've tested it on my local machine and it appears to work as expected. I'll work on getting the test done, but I'm not so familiar with java tests (I mostly come from a python background), but I'm sure I'll figure something out. |
hey, I took a look at this and like it so far, I also have a test ready, so if you are ok with this, I take your work, add a test and push it. |
Awesome! I guess we were both working on tests at the same time :). If you don't mind, I'd like to get some help figuring out where my tests could be improved, and push this whole thing in together. That way I can actually write quality tests moving forward! If you prefer email communication you can reach me at my github username @gmail.com |
XContentBuilder builder = channel.newBuilder(); | ||
String errorJSON = builder.startObject().field("error","JSONP is disabled. Set http.jsonp.enable = true to allow JSONP responses.").endObject().string(); | ||
channel.sendResponse(new BytesRestResponse(FORBIDDEN,"application/javascript",errorJSON)); | ||
} catch (IOException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you do not supply the the content-type, you can just hand over the builder and do not need to create a string object here.
Also I would just return JSON is disabled
instead of mentioning the config option here. The shorter the better IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair, but standards dictate that all JSONP responses should (at least in theory) return an application/javascript mime type.
http://stackoverflow.com/questions/111302/best-content-type-to-serve-jsonp
As for the note, I'll change it. I agree, it's a bit long-winded :)
actually my test looks nearly the same, nice work! Just had some minor nitpicks. Apart from that we are good and almost ready to go! Maybe you can squash the commits and force push so we only have one history commit, when getting it into ES. |
Cool - thanks a ton for your help in all this @spinscale. One last thing, I notice that my disabled test seems to fail. Any idea why? It could very well be my eclipse env, but it seems like I should release something or call some sort of finished routine. For my JsonpOptionEnabledTest things are all green, but not for the disabled test. Traceback inline.
|
JsonpOptionEnabledTest test output:
|
couple of things here: The test fails, because you do not free resources.. the builder actually has a I suggest the following code to fix it and to have the right content type
If you do this, you can change the test to check for this as well
However, the test will fail, as there is a tiny bug in
I think thats it (with the exception of squashing) :-) |
Wonderful! This is exactly what I was looking for. I'll get those commits Thanks for all your help in all this. If you ever find yourself on the west Ryan On Fri, Jun 13, 2014 at 7:36 AM, Alexander Reelsen <notifications@github.com
|
@spinscale this should be good to go. All tests are green and everything works as expected. Squashing. |
This adds the http.jsonp.enable option, which left enabled by default at the reqeust of the Elasticsearch folks. Allowing JSONP responses by default on all API endpoints poses a security risk, and probably shouldn't be used unless it's necessary. This also squashes a couple bugs in NettyHttpChannel.java - JSONP responses were never setting application/javascript as the content-type - The content-type and content-length headers were being overwritten even if they were set
Added the http.jsonp.enable option to configure disabling of JSONP responses, as those might pose a security risk, and can be disabled if unused. This also fixes bugs in NettyHttpChannel * JSONP responses were never setting application/javascript as the content-type * The content-type and content-length headers were being overwritten even if they were set before Closes #6164
Finally. Thanks a lot for all the time you invested here! Highly appreciated. Will come back to your west coast offer on the long run :-) |
Woo hoo! Thanks a lot for your help as well :) |
This poses a significant security threat by being on by default. If an attacker can entice a user to load a legitimate ElasticSearch query as a script tag, they can effectively bypass SOP and exfiltrate the contents of a local ElasticSearch instance (or bypass firewall rules by using a victim's browser to talk to a remote instance).
This would be trivial to implement from an attackers perspective, and I'll be submitting a pull request to the BeEF project (https://github.com/beefproject/beef) to automate this sort of attack, as it'll be useful during a pentesting engagement.
If you have any questions or change requests please let me know, and I'll be more than happy to accommodate.
Thanks!
Fitblip