nccgroup / sobelow

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

CAStore error when running as standalone script #133

Closed mbramson closed 1 year ago

mbramson commented 1 year ago

When running mix sobelow, I encounter the following error on version 0.12.1 of sobelow.

This occurs both locally and in CI (CircleCI).

code/elixir/test > mix archive.install hex sobelow
Resolving Hex dependencies...
Dependency resolution completed:
New:
  castore 1.0.1
  jason 1.4.0
  sobelow 0.12.1
* Getting sobelow (Hex package)
* Getting castore (Hex package)
* Getting jason (Hex package)
Resolving Hex dependencies...
Dependency resolution completed:
Unchanged:
  castore 1.0.1
  jason 1.4.0
All dependencies are up to date
==> jason
Compiling 10 files (.ex)
Generated jason app
==> castore
Compiling 1 file (.ex)
Generated castore app
==> sobelow
Compiling 51 files (.ex)
Generated sobelow app
Generated archive "sobelow-0.12.1.ez" with MIX_ENV=prod
Are you sure you want to install "sobelow-0.12.1.ez"? [Yn] Y
* creating /Users/mbramson/.asdf/installs/elixir/1.13.4/.mix/archives/sobelow-0.12.1

code/elixir/test > mix sobelow --config
** (UndefinedFunctionError) function CAStore.file_path/0 is undefined (module CAStore is not available)
    CAStore.file_path()
    lib/sobelow.ex:552: Sobelow.get_sobelow_version/0
    lib/sobelow.ex:581: Sobelow.maybe_prompt_update/2
    lib/sobelow.ex:34: Sobelow.run/0
    (mix 1.13.4) lib/mix/task.ex:397: anonymous fn/3 in Mix.Task.run_task/3
    (mix 1.13.4) lib/mix/cli.ex:84: Mix.CLI.run_task/2
houllette commented 1 year ago

Hmm I'm having difficulties recreating this error - we've had success deploying fresh on multiple different systems. But I will try to look into this some more, in the meantime mix sobelow --private will avoid calling the function which triggers this error.

houllette commented 1 year ago

It's also worth noting that mix archive.install hex sobelow is no longer the recommended way of installing Sobelow, but instead mix escript.install hex sobelow - not sure if that will change anything, but worth a shot!

houllette commented 1 year ago

I was able to recreate the issue finally! Will dig in more now!

houllette commented 1 year ago

Ok, so I've determined how to fix the error if you wish to run the application as an archive - you'll need to additionally run mix archive.install hex castore (and also mix archive.install hex jason if you plan on using json output).

Looks like this is a similar issue as https://github.com/nccgroup/sobelow/issues/89 which is what prompted the switch from an archive.install to an escript.install if you desire to use Sobelow as a standalone tool - so that might fix it for you!

houllette commented 1 year ago

Alright, final update:

Normally, installing as an escript would fix this for you. But in this particular case, CAStore reads from priv/ which is not supported by escript.build (which is where CAStore reads the .pem file from).

Temporary Workaround

As mentioned before, if you wish to install / use Sobelow as a standalone application - please use --private and it will avoid the problematic code.

Long Term Solution(s)

Option 1

The only reason we have to use CAStore in the first place is because we cannot exclusively use OTP 25+ yet which would allow us to use :public_key.get_certs() and avoid having to rely on an external CA trust store.

Once it has been determined we can move up the minimum supported version of the OTP we could deprecate the use of CAStore in favor of built in Erlang functionality.

Option 2

Personally I don't like the whole, "reach out to a website to check for the current version of the app to see if the local version is out of date" flow in the first place - especially since the website it reaches out to is just another thing to keep updated when a new version is pushed up to hex (since it doesn't auto update).

So this option would just be to gut the functionality of checking for updates.

Option 3

Elect to introduce a new dependency to take over HTTP request duties so we don't have to worry about this stuff anymore - I don't love this option since it introduces more dependencies for a pretty simple, one-off function

mbramson commented 1 year ago

Thank you for the blazing fast response! Appreciate the workaround and thoughts on the go-forward path.

houllette commented 1 year ago

@mbramson - the new version (0.12.2) has been published with what I think fixes the issue!

Closing Thoughts / Decision Context

I made a decision to pursue an option not outlined in my previous post - which was moving back to :verify_none for the SSL verify option.

It was not an easy decision but I hope to make it clear here as a matter of record. It's worth mentioning that the aforementioned Option 1 will be the long term solution once OTP 25 is exclusively used, but the other options could not be pursued further for a myriad of reasons:

Option 3 (the easy one to rule out)

Even if we used a more appropriate HTTP Library, we would still have to verify certificates at some point - so we'd run into the same problems as now basically.

Option 2

While I still don't love the overall concept of reaching out to a web server to check for current version (not to mention I don't personally have the ability to update said version on https://sobelow.io currently) - I believe it's a necessary evil.

Basically the folks who benefit from this occasional HTTP request flow are the very folks who need it - local global installations Sobelow that are not tied to application code (in which versions can be monitored and bumped up via Dependabot)

So why go with :verify_none?

The simple answer is that we were basically doing that up until my initial feeble attempt to fix the warning from not having verify configured so we aren't introducing risk that wasn't already there.

The risk being that we cannot guarantee that our request isn't being sniffed through MitM - which in this instance is just...a public version number. If there were more robust HTTP calls being made, I'd be a lot more concerned - but I think I can feel ok-ish leaving it how it is for now.

Additionally, at least by purposefully setting it I believe it still does away with the warning (which was the original goal).

Takeaways

I had to move quickly on this since, as I discovered, due to the GitHub Action for Sobelow installing via escript this bug was causing GitHub CI jobs to fail everywhere that used it (see issue here).

Overall I am learning the nuances of building tooling at this scale and I appreciate the patience here - this incident is making it even more clear to me how the testing pipeline for Sobelow needs to be overhauled since all my work on the repo thus far was focused on just testing through mix deps installation.