Off-by-one bug in AF_ALG

[tiran]

BPO	35050
Nosy	@vstinner, @tiran
PRs	bpo-35050: AF_ALG length check off-by-one error #10058 [3.7] bpo-35050: AF_ALG length check off-by-one error (GH-10058) #11069 [3.6] bpo-35050: AF_ALG length check off-by-one error (GH-10058) #11070

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2018-12-10.11:13:53.623>
created_at = <Date 2018-10-23.13:00:21.429>
labels = ['extension-modules', '3.8', 'type-bug', '3.7']
title = 'Off-by-one bug in AF_ALG'
updated_at = <Date 2018-12-10.11:13:53.622>
user = 'https://github.com/tiran'

bugs.python.org fields:

activity = <Date 2018-12-10.11:13:53.622>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2018-12-10.11:13:53.623>
closer = 'vstinner'
components = ['Extension Modules']
creation = <Date 2018-10-23.13:00:21.429>
creator = 'christian.heimes'
dependencies = []
files = []
hgrepos = []
issue_num = 35050
keywords = ['patch']
message_count = 8.0
messages = ['328311', '328312', '328315', '328411', '331485', '331495', '331496', '331497']
nosy_count = 3.0
nosy_names = ['vstinner', 'christian.heimes', 'resmord']
pr_nums = ['10058', '11069', '11070']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue35050'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

[tiran]

The error checking code for salg_name and salg_type have an off-by-one bug. It should check that both strings are NUL terminated strings. It's not a security bug, because the Linux kernel ensures that the last byte is a NULL byte.

[vstinner]

Christian and me created a bug report at the same time :-) My message:

I found two interesting warnings on socketmodule.c in the Coverity report:

Error: BUFFER_SIZE_WARNING (CWE-120): [#def12]
Python-3.6.5/Modules/socketmodule.c:2069: buffer_size_warning: Calling strncpy with a maximum size argument of 14 bytes on destination array "sa->salg_type" of size 14 bytes might leave the destination string unterminated.
\bpo-2067| return 0;
\bpo-2068| }
\bpo-2069|-> strncpy((char *)sa->salg_type, type, sizeof(sa->salg_type));
\bpo-2070| if (strlen(name) > sizeof(sa->salg_name)) {
\bpo-2071| PyErr_SetString(PyExc_ValueError, "AF_ALG name too long.");

Error: BUFFER_SIZE_WARNING (CWE-120): [#def13]
Python-3.6.5/Modules/socketmodule.c:2074: buffer_size_warning: Calling strncpy with a maximum size argument of 64 bytes on destination array "sa->salg_name" of size 64 bytes might leave the destination string unterminated.
\bpo-2072| return 0;
\bpo-2073| }
\bpo-2074|-> strncpy((char *)sa->salg_name, name, sizeof(sa->salg_name));
\bpo-2075|
\bpo-2076| *len_ret = sizeof(*sa);

It seems like the Linux kernel always write a terminating NUL byte for AF_ALG:
https://elixir.bootlin.com/linux/latest/source/crypto/af_alg.c#L171

The Python code does not create buffer overflow, it's just that the Linux kernel will always reject names which are too long. Python should reject them as well.

[tiran]

The Python code does not create buffer overflow, it's just that the Linux kernel will always reject names which are too long.

The Kernel doesn't have a direct length restriction. It just ensures that type and name are NULL terminated. Other code inside the Kernel rejects unknown type and name values.

[resmord]

The error checking code for salg_name and salg_type have an off-by-one bug. Must check that both strings are NUL terminated strings.

[vstinner]

New changeset 2eb6ad8 by Victor Stinner (Christian Heimes) in branch 'master':
bpo-35050: AF_ALG length check off-by-one error (GH-10058)
2eb6ad8

[vstinner]

New changeset bad41ce by Victor Stinner in branch '3.6':
bpo-35050: AF_ALG length check off-by-one error (GH-10058) (GH-11070)
bad41ce

[vstinner]

New changeset 1a7b62d by Victor Stinner in branch '3.7':
bpo-35050: AF_ALG length check off-by-one error (GH-10058) (GH-11069)
1a7b62d

[vstinner]

Thanks for the fix Christian!

Note: Python 2 is not affected, it doesn't support AF_ALG.