|Summary:||CVE-2013-4251 scipy: weave /tmp and current directory issues|
|Product:||[Other] Security Response||Reporter:||Florian Weimer <fweimer>|
|Component:||vulnerability||Assignee:||Red Hat Product Security <security-response-team>|
|Status:||CLOSED WONTFIX||QA Contact:||Branislav Náter <bnater>|
|Version:||unspecified||CC:||jkejda, jkurik, jrusnack, security-response-team, ttomecek|
|Fixed In Version:||SciPy 0.12.1, SciPy 0.13.0rc1||Doc Type:||Bug Fix|
|Doc Text:||Story Points:||---|
|Last Closed:||2016-03-15 06:58:04 UTC||Type:||Bug|
|oVirt Team:||---||RHEL 7.3 requirements from Atomic Host:|
|Bug Depends On:||997579, 1018351, 1018353|
|Bug Blocks:||916691, 1007017|
Description Florian Weimer 2013-02-28 16:45:42 UTC
I think it would be a good idea to remove the fallback to the current directory in the weave code (which happens if ~/.python26_compiled is not a writable directory). This looks unsafe when the current directory is not trusted.
Comment 2 Tomas Tomecek 2013-03-01 12:19:45 UTC
I'm sorry, but I'm a bit puzzled by this one. You are talking about build_dir, right? Current routine sets it to current dir: build_dir = configure_build_dir(module_dir) which is a bug IMO, because argument 'build_dir' is completely ignored -- I'm gonna report it to upstream. But back to the routine that is trying to set build_dir if it's not specified: 1. if build_dir is not writable, it is set to current dir 2. if current dir is not writable either, '/tmp' is used then So you don't like 1., right?
Comment 3 Florian Weimer 2013-03-01 12:49:21 UTC
(In reply to comment #2) > I'm sorry, but I'm a bit puzzled by this one. You are talking about > build_dir, right? Current routine sets it to current dir: > > build_dir = configure_build_dir(module_dir) > > which is a bug IMO, because argument 'build_dir' is completely ignored -- > I'm gonna report it to upstream. But back to the routine that is trying to > set build_dir if it's not specified: > > 1. if build_dir is not writable, it is set to current dir > 2. if current dir is not writable either, '/tmp' is used then > > So you don't like 1., right? I don't like 1. 2 doesn't look great, either, unless a private subdirectory of /tmp is created exactly for this purposes. I haven't tested the second scenario.
Comment 4 Tomas Tomecek 2013-03-01 13:45:44 UTC
Okay, I'll fix them both. It is really easy. Brew builds will be done by Monday (March 4th) for sure (if you want to test it).
Comment 5 Tomas Tomecek 2013-03-04 14:57:29 UTC
Here  is a build of scipy with these changes: - Use argument build_dir if it is specified, fallback to module path - Do not fall for current dir if build_dir is not specified - Create temp dir (mkdtemp) when building extension if build_dir is not specified (last fallback) Tests that test this fall now (I haven't updated them yet). Is this behaviour okay or should I change it somehow?  https://brewweb.devel.redhat.com/taskinfo?taskID=5461629
Comment 6 Florian Weimer 2013-03-05 16:40:27 UTC
(In reply to comment #5) > Here  is a build of scipy with these changes: > - Use argument build_dir if it is specified, fallback to module path > - Do not fall for current dir if build_dir is not specified > - Create temp dir (mkdtemp) when building extension if build_dir is not > specified (last fallback) This is good, but I think you have to clean up the directory created with mkdtemp, otherwise it will stick around.
Comment 7 Tomas Tomecek 2013-03-05 17:14:05 UTC
But what will be the purpose of the execution if the actual result will be deleted? (I think in that case there should be a way to inform the user where is his extension stored)
Comment 8 Florian Weimer 2013-03-05 17:18:05 UTC
(In reply to comment #7) > But what will be the purpose of the execution if the actual result will be > deleted? (I think in that case there should be a way to inform the user > where is his extension stored) If you use os.mkdtemp(), you won't be able to find the directory again, so the data cached there will not be reused. Perhaps it's best to just error out in this case, simply requiring a writable home directory? That part is hard to tell for me.
Comment 9 Tomas Tomecek 2013-03-18 15:26:14 UTC
I found out that it could be traceable, because if 'build_dir' is not on sys.path, build_extension adds it there. But it's not very nice, since /tmp is tmpfs, thus not permanent storage. Also, answer to comment ( https://bugzilla.redhat.com/show_bug.cgi?id=916695#c8 ) from #916695 would help me in here.
Comment 10 Florian Weimer 2013-08-14 14:07:59 UTC
Tomas, I answered your question on bug 916695. During testing, I noticed this: [fweimer@localhost ~]$ ls -lhd /tmp/fweimer drwxr-xr-x. 2 nobody nobody 4.0K Aug 14 09:45 /tmp/fweimer [fweimer@localhost ~]$ python Python 2.7.5 (default, Aug 5 2013, 02:37:08) [GCC 4.8.1 20130717 (Red Hat 4.8.1-5)] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> import random, scipy.weave; a=1; scipy.weave.inline('printf("%%d\\n",a,%f);' % random.random(),['a']) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/lib64/python2.7/site-packages/scipy/weave/inline_tools.py", line 357, in inline **kw) File "/usr/lib64/python2.7/site-packages/scipy/weave/inline_tools.py", line 484, in compile_function verbose=verbose, **kw) File "/usr/lib64/python2.7/site-packages/scipy/weave/ext_tools.py", line 365, in compile temp = catalog.intermediate_dir() File "/usr/lib64/python2.7/site-packages/scipy/weave/catalog.py", line 220, in intermediate_dir os.makedirs(path, mode=0o700) File "/usr/lib64/python2.7/os.py", line 157, in makedirs mkdir(name, mode) OSError: [Errno 13] Permission denied: '/tmp/fweimer/python27_intermediate' >>> So this is at least a denial of service. With laxer permissions, weave uses that directory: [fweimer@localhost ~]$ ls -lhd /tmp/fweimer drwxrwxrwx. 2 nobody nobody 4.0K Aug 14 09:45 /tmp/fweimer [fweimer@localhost ~]$ python Python 2.7.5 (default, Aug 5 2013, 02:37:08) [GCC 4.8.1 20130717 (Red Hat 4.8.1-5)] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> import random, scipy.weave; a=1; scipy.weave.inline('printf("%%d\\n",a,%f);' % random.random(),['a']) creating /tmp/fweimer/python27_intermediate/compiler_ddba75e443dd3a9a304e5ced64620fbc In file included from /usr/lib64/python2.7/site-packages/numpy/core/include/numpy/ndarraytypes.h:1728:0, from /usr/lib64/python2.7/site-packages/numpy/core/include/numpy/ndarrayobject.h:17, from /usr/lib64/python2.7/site-packages/numpy/core/include/numpy/arrayobject.h:15, from /home/fweimer/.python27_compiled/sc_a35dbf436a57cecf7066976e9761bd000.cpp:22: /usr/lib64/python2.7/site-packages/numpy/core/include/numpy/npy_deprecated_api.h:11:2: warning: #warning "Using deprecated NumPy API, disable it by #defining NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION" [-Wcpp] #warning "Using deprecated NumPy API, disable it by #defining NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION" ^ /home/fweimer/.python27_compiled/sc_a35dbf436a57cecf7066976e9761bd000.cpp: In function ‘PyObject* compiled_func(PyObject*, PyObject*)’: /home/fweimer/.python27_compiled/sc_a35dbf436a57cecf7066976e9761bd000.cpp:673:29: warning: too many arguments for format [-Wformat-extra-args] printf("%d\n",a,0.946759); /*I would like to fill in changed locals and globals here...*/ ^ 1 >>> [fweimer@localhost ~]$ find /tmp/fweimer -type f /tmp/fweimer/python27_intermediate/compiler_ddba75e443dd3a9a304e5ced64620fbc/home/fweimer/.python27_compiled/sc_777337fffcbda13be8e1c5a8c5ce3aef0.o /tmp/fweimer/python27_intermediate/compiler_ddba75e443dd3a9a304e5ced64620fbc/home/fweimer/.python27_compiled/sc_a35dbf436a57cecf7066976e9761bd000.o /tmp/fweimer/python27_intermediate/compiler_ddba75e443dd3a9a304e5ced64620fbc/usr/lib64/python2.7/site-packages/scipy/weave/scxx/weave_imp.o The files are actually used as linker input files, so this allows local users to trick each other into executing code. This is much more severe, not merely hardening, and probably warrants CVE assignment. This should be fixed in cooperation with upstream, perhaps with a (short) embargo period. Please do not commit a fix to a public repository or file a public bug report.
Comment 11 Vincent Danen 2013-08-15 16:16:43 UTC
To summarize, scipy.weave will use /tmp/[username] as persistent storage (cache), but it does not check whether or not this directory already exists, does not check whether it is a directory or a symlink, and also does not verify permissions or ownership, which could allow someone to place code in this directory that would be executed as the user running scipy.weave. Ideally, it should be creating the directory if it doesn't exist with appropriate ownership/permissions, and if the directory already exists (and permissions/ownership is inappropriate), create a new directory and use it instead (in a similar way that Pulseaudio does, perhaps /var/tmp/scipy-[variable]).
Comment 12 Vincent Danen 2013-08-15 16:19:36 UTC
Acknowledgements: This issue was found by Florian Weimer of the Red Hat Product Security Team.
Comment 14 Tomas Tomecek 2013-08-20 13:33:35 UTC
(In reply to Vincent Danen from comment #11) > To summarize, scipy.weave will use /tmp/[username] as persistent storage > (cache), but it does not check whether or not this directory already exists I would like to correct this a little bit. scipy does check whether the dir exists. This is the function that is responsible for creating the directory: def intermediate_dir(): """ Location in temp dir for storing .cpp and .o files during builds. """ python_name = "python%d%d_intermediate" % tuple(sys.version_info[:2]) path = os.path.join(tempfile.gettempdir(),"%s" % whoami(),python_name) if not os.path.exists(path): os.makedirs(path, mode=0o700) return path As you can see, it checks if the directory already exists. Currently I'm writing tests for this CVE. When I finish I'll attach them here so it will be pretty easy to fix this. Is this okay?
Comment 15 Tomas Tomecek 2013-08-21 13:45:54 UTC
Created attachment 788882 [details] Unit tests for this CVE I've mocked unit tests for this CVE. These are definitely not final. Especially a part, where the tests play with actual temporary directory (/tmp/<name>/python27...) and not with one created only for purpose of the tests, is really bad. They were created to fit current code only. Also I wanted to know if the tests check correct values. Once there will be code which fixes this bug, the tests will be easy to adapt.
Comment 17 Tomas Tomecek 2013-08-23 12:35:39 UTC
Created attachment 789580 [details] catalog reproducer While writing the patch I noticed that same thing occurs for persistent cache/store of compiled functions -- catalog. We agreed with Florian that it should be fixed too, so I'm going to "respin" the patch.
Comment 18 Vincent Danen 2013-08-27 01:08:25 UTC
Thanks, Tomas. Have we reported this upstream or gotten any feedback on it from them yet?
Comment 19 Tomas Tomecek 2013-08-27 09:45:05 UTC
As I wrote in the e-mail to your mailing list, I haven't got response yet. This may be because there was an event taking place recently -- EuroSciPy. So I'm assuming they were pretty busy and that's why they didn't respond.
Comment 20 Tomas Tomecek 2013-08-27 14:50:53 UTC
Created attachment 791020 [details] Patch 2 This patch fixes original issue and similar problem with function 'default_dir'. I will post a build of scipy here tomorrow.
Comment 22 Vincent Danen 2013-09-11 18:11:44 UTC
Any response from upstream yet?
Comment 23 Tomas Tomecek 2013-09-12 05:56:44 UTC
(In reply to Vincent Danen from comment #22) > Any response from upstream yet? Unfortunately no. I'm assuming they have problems with verification if the patch works. Especially on Windows environment. I may write them this week if no response come til Friday.
Comment 24 Vincent Danen 2013-09-27 17:47:26 UTC
Created attachment 804078 [details] patch from upstream
Comment 25 Vincent Danen 2013-10-03 21:39:40 UTC
The CRD date for this is October 10th.
Comment 26 Vincent Danen 2013-10-11 18:17:28 UTC
SciPy 0.12.1 has been released to fix this issue: http://sourceforge.net/projects/scipy/files/scipy/0.12.1/ https://github.com/scipy/scipy/commit/bd296e0336420b840fcd2faabb97084fd252a973
Comment 27 Vincent Danen 2013-10-11 18:25:35 UTC
Created scipy tracking bugs for this issue: Affects: fedora-all [bug 1018351] Affects: epel-5 [bug 1018353]
Comment 28 Vincent Danen 2013-10-15 22:37:28 UTC
This is also correct in SciPy 0.13.0rc1: sourceforge.net/projects/scipy/files/scipy/0.13.0rc1/
Comment 29 Fedora Update System 2013-10-27 05:37:17 UTC
scipy-0.12.1-1.fc18 has been pushed to the Fedora 18 stable repository. If problems still persist, please make note of it in this bug report.
Comment 30 Fedora Update System 2013-10-27 05:39:16 UTC
scipy-0.12.1-1.fc19 has been pushed to the Fedora 19 stable repository. If problems still persist, please make note of it in this bug report.
Comment 31 Fedora Update System 2013-11-01 21:05:47 UTC
scipy-0.6.0-7.el5 has been pushed to the Fedora EPEL 5 stable repository. If problems still persist, please make note of it in this bug report.
Comment 32 Fedora Update System 2013-11-10 06:55:56 UTC
scipy-0.12.1-1.fc20 has been pushed to the Fedora 20 stable repository. If problems still persist, please make note of it in this bug report.