Skip to content

Security: avoid requests with NULL characters (0x00) on GET #9350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions readthedocs/core/middleware.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import structlog
import time

import structlog
from django.conf import settings
from django.contrib.sessions.backends.base import SessionBase, UpdateError
from django.contrib.sessions.middleware import SessionMiddleware
Expand All @@ -11,7 +11,6 @@
)
from django.utils.cache import patch_vary_headers
from django.utils.http import http_date
from django.utils.translation import gettext_lazy as _

log = structlog.get_logger(__name__)

Expand Down Expand Up @@ -192,3 +191,27 @@ def __call__(self, request):
response = self.get_response(request)
response['Referrer-Policy'] = settings.SECURE_REFERRER_POLICY
return response


class NullCharactersMiddleware:

"""
Block all requests that contains NULL characters (0x00) on their GET attributes.

Requests containing NULL characters make our code to break. In particular,
when trying to save the content containing a NULL character into the
database, producing a 500 and creating an event in Sentry.

NULL characters are also used as an explotation technique, known as "Null Byte Injection".
"""

def __init__(self, get_response):
self.get_response = get_response

def __call__(self, request):
for key, value in request.GET.items():
if "\x00" in value:
raise SuspiciousOperation(
f"There are NULL (0x00) characters in at least one of the parameters ({key}) passed to the request." # noqa
)
return self.get_response(request)
16 changes: 15 additions & 1 deletion readthedocs/rtd_tests/tests/test_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@

from corsheaders.middleware import CorsMiddleware
from django.conf import settings
from django.core.exceptions import SuspiciousOperation
from django.http import HttpResponse
from django.test import TestCase, override_settings
from django.test.client import RequestFactory
from django_dynamic_fixture import get

from readthedocs.builds.constants import LATEST
from readthedocs.core.middleware import ReadTheDocsSessionMiddleware
from readthedocs.core.middleware import (
NullCharactersMiddleware,
ReadTheDocsSessionMiddleware,
)
from readthedocs.projects.constants import PRIVATE, PUBLIC
from readthedocs.projects.models import Domain, Project, ProjectRelationship
from readthedocs.rtd_tests.utils import create_user
Expand Down Expand Up @@ -259,3 +263,13 @@ def test_main_cookie_samesite_lax(self):

self.assertEqual(response.cookies[settings.SESSION_COOKIE_NAME]['samesite'], 'Lax')
self.assertTrue(self.test_main_cookie_samesite_none not in response.cookies)


class TestNullCharactersMiddleware(TestCase):
def setUp(self):
self.factory = RequestFactory()
self.middleware = NullCharactersMiddleware(None)

def test_request_with_null_chars(self):
request = self.factory.get("/?language=en\x00es&project_slug=myproject")
self.assertRaises(SuspiciousOperation, lambda: self.middleware(request))
1 change: 1 addition & 0 deletions readthedocs/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ def USE_PROMOS(self): # noqa
return 'readthedocsext.donate' in self.INSTALLED_APPS

MIDDLEWARE = (
'readthedocs.core.middleware.NullCharactersMiddleware',
'readthedocs.core.middleware.ReadTheDocsSessionMiddleware',
'django.middleware.locale.LocaleMiddleware',
'django.middleware.common.CommonMiddleware',
Expand Down