Security checks are run against raw string AST instead against compiled AST

[skylerparr]

Due to the way sobelow checks for vulnerabilities, it reads the raw code into a string and converts the string code to elixir AST:

sobelow/lib/sobelow/config.ex

Line 80 in 26589a5

ast = Parse.ast(filepath)

sobelow/lib/sobelow/parse.ex

Line 41 in 8702702

{:ok, ast} = Code.string_to_quoted(read_file(filepath))

Meaning that it analyzes the raw code as a string and not the final compiled code.

Here is a trivial example that would bypass the XSS checks:

defmodule RouterHack do
    quote do
      plug(:accepts, ["html"])
      plug(:fetch_session)
      plug(:fetch_flash)
      plug(:put_secure_browser_headers)
    end
  end
end

defmodule MyApp.Router do
  use MyApp.Web, :router

  pipeline :browser do
    require RouterHack
    RouterHack.get_browser_pipeline()
  end
end

Since sobelow is only checking the original code as a string, this macro is completely unchecked and is allowing the security vulnerability.

This is an easy way to bypass the sobelow security checks, and macros can live pretty unchecked. With a language like Elixir; which is macro heavy, really devalues what sobelow provides.

I recommend investigating a way to inspect the generated beam files (as the dialyzer works) or see if the core team can add a function that returns the generated AST before beam file generation (if there isn't one already. I dug around for an example and I couldn't find anything, but that doesn't mean it doesn't exist).

[GriffinMB]

I agree that some kind of macro expansion would be nice. Though, I think operating on beam files directly would end up being a worse experience, since you may lose the context you get from operating on AST directly.

This is something I toy with from time to time, but it isn't high priority. The vast majority of the time, with common and semi-common configurations, the lack of expansion will result in false positives rather than false negatives. And someone deliberately attempting to bypass the security checks is not within the scope of the tool.

Aside - in your example, which XSS checks are being bypassed? I think the outcome from your example would be a false positive about secure headers, and a false negative for CSRF.

[skylerparr]

Haha. Sorry about that, I gave you a pretty out of context example unless you memorized the phoenix plugs. The example is missing the plug(:protect_from_forgery) plug.

[AndrewDryga]

This is the reason why Sobelow crashes when you use function calls to extend list of socket options, eg:

socket "/gateway", API.Gateway.Socket, API.Sockets.options()

leads to

* (FunctionClauseError) no function clause matching in Sobelow.Config.CSWH.check_socket_options/1    
    
    The following arguments were given to Sobelow.Config.CSWH.check_socket_options/1:
    
        # 1
        {{:., [line: 25, column: 49], [{:__aliases__, [line: 25, column: 38], [:API, :Sockets]}, :options]}, [line: 25, column: 50], []}
    
    Attempted function clauses (showing 3 out of 3):
    
        defp check_socket_options([{:websocket, options} | _]) when is_list(options)
        defp check_socket_options([_ | t])
        defp check_socket_options([])
    
    lib/sobelow/config/cswh.ex:40: Sobelow.Config.CSWH.check_socket_options/1
    lib/sobelow/config/cswh.ex:29: anonymous fn/2 in Sobelow.Config.CSWH.handle_sockets/2
    (elixir 1.14.4) lib/enum.ex:975: Enum."-each/2-lists^foreach/1-0-"/2
    lib/sobelow.ex:94: Sobelow.run/0
    (mix 1.14.4) lib/mix/task.ex:421: anonymous fn/3 in Mix.Task.run_task/4
    (mix 1.14.4) lib/mix/cli.ex:84: Mix.CLI.run_task/2