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.
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?
(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.
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).
Here [1] 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? [1] https://brewweb.devel.redhat.com/taskinfo?taskID=5461629
(In reply to comment #5) > Here [1] 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.
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)
(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.
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.
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.
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]).
Acknowledgements: This issue was found by Florian Weimer of the Red Hat Product Security Team.
(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?
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.
Created attachment 789545 [details] Patch
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.
Thanks, Tomas. Have we reported this upstream or gotten any feedback on it from them yet?
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.
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.
Any response from upstream yet?
(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.
Created attachment 804078 [details] patch from upstream
The CRD date for this is October 10th.
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
Created scipy tracking bugs for this issue: Affects: fedora-all [bug 1018351] Affects: epel-5 [bug 1018353]
This is also correct in SciPy 0.13.0rc1: sourceforge.net/projects/scipy/files/scipy/0.13.0rc1/
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.
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.
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.
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.