classification
Title: Off-by-one bug in AF_ALG
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, resmord, vstinner
Priority: normal Keywords: patch

Created on 2018-10-23 13:00 by christian.heimes, last changed 2018-12-10 11:13 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 10058 merged christian.heimes, 2018-10-23 13:21
PR 11069 merged vstinner, 2018-12-10 10:26
PR 11070 merged vstinner, 2018-12-10 10:26
Messages (8)
msg328311 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-10-23 13:00
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.
msg328312 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-23 13:02
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.
# 2067|               return 0;
# 2068|           }
# 2069|->         strncpy((char *)sa->salg_type, type, sizeof(sa->salg_type));
# 2070|           if (strlen(name) > sizeof(sa->salg_name)) {
# 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.
# 2072|               return 0;
# 2073|           }
# 2074|->         strncpy((char *)sa->salg_name, name, sizeof(sa->salg_name));
# 2075|   
# 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.
msg328315 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-10-23 13:27
> 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.
msg328411 - (view) Author: Michael Airey (resmord) Date: 2018-10-25 05:14
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.
msg331485 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-12-10 10:22
New changeset 2eb6ad8578fa9d764c21a92acd8e054e3202ad19 by Victor Stinner (Christian Heimes) in branch 'master':
bpo-35050: AF_ALG length check off-by-one error (GH-10058)
https://github.com/python/cpython/commit/2eb6ad8578fa9d764c21a92acd8e054e3202ad19
msg331495 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-12-10 11:12
New changeset bad41cefef6625807198a813d9dec2c08d59dc60 by Victor Stinner in branch '3.6':
bpo-35050: AF_ALG length check off-by-one error (GH-10058) (GH-11070)
https://github.com/python/cpython/commit/bad41cefef6625807198a813d9dec2c08d59dc60
msg331496 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-12-10 11:13
New changeset 1a7b62d5571b3742e706d247dfe6509f68f1409d by Victor Stinner in branch '3.7':
bpo-35050: AF_ALG length check off-by-one error (GH-10058) (GH-11069)
https://github.com/python/cpython/commit/1a7b62d5571b3742e706d247dfe6509f68f1409d
msg331497 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-12-10 11:13
Thanks for the fix Christian!

Note: Python 2 is not affected, it doesn't support AF_ALG.
History
Date User Action Args
2018-12-10 11:13:53vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg331497

stage: patch review -> resolved
2018-12-10 11:13:04vstinnersetmessages: + msg331496
2018-12-10 11:12:55vstinnersetmessages: + msg331495
2018-12-10 10:26:51vstinnersetpull_requests: + pull_request10304
2018-12-10 10:26:39vstinnersetpull_requests: + pull_request10303
2018-12-10 10:22:39vstinnersetmessages: + msg331485
2018-10-25 05:14:51resmordsetnosy: + resmord
messages: + msg328411
2018-10-23 13:27:46christian.heimessetmessages: + msg328315
2018-10-23 13:21:30christian.heimessetkeywords: + patch
stage: patch review
pull_requests: + pull_request9395
2018-10-23 13:02:39vstinnersetnosy: + vstinner
messages: + msg328312
2018-10-23 13:00:21christian.heimescreate