4 ms·

No, I wasn't assuming the API will remain identical -- I saw the "should implement a REST API" part of your recommendation. Here's what's wrong with your recom

No, I wasn't assuming the API will remain identical -- I saw the "should implement a REST API" part of your recommendation. Here's what's wrong with your recommendation: 1. An AJAX REST API request doesn't necessarily mean "application/json". It could also be "application/x-www-form-urlencoded", which won't necessarily trigger a pre-flight request. 2. Your advice goes into detail on an aspect that is not the crux of this particular vulnerability ("Access-Control-Allow-Origin") but not on the aspects that are: 2a. How to handle the pre-flight request, which is the thing that would actually block wrong-site requests. 2b. If you're relying on a Content-Type of "application/json" to trigger a pre-flight request, then it should be made clear that this is now a security check -- it's common for server code to ignore the Content-Type header as long as the body is valid.

chrisfosterelli 7y ago
Ah I see -- so it sounds like you agree the advice is not wrong, but it's not in the level of detail you would like. It is only a 500 character example unfortunately, the details of the server API are not really in the scope of my post but I appreciate you elaborating here.
- Thorrez 7y ago
  The advice is wrong. Someone could follow the advice in the article and still be vulnerable. The article should mention the actual vulnerability: CSRF, because there's a state-changing HTTP handler with no CSRF protection. CORS doesn't automatically protect against CSRF, and people shouldn't be given that impression.
  - eranation 7y ago
    I've been seeing more and more developers not being aware about CSRF. Maybe the reason is that frameworks got very good built in support for CSRF protection, which caused CSRF vulnerabilities to be less common, which lead to OWASP to remove it from #8 on the list in 2013 to to not be explicitly on the top 10 at all in 2017. (marked as "Merged or retired, but not forgotten"): https://www.owasp.org/images/7/72/OWASP_Top_10-2017_%28en%29.pdf.pdf https://www.owasp.org/images/7/72/OWASP_Top_10-2017_%28en%29...) but it's still a very valid vulnerability that I think every developer should understand.
    - majewsky 7y ago
      > Merged or retired, but not forgotten Oh the irony.
  - chrisfosterelli 7y ago
    > Someone could follow the advice in the article and still be vulnerable. This applies to any advice that is not an actual reference implementation in code. There's only a state-changing GET handler because they chose to work around CORS and you can't POST for images. Implemented properly this CORS approach does protect against CSRF without the need for an explicit token. I've written more about this before, with concrete examples in Node for example: https://fosterelli.co/dangerous-use-of-express-body-parser https://fosterelli.co/dangerous-use-of-express-body-parser
    - Thorrez 7y ago
      >There's only a state-changing GET handler because they chose to work around CORS and you can't POST for images. I'm not sure of that. There's tons of stuff in existence that has state changing GET handlers that don't have the goal of working around CORS, check out the HN upvote button. It's conceivable to me that if Zoom managed to send a CORS response header, they might have used an XHR with GET (considering GET is the default for XHR). But a discussion of GET vs POST seems largely irrelevant, because POST doesn't solve the problem. >Implemented properly this CORS approach does protect against CSRF without the need for an explicit token. The problem is that your definition of "properly" is "must reject requests with a Content-Type other than application/json". That's a non-obvious definition of properly that's not stated in the CORS article. That body parser article is better (although it does seem mistaken about XHRs being unable to make cross-origin requests). The CORS article would be better if it mentioned the Content-Type requirement and linked to the body parser article. Although implementing CSRF protection via Content-Type is somewhat fragile. Someone might want to support a new Content-Type in the future and not realize that the Content-Type restriction is security-critical. Implementing it via an Origin check (or an X-Not-Simple-CORS: true header) would be clearer.
      - etatoby 7y ago
        State-changing GET handlers are against the HTTP concept and specs and are trouble waiting to happen. Each and every one of them were designed and implemented by incompetent and/or inexperienced developers.
- cakoose 7y ago
  It's not just the level of detail. For example, what if your advice included one fewer detail: > The webserver listening in on "localhost:19421" should implement a REST API. This will ensure that only Javascript running on the zoom.us domain can talk to the localhost webserver. Do you see how that is wrong?

chrisfosterelli 7y ago

Ah I see -- so it sounds like you agree the advice is not wrong, but it's not in the level of detail you would like. It is only a 500 character example unfortunately, the details of the server API are not really in the scope of my post but I appreciate you elaborating here.

Thorrez 7y ago
The advice is wrong. Someone could follow the advice in the article and still be vulnerable. The article should mention the actual vulnerability: CSRF, because there's a state-changing HTTP handler with no CSRF protection. CORS doesn't automatically protect against CSRF, and people shouldn't be given that impression.
- eranation 7y ago
  I've been seeing more and more developers not being aware about CSRF. Maybe the reason is that frameworks got very good built in support for CSRF protection, which caused CSRF vulnerabilities to be less common, which lead to OWASP to remove it from #8 on the list in 2013 to to not be explicitly on the top 10 at all in 2017. (marked as "Merged or retired, but not forgotten"): https://www.owasp.org/images/7/72/OWASP_Top_10-2017_%28en%29.pdf.pdf https://www.owasp.org/images/7/72/OWASP_Top_10-2017_%28en%29...) but it's still a very valid vulnerability that I think every developer should understand.
  - majewsky 7y ago
    > Merged or retired, but not forgotten Oh the irony.
- chrisfosterelli 7y ago
  > Someone could follow the advice in the article and still be vulnerable. This applies to any advice that is not an actual reference implementation in code. There's only a state-changing GET handler because they chose to work around CORS and you can't POST for images. Implemented properly this CORS approach does protect against CSRF without the need for an explicit token. I've written more about this before, with concrete examples in Node for example: https://fosterelli.co/dangerous-use-of-express-body-parser https://fosterelli.co/dangerous-use-of-express-body-parser
  - Thorrez 7y ago
    >There's only a state-changing GET handler because they chose to work around CORS and you can't POST for images. I'm not sure of that. There's tons of stuff in existence that has state changing GET handlers that don't have the goal of working around CORS, check out the HN upvote button. It's conceivable to me that if Zoom managed to send a CORS response header, they might have used an XHR with GET (considering GET is the default for XHR). But a discussion of GET vs POST seems largely irrelevant, because POST doesn't solve the problem. >Implemented properly this CORS approach does protect against CSRF without the need for an explicit token. The problem is that your definition of "properly" is "must reject requests with a Content-Type other than application/json". That's a non-obvious definition of properly that's not stated in the CORS article. That body parser article is better (although it does seem mistaken about XHRs being unable to make cross-origin requests). The CORS article would be better if it mentioned the Content-Type requirement and linked to the body parser article. Although implementing CSRF protection via Content-Type is somewhat fragile. Someone might want to support a new Content-Type in the future and not realize that the Content-Type restriction is security-critical. Implementing it via an Origin check (or an X-Not-Simple-CORS: true header) would be clearer.
    - etatoby 7y ago
      State-changing GET handlers are against the HTTP concept and specs and are trouble waiting to happen. Each and every one of them were designed and implemented by incompetent and/or inexperienced developers.
cakoose 7y ago
It's not just the level of detail. For example, what if your advice included one fewer detail: > The webserver listening in on "localhost:19421" should implement a REST API. This will ensure that only Javascript running on the zoom.us domain can talk to the localhost webserver. Do you see how that is wrong?