#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
- Input following URL to browser URL field and access.
http://localhost/admin/auth/user/?q=%00
- 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 , 6 years ago
Summary: | Inputting search-query with null character to browser URL field and crash → Admin search with a null character crashes with "A string literal cannot contain NUL (0x00) characters." on PostgreSQL |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:3 by , 6 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.
comment:5 by , 6 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 , 6 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.
comment:7 by , 6 years ago
ok Thanks for the detailed description, I will fix it as per your suggestion.
comment:8 by , 6 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:10 by , 6 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.
comment:13 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:14 by , 6 years ago
Patch needs improvement: | set |
---|
comment:15 by , 6 years ago
Patch needs improvement: | unset |
---|
comment:16 by , 6 years ago
Patch needs improvement: | set |
---|
comment:18 by , 6 years ago
Patch needs improvement: | unset |
---|
comment:19 by , 6 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 , 6 years ago
Patch needs improvement: | unset |
---|
comment:21 by , 6 years ago
Patch needs improvement: | set |
---|
Unrelated changes need moving to separate cleanup ticket/PR, but looking good after that.
comment:22 by , 6 years ago
Patch needs improvement: | unset |
---|
comment:23 by , 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 , 5 years ago
Patch needs improvement: | unset |
---|
comment:25 by , 5 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:27 by , 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.)
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.