Skip to content

Commit

Permalink
๐Ÿ”’๏ธ Protect against CSRF attacks (#5)
Browse files Browse the repository at this point in the history
* ๐Ÿ”’๏ธ Protect against CSRF attacks

CORS and the JSON API heavily reduced the
attack surface here, but especially the upload
endpoints were still vulnerable to CSRF attacks.

All endpoints on the API that use methods
beyond GET, HEAD or OPTIONS now require
the presence of a csrf token (generated
by the UI on first load) AND a X-CSRF-Token
header with a matching value (Double Submit
Cookie), unless authorization is done through
an API-Key instead of a browser session.

Additionally, the SameSite attribute now is
set to "Strict" by default.

* ๐Ÿ”’๏ธ Expand CSRF protection to plugins

Plugins are currently by default *opted out* of CSRF protection, to ensure
backwards compatibility with third party plugins in the field. However,
in a future OctoPrint version this will become a default *opt in*. Plugin
authors are advised to adjust the implementation with an explicit decision
via a logged warning generated in the default implementation. Additionally,
plugins that have opted out their whole plugin will generate a warning in
the startup messages.

A decorator to exempt individual endpoints from CSRF protection as certain
workflows require (see for example the auth request in the appkeys plugin)
is provided.

The general rule of thumb should be to opt in and only exempt those non
GET/HEAD/OPTIONS endpoints that really need to be usable without an
API key or CSRF cookie/header combo. In practice, that should be pretty
much non.

* ๐Ÿ› Fix a left-over from a local test

* ๐Ÿ› Fix wrong setting in E2E test config.yaml

* ๐Ÿง‘โ€๐Ÿ’ป Set CSRF header for $.ajax as well

We already ensure the header is set in the client lib and on the
file upload widgets, but many third party plugins don't use the
client lib but rather $.ajax directly and thus break without this.

* ๐Ÿ“ Improve docs on CSRF protection
  • Loading branch information
foosel committed Sep 19, 2022
1 parent 0db80dc commit 59a0c8e
Show file tree
Hide file tree
Showing 22 changed files with 402 additions and 20 deletions.
1 change: 1 addition & 0 deletions .github/fixtures/with_acl/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ accessControl:
salt: zXmvzI3uWuTLkSPOEfA2ZLwn3f3sGUNS
devel:
enableRateLimiter: false
enableCsrfProtection: false
plugins:
virtual_printer:
enabled: true
Expand Down
25 changes: 24 additions & 1 deletion docs/api/general.rst
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ check the corresponding checkbox in the API settings dialog.

.. warning::

This means any browser page can send requests to the OctoPrint API. Authorization is still required however.
This means any browser page can send requests to the OctoPrint API. Authorization via an API-Key is still required however.

If CORS is not enabled you will get errors like the following::

Expand All @@ -151,6 +151,29 @@ If CORS is not enabled you will get errors like the following::
the login mechanism (or reusing an existing login session). When accessing OctoPrint
via CORS, you'll therefore always need to use an API key.

.. _sec-api-general-csrf:

CSRF Protection
===============

.. versionadded:: 1.8.3

To protect OctoPrint against `CSRF attacks <https://owasp.org/www-community/attacks/csrf>`_ against the non CORS affected upload endpoints, in case of browser session based authorization the API
is protected using the `Double Submit Cookie mitigation strategy <https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#double-submit-cookie>`_.
On first page load of the UI, the login page or the recovery page, a ``csrf_token_P<port>`` or ``csrf_token_P<port>_R<root>`` cookie is set
that can be read via client-side JavaScript. All requests towards the API that are not ``GET``, ``HEAD`` or ``OPTIONS``
and rely on cookie based authorization (so not on an API key but rather an active login session) are required
to send both the ``csrf_token`` cookie as well as an ``X-CSRF-Token`` header containing its value.

.. note::

If you use the :ref:`JS Client library <sec-jsclientlib>`, this will take care of doing the needful for you. Any code in the *Core UI* calling
API functions through ``$.ajax`` or ``$.get`` or ``$.post`` will also take care of this for you. If you use another library for
accessing OctoPrint's API in a browser context, you'll need to make sure to send the ``X-CSRF-Token`` header yourself. Examples for
several JS frameworks can be found in the `OWASP cheatsheet on CSRF attacks <https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#javascript-guidance-for-auto-inclusion-of-csrf-tokens-as-an-ajax-request-header>`_.
Take a look at the implementations of ``OctoPrintClient.getCookie`` and ``OctoPrintClient.getHeaders`` in ``src/octoprint/static/js/client/base.js``
for details on how to retrieve the cookie value and how to construct the header.

.. _sec-api-general-login:

Login
Expand Down
30 changes: 27 additions & 3 deletions docs/jsclientlib/base.rst
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,39 @@

:returns string: The base url to use to access OctoPrint's API.

.. js:function:: OctoPrintClient.getRequestHeaders(additional)
.. js:function:: OctoPrintClient.getCookie(name)

.. versionadded:: 1.8.3

Returns the value of the cookie with name ``name``. The port based cookie suffix and if
necessary also the script root based suffix will be automatically applied to the ``name``
prior to looking up the value.

:return string: The value of the cookie, if set & accessible, else an empty string.

.. js:function:: OctoPrintClient.getRequestHeaders(method, additional, opts)

.. versionchanged:: 1.8.3

Generates a dictionary of HTTP headers to use for API requests against OctoPrint with all
necessary headers and any additionally provided ones.

At the moment this entails setting the ``X-Api-Key`` header based on the current value of
``OctoPrint.options.apikey`` at a minimum.
``OctoPrint.options.apikey``, if set. Otherwise, for non-cross-domain requests targeting
methods other than ``GET``, ``HEAD`` or ``OPTIONS``, the current CSRF token is read from the
associated cookie and set as ``X-CSRF-Token``.

.. note::

Up until OctoPrint 1.8.3, this method's signature consisted only of the ``additional``
parameter. It has been changed in a backward compatible way so that calling it with the
first parameter being the set of additional headers will still work. This mode of operation
is deprecated however and will be removed in a future version.
A warning will be logged to the debug console accordingly.

:param object additional: Additional headers to add to the request.
:param str method: Method of the request for which to set headers
:param object additional: Additional headers to add to the request, optional.
:param object opts: Additional opts passed to the request, used to read cross domain setting, optional.
:returns object: HTTP headers to use for requests.

.. js:function:: OctoPrintClient.ajax(method, url, opts)
Expand Down
90 changes: 86 additions & 4 deletions src/octoprint/plugin/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -1352,8 +1352,39 @@ def myEcho(self):
flask.url_for("plugin.myblueprintplugin.myEcho") # will return "/plugin/myblueprintplugin/echo"
.. warning::
As of OctoPrint 1.8.3, endpoints provided through a ``BlueprintPlugin`` do **not** automatically fall under
OctoPrint's :ref:`CSRF protection <sec-api-general-csrf>`, for reasons of backwards compatibility. There will be a short grace period before this changes. You
can and should however already opt into CSRF protection for your endpoints by implementing ``is_blueprint_csrf_protected``
and returning ``True`` from it. You can exempt certain endpoints from CSRF protection by decorating them with
``@octoprint.plugin.BlueprintPlugin.csrf_exempt``.
.. code-block:: python
class MyPlugin(octoprint.plugin.BlueprintPlugin):
@octoprint.plugin.BlueprintPlugin.route("/hello_world", methods=["GET"])
def hello_world(self):
# This is a GET request and thus not subject to CSRF protection
return "Hello world!"
@octoprint.plugin.BlueprintPlugin.route("/hello_you", methods=["POST"])
def hello_you(self):
# This is a POST request and thus subject to CSRF protection. It is not exempt.
return "Hello you!"
@octoprint.plugin.BlueprintPlugin.route("/hello_me", methods=["POST"])
@octoprint.plugin.BlueprintPlugin.csrf_exempt()
def hello_me(self):
# This is a POST request and thus subject to CSRF protection, but this one is exempt.
return "Hello me!"
def is_blueprint_csrf_protected(self):
return True
``BlueprintPlugin`` implements :class:`~octoprint.plugins.core.RestartNeedingPlugin`.
.. versionchanged:: 1.8.3
"""

@staticmethod
Expand All @@ -1372,7 +1403,7 @@ def route(rule, **options):
def decorator(f):
# We attach the decorator parameters directly to the function object, because that's the only place
# we can access right now.
# This neat little trick was adapter from the Flask-Classy project: https://pythonhosted.org/Flask-Classy/
# This neat little trick was adapted from the Flask-Classy project: https://pythonhosted.org/Flask-Classy/
if not hasattr(f, "_blueprint_rules") or f._blueprint_rules is None:
f._blueprint_rules = defaultdict(list)
f._blueprint_rules[f.__name__].append((rule, options))
Expand Down Expand Up @@ -1405,6 +1436,27 @@ def decorator(f):

return decorator

@staticmethod
def csrf_exempt():
"""
A decorator to mark a view method in your BlueprintPlugin as exempt from :ref:`CSRF protection <sec-api-general-csrf>`. This makes sense
if you offer an authenticated API for a certain workflow (see e.g. the bundled appkeys plugin) but in most
cases should not be needed.
.. versionadded:: 1.8.3
"""

def decorator(f):
if (
not hasattr(f, "_blueprint_csrf_exempt")
or f._blueprint_csrf_exempt is None
):
f._blueprint_csrf_exempt = set()
f._blueprint_csrf_exempt.add(f.__name__)
return f

return decorator

# noinspection PyProtectedMember
def get_blueprint(self):
"""
Expand All @@ -1422,6 +1474,8 @@ def get_blueprint(self):

import flask

from octoprint.server.util.csrf import add_exempt_view

kwargs = self.get_blueprint_kwargs()
blueprint = flask.Blueprint(self._identifier, self._identifier, **kwargs)

Expand All @@ -1435,9 +1489,14 @@ def get_blueprint(self):
# this attribute was annotated with our @route decorator
for blueprint_rule in f._blueprint_rules[member]:
rule, options = blueprint_rule
blueprint.add_url_rule(
rule, options.pop("endpoint", f.__name__), view_func=f, **options
)
endpoint = options.pop("endpoint", f.__name__)
blueprint.add_url_rule(rule, endpoint, view_func=f, **options)

if (
hasattr(f, "_blueprint_csrf_exempt")
and member in f._blueprint_csrf_exempt
):
add_exempt_view(f"plugin.{self._identifier}.{endpoint}")

if (
hasattr(f, "_blueprint_error_handler")
Expand Down Expand Up @@ -1486,6 +1545,29 @@ def is_blueprint_protected(self):
"""
return True

# noinspection PyMethodMayBeStatic
def is_blueprint_csrf_protected(self):
"""
Whether a blueprint's endpoints are :ref:`CSRF protected <sec-api-general-csrf>`. For now, this defaults to ``False`` to leave it up to
plugins to decide which endpoints *should* be protected. Long term, this will default to ``True`` and hence
enforce protection unless a plugin opts out by returning False here.
If you do not override this method in your mixin implementation, a warning will be logged to the console
to alert you of the requirement to make a decision here and to not rely on the default implementation, due to the
forthcoming change in implemented default behaviour.
.. versionadded:: 1.8.3
"""
self._logger.warning(
"The Blueprint of this plugin is relying on the default implementation of "
"is_blueprint_csrf_protected (newly added in OctoPrint 1.8.3), which in a future version will "
"be switched from False to True for security reasons. Plugin authors should ensure they explicitly "
"declare the CSRF protection status in their BlueprintPlugin mixin implementation. "
"Recommendation is to enable CSRF protection and exempt views that must not use it with the "
"octoprint.plugin.BlueprintPlugin.csrf_exempt decorator."
)
return False

# noinspection PyMethodMayBeStatic
def get_blueprint_api_prefixes(self):
"""
Expand Down
3 changes: 3 additions & 0 deletions src/octoprint/plugins/announcements/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,9 @@ def channel_command(self, channel):
def is_blueprint_protected(self):
return False

def is_blueprint_csrf_protected(self):
return True

##~~ EventHandlerPlugin

def on_event(self, event, payload):
Expand Down
8 changes: 6 additions & 2 deletions src/octoprint/plugins/appkeys/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ def handle_probe(self):
return NO_CONTENT

@octoprint.plugin.BlueprintPlugin.route("/request", methods=["POST"])
@octoprint.plugin.BlueprintPlugin.csrf_exempt()
@no_firstrun_access
def handle_request(self):
data = flask.request.json
Expand Down Expand Up @@ -196,7 +197,7 @@ def handle_request(self):
)
return response

@octoprint.plugin.BlueprintPlugin.route("/request/<app_token>")
@octoprint.plugin.BlueprintPlugin.route("/request/<app_token>", methods=["GET"])
@no_firstrun_access
def handle_decision_poll(self, app_token):
result = self._get_pending_by_app_token(app_token)
Expand All @@ -214,7 +215,7 @@ def handle_decision_poll(self, app_token):

return flask.abort(404)

@octoprint.plugin.BlueprintPlugin.route("/auth/<app_token>")
@octoprint.plugin.BlueprintPlugin.route("/auth/<app_token>", methods=["GET"])
@no_firstrun_access
def handle_auth_dialog(self, app_token):
user_id = current_user.get_name()
Expand Down Expand Up @@ -284,6 +285,9 @@ def handle_decision(self, user_token):
def is_blueprint_protected(self):
return False # No API key required to request API access

def is_blueprint_csrf_protected(self):
return True # protect anything that isn't explicitly marked as exempt

##~~ SimpleApiPlugin mixin

def get_api_commands(self):
Expand Down
3 changes: 3 additions & 0 deletions src/octoprint/plugins/backup/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,9 @@ def on_invalid_backup(line):
def is_blueprint_protected(self):
return False

def is_blueprint_csrf_protected(self):
return True

##~~ WizardPlugin

def is_wizard_required(self):
Expand Down
1 change: 0 additions & 1 deletion src/octoprint/plugins/backup/static/js/backup.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ $(function () {
dataType: "json",
maxNumberOfFiles: 1,
autoUpload: false,
headers: OctoPrint.getRequestHeaders(),
add: function (e, data) {
if (data.files.length === 0) {
// no files? ignore
Expand Down
5 changes: 5 additions & 0 deletions src/octoprint/plugins/corewizard/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ def get_assets(self):
else:
return {}

# ~~ BlueprintPlugin API

def is_blueprint_csrf_protected(self):
return True

# ~~ WizardPlugin API

def is_wizard_required(self):
Expand Down
3 changes: 3 additions & 0 deletions src/octoprint/plugins/discovery/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,9 @@ def discovery(self):
def is_blueprint_protected(self):
return False

def is_blueprint_csrf_protected(self):
return True

##~~ StartupPlugin API -- used for registering OctoPrint's Zeroconf and SSDP services upon application startup

def on_startup(self, host, port):
Expand Down
3 changes: 3 additions & 0 deletions src/octoprint/plugins/logging/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ def set_logging_levels_api(self):
def is_blueprint_protected(self):
return False

def is_blueprint_csrf_protected(self):
return True

def _get_usage(self):
import psutil

Expand Down
3 changes: 3 additions & 0 deletions src/octoprint/plugins/pluginmanager/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -609,6 +609,9 @@ def condition():
def is_blueprint_protected(self):
return False

def is_blueprint_csrf_protected(self):
return True

##~~ EventHandlerPlugin

def on_event(self, event, payload):
Expand Down
3 changes: 3 additions & 0 deletions src/octoprint/plugins/softwareupdate/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -1453,6 +1453,9 @@ def configure_update(self):
def is_blueprint_protected(self):
return False

def is_blueprint_csrf_protected(self):
return True

# ~~ Asset API

def get_assets(self):
Expand Down
11 changes: 11 additions & 0 deletions src/octoprint/server/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@
from octoprint.server.util import (
corsRequestHandler,
corsResponseHandler,
csrfRequestHandler,
loginFromApiKeyRequestHandler,
requireLoginRequestHandler,
)
Expand Down Expand Up @@ -1916,6 +1917,16 @@ def _prepare_blueprint_plugin(self, plugin):
blueprint.before_request(loginFromApiKeyRequestHandler)
blueprint.after_request(corsResponseHandler)

if plugin.is_blueprint_csrf_protected():
self._logger.debug(
f"CSRF Protection for Blueprint of plugin {name} is enabled"
)
blueprint.before_request(csrfRequestHandler)
else:
self._logger.warning(
f"CSRF Protection for Blueprint of plugin {name} is DISABLED"
)

if plugin.is_blueprint_protected():
blueprint.before_request(requireLoginRequestHandler)

Expand Down
2 changes: 2 additions & 0 deletions src/octoprint/server/api/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
from octoprint.server.util import (
corsRequestHandler,
corsResponseHandler,
csrfRequestHandler,
loginFromApiKeyRequestHandler,
loginFromAuthorizationHeaderRequestHandler,
noCachingExceptGetResponseHandler,
Expand Down Expand Up @@ -68,6 +69,7 @@
api.before_request(corsRequestHandler)
api.before_request(loginFromAuthorizationHeaderRequestHandler)
api.before_request(loginFromApiKeyRequestHandler)
api.before_request(csrfRequestHandler)
api.after_request(corsResponseHandler)

# ~~ data from plugins
Expand Down
Loading

0 comments on commit 59a0c8e

Please sign in to comment.