classification
Title: Copy to fixed size buffer w/o check in sys_update_path
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, eric.snow, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2012-09-10 16:09 by christian.heimes, last changed 2013-07-22 10:57 by christian.heimes. This issue is now closed.

Files
File name Uploaded Description Edit
path_wcsncpy.patch christian.heimes, 2013-07-20 23:12
Messages (8)
msg170200 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-09-10 16:09
In Python/sysmodule.c the function sys_update_path() uses wcscpy to copy data to a fixed size buffer. The input comes from an external source (argv[0]) and could theoretically be larger than the buffer.

Suggested solution:
Increase the buffer a bit:

    wchar_t argv0copy[sizeof(wchar_t)* (MAXPATHLEN+1)];

and use wcsncpy:

    wcsncpy(argv0copy, argv0, MAXPATHLEN);
    argv0copy[MAXPATHLEN] = L'\0';


CID 486850
msg170201 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-09-10 16:15
search_for_prefix() and search_for_exec_prefix() contain similar code.

CID 486612
CID 486611
msg193423 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-07-20 23:12
Here is a patch for 3.3 and tip that replaces wcscpy() with wcsncpy() and adds a proper NUL terminator at MAXPATHLEN.
msg193443 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-07-21 13:06
> -                wcscpy(q+1, link);
> +                wcsncpy(q+1, link, MAXPATHLEN);
> +                argv0copy[2*MAXPATHLEN] = L'\0';

Should be `q[MAXPATHLEN] = L'\0';`. Otherwise there will be a bug when a length of link is MAXPATHLEN.
msg193519 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-07-22 08:49
Good point, but I think it should be `q[MAXPATHLEN + 1] = L'\0';`.
msg193522 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-07-22 10:04
Agree.
msg193526 - (view) Author: Roundup Robot (python-dev) Date: 2013-07-22 10:54
New changeset dca92e8a011a by Christian Heimes in branch '3.3':
Issue #15905: Fix theoretical buffer overflow in handling of sys.argv[0],
http://hg.python.org/cpython/rev/dca92e8a011a

New changeset 01597384531f by Christian Heimes in branch 'default':
Issue #15905: Fix theoretical buffer overflow in handling of sys.argv[0],
http://hg.python.org/cpython/rev/01597384531f
msg193527 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-07-22 10:57
Thanks!
History
Date User Action Args
2013-07-22 10:57:54christian.heimessetstatus: open -> closed
resolution: fixed
messages: + msg193527

stage: patch review -> resolved
2013-07-22 10:54:31python-devsetnosy: + python-dev
messages: + msg193526
2013-07-22 10:04:06serhiy.storchakasetmessages: + msg193522
2013-07-22 08:49:14christian.heimessetmessages: + msg193519
2013-07-21 13:06:05serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg193443
2013-07-20 23:12:42christian.heimessetfiles: + path_wcsncpy.patch
versions: - Python 3.2
messages: + msg193423

keywords: + patch
stage: needs patch -> patch review
2012-11-13 06:06:42eric.snowsetnosy: + eric.snow
2012-09-10 16:15:50christian.heimessetmessages: + msg170201
2012-09-10 16:09:46christian.heimescreate