Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30064 closed Bug (fixed)

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

Reported by: kenichi-cc Owned by: Can Sarıgöl
Component: contrib.admin Version: 2.1
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

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

  1. 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.

Change History (27)

comment:1 by Tim Graham, 5 years ago

Summary: Inputting search-query with null character to browser URL field and crashAdmin search with a null character crashes with "A string literal cannot contain NUL (0x00) characters." on PostgreSQL
Triage Stage: UnreviewedAccepted

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.

Last edited 5 years ago by Tim Graham (previous) (diff)

comment:2 by Can Sarıgöl, 5 years ago

Has patch: set
Last edited 5 years ago by Tim Graham (previous) (diff)

comment:3 by Simon Charette, 5 years ago

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.

Last edited 5 years ago by Simon Charette (previous) (diff)

comment:4 by Tim Graham, 5 years ago

Patch needs improvement: set

Agreed.

comment:5 by Can Sarıgöl, 5 years ago

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.

comment:6 by Simon Charette, 5 years ago

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 thrown at 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.

Last edited 5 years ago by Simon Charette (previous) (diff)

comment:7 by Can Sarıgöl, 5 years ago

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

comment:8 by Can Sarıgöl, 5 years ago

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:

comment:9 by Can Sarıgöl, 5 years ago

I thought like this, is it correct route?

comment:10 by Simon Charette, 5 years ago

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.

Last edited 5 years ago by Simon Charette (previous) (diff)

comment:11 by Can Sarıgöl, 5 years ago

Thanks again ı will apply these within few hours

comment:12 by Can Sarıgöl, 5 years ago

Patch needs improvement: unset

comment:13 by Can Sarıgöl, 5 years ago

Owner: changed from nobody to Can Sarıgöl
Status: newassigned

comment:14 by Tim Graham, 5 years ago

Patch needs improvement: set

comment:15 by Can Sarıgöl, 5 years ago

Patch needs improvement: unset

comment:16 by Carlton Gibson, 5 years ago

Patch needs improvement: set

comment:18 by Can Sarıgöl, 5 years ago

Patch needs improvement: unset

comment:19 by Carlton Gibson, 5 years ago

Patch needs improvement: set

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.)

comment:20 by Can Sarıgöl, 5 years ago

Patch needs improvement: unset

comment:21 by Carlton Gibson, 5 years ago

Patch needs improvement: set

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

comment:22 by Asif Saifuddin Auvi, 5 years ago

Patch needs improvement: unset

comment:23 by Carlton Gibson, 5 years ago

Patch needs improvement: set

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.

comment:24 by Can Sarıgöl, 5 years ago

Patch needs improvement: unset

comment:25 by Carlton Gibson, 5 years ago

Triage Stage: AcceptedReady for checkin

comment:26 by GitHub <noreply@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 5b4c6b58:

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

comment:27 by Carlton Gibson, 5 years ago

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.)

comment:28 by Can Sarıgöl, 5 years ago

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

Note: See TracTickets for help on using tickets.
Back to Top