This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 916690 - (CVE-2013-4251) CVE-2013-4251 scipy: weave /tmp and current directory issues
CVE-2013-4251 scipy: weave /tmp and current directory issues
Status: CLOSED WONTFIX
Product: Security Response
Classification: Other
Component: vulnerability (Show other bugs)
unspecified
Unspecified Unspecified
medium Severity medium
: ---
: ---
Assigned To: Red Hat Product Security
Branislav Náter
impact=moderate,public=20131010,repor...
: Security
Depends On: 997579 1018351 1018353
Blocks: 916691 1007017
  Show dependency treegraph
 
Reported: 2013-02-28 11:45 EST by Florian Weimer
Modified: 2016-03-16 05:22 EDT (History)
5 users (show)

See Also:
Fixed In Version: SciPy 0.12.1, SciPy 0.13.0rc1
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-03-15 02:58:04 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
Unit tests for this CVE (2.25 KB, text/plain)
2013-08-21 09:45 EDT, Tomas Tomecek
no flags Details
Patch (8.35 KB, patch)
2013-08-23 06:17 EDT, Tomas Tomecek
no flags Details | Diff
catalog reproducer (625 bytes, application/x-shellscript)
2013-08-23 08:35 EDT, Tomas Tomecek
no flags Details
Patch 2 (18.18 KB, patch)
2013-08-27 10:50 EDT, Tomas Tomecek
no flags Details | Diff
patch from upstream (19.88 KB, patch)
2013-09-27 13:47 EDT, Vincent Danen
no flags Details | Diff

  None (edit)
Description Florian Weimer 2013-02-28 11:45:42 EST
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 07:19:45 EST
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 07:49:21 EST
(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 08:45:44 EST
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 09:57:29 EST
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
Comment 6 Florian Weimer 2013-03-05 11:40:27 EST
(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.
Comment 7 Tomas Tomecek 2013-03-05 12:14:05 EST
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 12:18:05 EST
(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 11:26:14 EDT
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 10:07:59 EDT
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 12:16:43 EDT
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 12:19:36 EDT
Acknowledgements:

This issue was found by Florian Weimer of the Red Hat Product Security Team.
Comment 14 Tomas Tomecek 2013-08-20 09:33:35 EDT
(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 09:45:54 EDT
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 16 Tomas Tomecek 2013-08-23 06:17:04 EDT
Created attachment 789545 [details]
Patch
Comment 17 Tomas Tomecek 2013-08-23 08:35:39 EDT
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-26 21:08:25 EDT
Thanks, Tomas.  Have we reported this upstream or gotten any feedback on it from them yet?
Comment 19 Tomas Tomecek 2013-08-27 05:45:05 EDT
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 10:50:53 EDT
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 14:11:44 EDT
Any response from upstream yet?
Comment 23 Tomas Tomecek 2013-09-12 01:56:44 EDT
(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 13:47:26 EDT
Created attachment 804078 [details]
patch from upstream
Comment 25 Vincent Danen 2013-10-03 17:39:40 EDT
The CRD date for this is October 10th.
Comment 27 Vincent Danen 2013-10-11 14:25:35 EDT
Created scipy tracking bugs for this issue:

Affects: fedora-all [bug 1018351]
Affects: epel-5 [bug 1018353]
Comment 28 Vincent Danen 2013-10-15 18:37:28 EDT
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 01:37:17 EDT
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 01:39:16 EDT
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 17:05:47 EDT
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 01:55:56 EST
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.

Note You need to log in before you can comment on or make changes to this bug.