-
-
Notifications
You must be signed in to change notification settings - Fork 679
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
Discussion: serving content when resource is not meant to be accessed directly #1009
Comments
It's the user interface's job to protect against that based on how the XML or JSON is being embedded in a UI. This is not the job of a server. If I missed something can you give me a more specific example? PS: THANK YOU for opening a new issue and moving the content here. I appreciate this Elar! |
Dear @NeilMadden and @Bankde can you take a look at this issue and provide comments, please? |
I think he mean directly access the URL; e.g. attacker sending API link to the victim. |
If If you can make request directly to URL where API is serving XML content with An example: <?xml version="1.0"?>
<html:html xmlns:html='http://www.w3.org/1999/xhtml'>
<html:script>
alert(1);
</html:script>
</html:html> And if API is serving some other content, which is not XML or JSON? Then what? |
I have reviewed this issue a bit and here is my opinion. Current browsers consider XML mime type to be active content (thus executing Javascript/XSS). There are also other contents that fall into this categories. This link has very good summary on the content-types with XSS issue: https://github.com/BlackFan/content-type-research/blob/master/XSS.md I'm not sure I understand this correctly
It also stated that text/xml may also include processing instructions:
I understand that the RFC does not explicitly state about the browsers but overall it looks like XML being executed is correct by the design (or until browsers decide to change this behavior). If this is true, then the responsibility of preventing vulnerabilities (not only XSS, but also many others) should normally fall into the input validation and encoding categories. I'm not confident in XML topic though. I will need more time for the research later. |
First, let's not get stuck only to JSON and XML answers from an API - we don't have any requirement for such limitation.
That is the same like to say "XSS is input validation problem" because execution HTML / JavaScript is correct by design. It is not input validation, it is output encoding problem. And if encoding is not the solution, then serving content correctly ( The problem here is, in this example, XML response from an API is not meant to be executed in browser as a response for direct request to URL of an API - it's meant to be loaded by browser via user interface (using JavaScript). |
We might both have the same idea but just from different perspectives. I personally think it is both. We have to ensure that user input do not cross into the domain of script/HTML structure. If we concatenate the user input directly into the Javascript content (don't do this) then output encoding may not be enough. My current breakdown idea is like this:
I get your point. And unlike json, this XML complex thing makes my head spin. |
By the way, I completely agree with @elarlang concern. While I don't have an exact idea of recommendation, I propose to add some general idea like this into the requirements:
This will cover not only json but also XML and others as well and let the complex details to the developers to decide according to their applications. |
Content-Type
What is the reason that you want/need to show answer from an API in browser? (inline vs attachment). In practice, what My idea is that Choosing correct Solution for current problem should be: to avoid response from an API being rendered as active content, serve it as attachment. ... and this is what deleted requirement V14.4.2 covered. Why I got stuck in #721 was:
If you don't need to navigate to this URL, it should be served as attachment and a lot of problems could be solved. |
That's true haha. (Forgive me if I sound like a troll), the point is I feel like "API response as an attachment" is also a dirty hack more or less.
Not much, some developers love to see it visualize though. That's the reason why browsers support json beautifier.
It is a positive side-effect but not the main concern of V14.4.2 though. In my opinion, deleting that one when it's deprecated then propose a new accurate requirement that matches you concern is a better call.
The problem is that while it solves problem for XML, it improves nothing for JSON. Not that I turn your idea down, I just feel that it's also a hacky solution and I don't want the new requirement to effect JSON API application too much. Should it be specifically for XML? or L3 for JSON?
I don't want it to be duplicate or sound like a new V14.4.1 though. But I don't want it to be too specific or a hindrance to the applications that do not have this XSS issue either. You may change it as you wish. |
Disagree. Serving content as attachment when you don't have any business logic need to watch it directly it's correct solution. You can not see any piece of content "out of context" (not only XSS problem)
"Some developers may like to see visualized JSON beautifier" is not really valid argument to remove some security requirement.
Can you please clarify - what exactly is deprecated?
Like I wrote many times, don't get stuck with XML and JSON examples. Till we don't have requirement to limit API output only for JSON and XML, we need to take in account, it can be whatever.
That's correct and I have never said anything against that but it is valid ONLY if you have
Disagree. Arguments? How does it affect JSON API? Just this "developers want to see" situation or something else? |
Apologize. My head is so spinning over so many issues tangling together @_@
Ah I get your perspective now. It does make sense.
At first, I wouldn't want a change that affect too much, against the norm, and with little improvement. But that argument was from my own mistake and confusion that this will be a new requirement; this requirement is actually been here for quite some time so there should not be any issue to developers if we bring it back. Another mistake is that I thought it exists solely to mitigate RFD and mime type confusion (which original commit stated) so I called a remove too early as I saw that most of RFD and mime type confusion is deprecated and handled by other requirements. However, from this discussion, it does also prevent browser execution on non-business-logic-related content too. For the level, I originally preferred it to be L3. I'm still inclined that proper encoding/validation can prevent XML XSS and this is just a defense in depth. However, I find the idea to be similar to CSP so I'm ok if it's L1, L2, L3 (same level as CSP). Thank you for tackling with my head. I'm still opened to the discussion but I need to rest for today. |
Allow me to chime in. APIs are the main orchestrator for the type that gets returned. What the issue should clearly say is that "The client should not parse the returned response as they please it to be", MIME confusion is a direct issue here, executing JSON, running a JPG file as a script, etc. This is related to anything being returned to ANY client, and not to browsers only. It's similar with how TLS can be configured to enforce the server config over the client one. The API if it returns XML, should clearly say "This is XML", do what you would do with XML. If the backend is worried about the client behaviour, then it should clearly secure its response. XML is supposed to execute, and should for all I know if it's sent as is. Maybe something to the lines of "Be aware of the execution paths of your responses" which is still vague, but I am trying to get this a bit closer to how APIs are and should work. Going for undercuts just make security look cheap and provide a false sense of security. |
Discussed this topic with @ThunderSon and I provided some more food for brain. TL;DR
Target: only resources which are not meant to be accessed directly but user/browser can do it - non-autenticated users or users in application which uses cookie-based sessions. Direct access - no extra defense
Direct access - defense
|
Test | Content-Type |
Content | HTML/XML executed | JavaScript executed |
---|---|---|---|---|
link | text/html |
HTML | No | No |
link | text/html |
JSON | No | No |
link | application/json |
JSON | No | No |
link | text/xml |
XHTML | No | No |
link | image/svg+xml |
SVG | No | No |
Direct access - defense Content-Security-Policy: sandbox
Test | Content-Type |
Content | HTML/XML executed | JavaScript executed |
---|---|---|---|---|
link | text/html |
HTML | Yes | No |
link | text/html |
JSON | Yes | No |
link | application/json |
JSON | No | No |
link | text/xml |
XHTML | Yes | No |
link | image/svg+xml |
SVG | Yes | No |
That's very nice summary and comparison with CSP. Thank you. Thought from me:
Lastly, I also like this idea. I'm interested if there is any reference (as a security requirement/practice) to this?
|
Some more options to achieve "do not render content out of context in browser":
The point is - there are many ways to achieve goal do not serve or render response content on direct access to resource, which is not intended for that. The reason, why #721 stayed opened, was that purpose for 14.4.2 was not clear and that is why it got (too fast) deleted as well. So just bringing back this requirement with the same text is not the best solution and instead of "technical requirement" maybe we should ask "what vector must be taken away". Direction for that requirement should contain goals:
At the moment it seems, that technically the most bullet-proof solution could be to bring back previous 14.4.2, removing API part from it and adding description, what goal this requirement have. So, opened for discussion and arguments if there is some better alternatives. |
I am tilted more towards the CSP solution:
The benefit of using The last defense option looks like using the |
Hi, I don't want this to go stale. I'm ok with either way.
I'm not a fan of adding specific implementations into the general guidelines; they are usually fragile. Developers are free to add them in their project as pleased though. |
One more random argument, CSP vs attachment but now against attachment - if user is forced to download some attacker controlled content, it is better for application but worse for user. Let's say application is serving HTML content and serving it as attachment. If user opens it in own machine, all the scripts inside HTML can be now executed without |
It's only true when you have multiple ways to achieve the same goal - like with CSRF. If we want to keep this requirement in V14.4 category, it should direct clearly what kind of configuration options it covers. |
Thank you for bringing that up. I realize we may not want something with possible negative effect in the guidelines so...
This way, we are enforcing the CSP (not yet perfect but no downside) while still open to other solutions (that devs have to take care by themselves) |
This just came in today: Exactly what we are discussing, if we want to consider the endpoint returning the content to be our API at hand. |
A bit late to this party (long weekend in the UK). I am generally in favour of To add to what @elarlang says about running a local file may result in CSP bypass, it looks like the origin of a |
I'm also happy with wording like @Bankde suggests. |
First big decision for wording is - do we limit it for API or we going to build general "do not allow browser to render responses if they are not intended to be rendered by the application itself"? |
I'd love it to cover all the cases.
|
Just a note on 12.5.2
|
How about this? My previous proposal still stands and feel free to adjust/criticize/edit/reword any way you like.
|
Re-read it all and #1009 (comment) is still valid opinion, goal and proposal. |
If resources are only meant to be used with JavaScript requests, a possible solution would be for the client to set a specific header ( Another solution is to set a content-type to |
It is one possible way of detecting it, but nowadays I would take direction to
This is technically incorrect and against |
Any request defense that requires certain non-session headers can be directly evaded.
|
So can you come up with a proposed requirement wording @elarlang because I am still not sure what we are asking developers to do. |
So, based on #1009 (comment) I tried to put my ideas here (wording needs some work): |
@tghosth - does the last comment makes it more clear for you? |
Ok so how about:
|
2 things to improve:
|
So how about this:
|
I tried to change to order, fix typo end remove the last part:
|
The requirement got in as 50.5.3 as it was proposed:
@tghosth - do you have any additional recommendations to this wording or we can close this? |
Makes sense to me |
Problem: there is no requirement which says, that API responses must be in JSON or XML format.
If they are not, and those return some content of file for example, then with direct request to the API response, the content may be executed as HTML, XML and/or JavaScript. Problem is valid for not authenticated API access or when session is cookie-based.
If an API returns XML output with
Content-Type: text/xml
and if an attacker can fully control the content, it's already possible to execute JavaScript from that situation with direct request.Previously there was requirement 14.4.2, which was covering this kind of situations - serve them as attachment = no execution problem:
There was issue #721 to discuss the meaning for that, but requirement was deleted via issue #1004.
Like in file section there is requirement:
I think something similar must be for API responses.
Or in general - everything, which is not intended to serve with direct request, can have
Content-Disposition: attachment
header and solve a lot of content execution problems.No proposals from my side yet, I was not supporting removing 14.4.2 for the reasons above, so now opened for discussions - what is the correct defense in this kind of situation and is it covered by ASVS? If not, we should fill the cap.
ping @NeilMadden @jmanico @Bankde
The text was updated successfully, but these errors were encountered: