Admin search with a null character crashes with "A string literal cannot contain NUL (0x00) characters." on PostgreSQL

[kenichi-cc]

1. Input following URL to browser URL field and access.

​http://localhost/admin/auth/user/?q=%00

2. Crash with following Error.

Environment:


Request Method: GET
Request URL: http://localhost/admin/auth/user/?q=%00

Django Version: 2.1.4
Python Version: 3.6.7
Installed Applications:
['django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.messages',
 'django.contrib.staticfiles',
 'django_extensions',
 'rest_framework',
 'select2',
 'corsheaders',
 .......]
Installed Middleware:
['django.middleware.security.SecurityMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'corsheaders.middleware.CorsMiddleware',
 'django.middleware.common.CommonMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware',
 'django.middleware.clickjacking.XFrameOptionsMiddleware']



Traceback:

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/core/handlers/exception.py" in inner
  34.             response = get_response(request)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/core/handlers/base.py" in _get_response
  126.                 response = self.process_exception_by_middleware(e, request)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/core/handlers/base.py" in _get_response
  124.                 response = wrapped_callback(request, *callback_args, **callback_kwargs)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/contrib/admin/options.py" in wrapper
  604.                 return self.admin_site.admin_view(view)(*args, **kwargs)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/utils/decorators.py" in _wrapped_view
  142.                     response = view_func(request, *args, **kwargs)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/views/decorators/cache.py" in _wrapped_view_func
  44.         response = view_func(request, *args, **kwargs)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/contrib/admin/sites.py" in inner
  223.             return view(request, *args, **kwargs)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/utils/decorators.py" in _wrapper
  45.         return bound_method(*args, **kwargs)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/utils/decorators.py" in _wrapped_view
  142.                     response = view_func(request, *args, **kwargs)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/contrib/admin/options.py" in changelist_view
  1675.             cl = self.get_changelist_instance(request)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/contrib/admin/options.py" in get_changelist_instance
  742.             sortable_by,

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/contrib/admin/views/main.py" in __init__
  81.         self.get_results(request)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/contrib/admin/views/main.py" in get_results
  209.         result_count = paginator.count

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/utils/functional.py" in __get__
  37.         res = instance.__dict__[self.name] = self.func(instance)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/core/paginator.py" in count
  87.             return self.object_list.count()

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/db/models/query.py" in count
  383.         return self.query.get_count(using=self.db)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/db/models/sql/query.py" in get_count
  498.         number = obj.get_aggregation(using, ['__count'])['__count']

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/db/models/sql/query.py" in get_aggregation
  483.         result = compiler.execute_sql(SINGLE)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/db/models/sql/compiler.py" in execute_sql
  1065.             cursor.execute(sql, params)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/db/backends/utils.py" in execute
  100.             return super().execute(sql, params)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/db/backends/utils.py" in execute
  68.         return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/db/backends/utils.py" in _execute_with_wrappers
  77.         return executor(sql, params, many, context)

File "/root/.pyenv/versions/3.6.7/envs/app/lib/python3.6/site-packages/django/db/backends/utils.py" in _execute
  85.                 return self.cursor.execute(sql, params)

Exception Type: ValueError at /admin/auth/user/
Exception Value: A string literal cannot contain NUL (0x00) characters.

[Tim Graham]

This is related to #28201. The exception was ​introduced in psycopg2 2.7+. It's not immediately obvious to me how to handle this and whether the solution should live in the admin or in the postgresql database backend. Without thinking about it too much, I'd lean toward the latter as that would help with other places that user input is passed directly to the ORM.

[Can Sarıgöl]

​PR

[Simon Charette]

I think this should be fixed at the admin layer just like #28201 was fixed at the form later; scanning each query string parameters for '\x00' as suggested in ​this PR will affect performance and is overkill IMO.

In my opinion the issue is that the ​admin is relying on unsanitized request.GET passing to the ORM, that looks like the same class of issues as Model.objects.get(int_field='foo'). The admin should use a form to sanitize the input to rely on #28201 cleansing mechanism.

[Tim Graham]

Agreed.

[Can Sarıgöl]

I thought that the parameter of a single execution doesn't affect too much. when the validator is called, the same case about performance would happen here as well.
Wouldn't it be better if a solution that also solves the raw query parameters? Users wouldn't have to check it out.

[Simon Charette]

I thought that the parameter of a single execution doesn't affect too much. when the validator is called, the same case about performance would happen here as well.

I think it's safe to assume every non-static or cached request handled by Django results in at least one database queries and that it isn't uncommon for queries to have at least one string parameter. Given these assumptions it's unlikely that performing a one time per admin changelist search request validation is ever going to have the same performance implications as performing a search and replace for every '\x00' string parameters provided to the ORM.

Wouldn't it be better if a solution that also solves the raw query parameters? Users wouldn't have to check it out.

I don't think so. User input should be sanitized before feeding it to the ORM.

[Can Sarıgöl]

ok Thanks for the detailed description, I will fix it as per your suggestion.

[Can Sarıgöl]

diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py
index 298e18c57e..4724ccfa96 100644
--- a/django/contrib/admin/views/main.py
+++ b/django/contrib/admin/views/main.py
@@ -1,6 +1,6 @@
 from collections import OrderedDict
 from datetime import datetime, timedelta
-
+from django import forms
 from django.conf import settings
 from django.contrib.admin import FieldListFilter
 from django.contrib.admin.exceptions import (
@@ -35,6 +35,33 @@ IGNORED_PARAMS = (
     ALL_VAR, ORDER_VAR, ORDER_TYPE_VAR, SEARCH_VAR, IS_POPUP_VAR, TO_FIELD_VAR)
 
 
+class ChangeListForm(forms.Form):
+
+    def __init__(self, *args, **kwargs):
+        super(ChangeListForm, self).__init__(*args, **kwargs)
+        for var in {SEARCH_VAR, PAGE_VAR, TO_FIELD_VAR}:
+            field = forms.CharField()
+            field.required = False
+            self.fields[var] = field
+
+    def clean(self):
+        query = self.data.get(SEARCH_VAR)
+        if '\x00' in query:
+            raise forms.ValidationError('Null characters are not allowed.')
+
+        page_num = self.cleaned_data.get(PAGE_VAR)
+        if not page_num:
+            page_num = 0
+            self.cleaned_data[PAGE_VAR]  = page_num
+
+        to_field = self.cleaned_data.get(TO_FIELD_VAR)
+        if to_field and not model_admin.to_field_allowed(request, to_field):
+            raise DisallowedModelAdminToField("The field %s cannot be referenced." % to_field)
+
+        return self.cleaned_data
+
+
+
 class ChangeList:
     def __init__(self, request, model, list_display, list_display_links,
                  list_filter, date_hierarchy, search_fields, list_select_related,
@@ -46,7 +73,6 @@ class ChangeList:
         self.list_display = list_display
         self.list_display_links = list_display_links
         self.list_filter = list_filter
-        self.has_filters = None
         self.date_hierarchy = date_hierarchy
         self.search_fields = search_fields
         self.list_select_related = list_select_related
@@ -57,16 +83,18 @@ class ChangeList:
         self.sortable_by = sortable_by
 
         # Get search parameters from the query string.
-        try:
-            self.page_num = int(request.GET.get(PAGE_VAR, 0))
-        except ValueError:
-            self.page_num = 0
+        change_list_form = ChangeListForm(request.GET)
+        if not change_list_form.is_valid():
+            raise forms.ValidationError(change_list_form.errors)
+
+        change_list_form_cleaned = change_list_form.clean()
+        self.page_num = change_list_form_cleaned.get(PAGE_VAR)
+        self.query = change_list_form_cleaned.get(SEARCH_VAR)
+        self.to_field = change_list_form_cleaned.get(TO_FIELD_VAR)
+
         self.show_all = ALL_VAR in request.GET
         self.is_popup = IS_POPUP_VAR in request.GET
-        to_field = request.GET.get(TO_FIELD_VAR)
-        if to_field and not model_admin.to_field_allowed(request, to_field):
-            raise DisallowedModelAdminToField("The field %s cannot be referenced." % to_field)
-        self.to_field = to_field
+
         self.params = dict(request.GET.items())
         if PAGE_VAR in self.params:
             del self.params[PAGE_VAR]
@@ -77,7 +105,6 @@ class ChangeList:
             self.list_editable = ()
         else:
             self.list_editable = list_editable
-        self.query = request.GET.get(SEARCH_VAR, '')
         self.queryset = self.get_queryset(request)
         self.get_results(request)
         if self.is_popup:
@@ -95,6 +122,7 @@ class ChangeList:

[Can Sarıgöl]

I thought like this, is it correct route?

[Simon Charette]

Hello Can, it does look more appropriate. The page field should probably be a forms.IntegerField and you shouldn't have to perform any form of manual check for '\x00' as forms.CharField already has a ProhibitNullCharactersValidator. We'll have to find a way to surface the exception appropriately because raising it at ChangeList initialization will just result in a different crash. Please submit your work as a PR so it's easier to review.

[Can Sarıgöl]

Thanks again ı will apply these within few hours

[Can Sarıgöl]

​new PR

[Carlton Gibson]

As per comment on PR, testcases are not correct: we can't fix the test by adjusting the expected exception to be the exception raised by the missing MessageMiddleware. (That's an error.)

[Carlton Gibson]

Unrelated changes need moving to separate cleanup ticket/PR, but looking good after that.

[Carlton Gibson]

Summary of review on PR: There's a small behaviour change that it would be nice to avoid, and a couple of other small points, but it's not far off.

[GitHub <noreply@…>]

In 5b4c6b58:

Fixed #30064 -- Added form to validate admin search fields query input.

[Carlton Gibson]

The commit here 5b4c6b58a097028de970875605680df941ab0a47 has been incorrectly credited to me.

It was the work of Can Sarıgöl. My apologies Can!

(I think, with input from Mariusz, that merging via the GitHub UI after pushing edits can, in circumstances yet to be 100% clarified, result in this kind of error. Will use the CLI in these cases.)

[Can Sarıgöl]

no problem at all, the important thing was to solve the problem :)