nccgroup / sobelow

Security-focused static analysis for the Phoenix Framework
Apache License 2.0
1.66k stars 92 forks source link

Cross-Site WebSocket Hijacking check false positive for Phoenix LiveView #72

Closed jarrodldavis closed 4 years ago

jarrodldavis commented 4 years ago

When generating a Phoenix app with LiveView, the endpoint has this line of code:

socket "/live", Phoenix.LiveView.Socket, websocket: [connect_info: [session: @session_options]]

Sobelow marks this as a potential for Cross-Site WebSocket Hijacking:

Config.CSWH: Cross-Site Websocket Hijacking - Low Confidence
File: lib/myapp_web/endpoint.ex
Line: 19

socket("/live", Phoenix.LiveView.Socket, websocket: [connect_info: [session: @session_options]])

Curiously, this code for Phoenix Sockets (just above the socket declaration for LiveView) doesn't appear to be flagged at all:

  socket "/socket", StringBookWeb.UserSocket,
    websocket: true,
    longpoll: false

It appears the Config.CSWH only reports potential issues when the websocket configuration is a keyword list, and even ignores check_origin: true when explicitly set in that keyword list rather than implicitly from the endpoint configuration (such as from config/config.exs or config/dev.exs):

https://github.com/nccgroup/sobelow/blob/8634de7994a8ca175ea14b8d7be2ff884a677c60/lib/sobelow/config/cswh.ex#L37-L47

I know that Sobelow errs on the side of over-reporting (which is ultimately a good thing), but since only function declarations can be skipped (which isn't readily available in the endpoint file), it would be nice if the check at least took an explicit check_origin: true configuration into account. It would also be nice if the global endpoint configuration were taken into account, but I understand that's probably unreasonable since it would need to correlate state across multiple files.

GriffinMB commented 4 years ago

Thanks for opening this issue! While you're waiting on a fix, you can use the --mark-skip-all flag to mark it as a false positive. You can find usage details here: https://sobelow.io/#v090

GriffinMB commented 4 years ago

I added checks for the nil and true socket options cases, so this should no longer be incorrectly flagging LiveView's default socket configuration. The change is on master, so you can install and test with mix archive.install github nccgroup/sobelow.

Let me know if that works for you, and I will publish to Hex.

Thanks again!

jarrodldavis commented 4 years ago

@GriffinMB I've tested the updated check and it works great!

GriffinMB commented 4 years ago

Meant to close this, sorry for the delay. This is in the latest release! Thanks again.