Bug 1051421 (CVE-2014-1402) - CVE-2014-1402 python-jinja2: FileSystemBytecodeCache insecure cache temporary file use
Summary: CVE-2014-1402 python-jinja2: FileSystemBytecodeCache insecure cache temporary...
Status: CLOSED ERRATA
Alias: CVE-2014-1402
Product: Security Response
Classification: Other
Component: vulnerability
Version: unspecified
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Red Hat Product Security
QA Contact:
URL:
Whiteboard: impact=moderate,public=20140109,repor...
Keywords: Reopened, Security
Depends On: 1051424 1051425 1051426 1051427 1102889 1102890 1102891 1102892 1102893 1102894
Blocks: 1051429
TreeView+ depends on / blocked
 
Reported: 2014-01-10 09:36 UTC by Ratul Gupta
Modified: 2016-04-26 13:43 UTC (History)
30 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2015-01-20 18:31:18 UTC


Attachments (Terms of Use)
Fix for CVE-2014-0012 (880 bytes, patch)
2014-01-27 16:05 UTC, Bohuslav "Slavek" Kabrda
no flags Details | Diff
Fix for CVE-2014-0012 that uses lstat instead of stat (882 bytes, patch)
2014-01-28 10:39 UTC, Bohuslav "Slavek" Kabrda
no flags Details | Diff


External Trackers
Tracker ID Priority Status Summary Last Updated
Red Hat Product Errata RHSA-2014:0747 normal SHIPPED_LIVE Moderate: python-jinja2 security update 2014-06-11 21:13:32 UTC
Red Hat Product Errata RHSA-2014:0748 normal SHIPPED_LIVE Moderate: python33-python-jinja2 and python27-python-jinja2 security update 2014-06-11 21:13:22 UTC

Description Ratul Gupta 2014-01-10 09:36:01 UTC
Jinja2, a template engine written in pure python, was found to use /tmp as a default directory for jinja2.bccache.FileSystemBytecodeCache, which is insecure because the /tmp directory is world-writable and the filenames used by FileSystemBytecodeCache are predictable. A malicious local user could exploit this bug to alter output generated by other user's application using Jinja2 or possibly, execute arbitrary code as another user.

References:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=734747

Comment 2 Ratul Gupta 2014-01-10 09:39:24 UTC
Created python-jinja2 tracking bugs for this issue:

Affects: fedora-all [bug 1051424]
Affects: epel-all [bug 1051425]

Comment 3 Ratul Gupta 2014-01-10 09:39:29 UTC
Created python26-jinja2 tracking bugs for this issue:

Affects: epel-5 [bug 1051426]

Comment 5 Ratul Gupta 2014-01-10 11:42:49 UTC
CVE Request:
http://seclists.org/oss-sec/2014/q1/71

Comment 8 Vincent Danen 2014-01-11 20:38:50 UTC
The fix for this issue:

https://github.com/mitsuhiko/jinja2/commit/acb672b6a179567632e032f547582f30fa2f4aa7

Comment 9 Vincent Danen 2014-01-11 20:55:21 UTC
Note that Arun Babu Neelicattu reported [1] that the fix above introduced a new flaw (temporary file handling) which was assigned the CVE name CVE-2014-0012.

I've also commented on the upstream commit to make the problem very clear.  So the above patch cannot be used as-is; it needs to be corrected for the temporary file flaw.

As it stands now, we are not vulnerable to CVE-2014-0012 as nothing has used this flawed patch.


[1] http://seclists.org/oss-sec/2014/q1/73

Comment 10 Thomas Moschny 2014-01-12 01:45:59 UTC
(In reply to Vincent Danen from comment #9)
> As it stands now, we are not vulnerable to CVE-2014-0012 as nothing has used
> this flawed patch.

Well, I've build 2.7.2 (which includes a variant of that patch) for Rawhide, which is thus currently affected by the (new) CVE-2014-0012.

Version 2.7.2 has also been built for F20, but given that it is flawed, I will not create an update. Instead, I'll wait for an updated version of the patch, or a new version.

The final patch can then be ported to the older versions for F19 (2.6), F18 (2.8) and EL5 (2.2.1 and 2.5.5).

Comment 11 Vincent Danen 2014-01-13 15:43:37 UTC
I'm not too concerned about Rawhide.  That's a development version.  You're clearly aware of it, so I trust you'll ensure it's fixed there as well.

Provided 2.7.2 for F20 hasn't been pushed to stable, I think we can safely say this doesn't affect F20.

Waiting for a proper patch would be ideal, yes, although it probably wouldn't be too hard to just create one ourselves if upstream hasn't done so yet.

The only thing I can't tell by quickly looking is whether this cache directory needs to persist across invocations.  For instance, if it's supposed to survive from one run to the next then mkstemp is probably not the most appropriate way to go unless the location can be written to a config file for subsequent lookups.

I'm not familiar with this code and don't have time to look at it closer right now, but I do see that upstream hasn't yet done anything in this regard yet.  Given the impact of the issue, I think it would be ok to take a "wait and see" approach, provided upstream doesn't take too long.

Comment 12 Thomas Moschny 2014-01-14 12:43:34 UTC
(In reply to Vincent Danen from comment #11)
> The only thing I can't tell by quickly looking is whether this cache
> directory needs to persist across invocations.  For instance, if it's
> supposed to survive from one run to the next then mkstemp is probably not
> the most appropriate way to go unless the location can be written to a
> config file for subsequent lookups.

Yes, the cache location needs to be persistent across invocations. Bugging upstream.

Comment 13 Bohuslav "Slavek" Kabrda 2014-01-15 08:11:07 UTC
(In reply to Thomas Moschny from comment #12)
> (In reply to Vincent Danen from comment #11)
> > The only thing I can't tell by quickly looking is whether this cache
> > directory needs to persist across invocations.  For instance, if it's
> > supposed to survive from one run to the next then mkstemp is probably not
> > the most appropriate way to go unless the location can be written to a
> > config file for subsequent lookups.
> 
> Yes, the cache location needs to be persistent across invocations. Bugging
> upstream.

But that means that the directory name has to predictable in some way, doesn't it? So the real question is, how can you make a temporary file that's both safe and its location is computable across interpreter invocations.

Comment 14 Vincent Danen 2014-01-17 18:14:15 UTC
There's a few things, and it's tough to do this correctly.  The problem, as I gather it, is you can have more than one user run this which is why they were using directories based on the uid.

What would be ideal here, I think, is to have /var/run/jinja2/ created by the package and that would be the root directory (as opposed to /tmp).  Within this directory can be user-based sub-directorys (/var/run/jinja2/user or /var/run/jinga2/uid) and the code would look to a) see if it exists (if not, create with 0700 perms), and b) see if it is owned by the appropriate user (if not, complain really really loudly).

That's still not perfect, and there might still be a race condition there, but I don't think os.mkdir() will complete successfully if the named path exists as a directory or file, so should be ok).

So perhaps something like:

path = '/var/run/jinja2/%s' % os.getuid()
try:
    os.mkdir(path, 0700)
except Exception as e:
    raise e

if os.stat(path).st_uid is not os.getuid():
    # error and exit

if oct(os.stat(path).st_mode & 0777) is not '0700':
    # error and exit

That should work I think.

The other alternative is to again use /var/run/jinja2/ but use temp.mkstemp() to create a directory in there, but only if a config file or setting doesn't exist.  For instance, we have something like ~/.jinja2 or ~/.jinja2/config or whatever that has the setting:

path = /var/run/jina2/532/tmp123456

and then the module looks here first as the cache location (probably performing the same tests as above... "is it owned by me?" and "is it mode 0700?" before doing anything with it).

If such a config file/setting doesn't exist, create the file with temp.mkstemp() and write it to the config/setting for subsequent use.  If the noted file doesn't exist (or isn't usable due to failing any tests), create a new one with temp.mkstemp() and save the new path to the config file.

Those seem to to be the two best options to me.

Comment 15 Bohuslav "Slavek" Kabrda 2014-01-20 08:12:29 UTC
(In reply to Vincent Danen from comment #14)
> There's a few things, and it's tough to do this correctly.  The problem, as
> I gather it, is you can have more than one user run this which is why they
> were using directories based on the uid.
> 
> What would be ideal here, I think, is to have /var/run/jinja2/ created by
> the package and that would be the root directory (as opposed to /tmp). 
> Within this directory can be user-based sub-directorys (/var/run/jinja2/user
> or /var/run/jinga2/uid) and the code would look to a) see if it exists (if
> not, create with 0700 perms), and b) see if it is owned by the appropriate
> user (if not, complain really really loudly).

So how is this different from doing the same in /tmp? I don't see why you'd need to move to /var/run/jinja2.
Based on what you're saying, I guess the upstream patch is OK, but it needs the two checks you mention:

> if os.stat(path).st_uid is not os.getuid():
>     # error and exit
> 
> if oct(os.stat(path).st_mode & 0777) is not '0700':
>     # error and exit

Or is there some problem doing this in /tmp that I don't see? Thanks.

Comment 17 Vincent Danen 2014-01-24 00:09:01 UTC
Persistence.  If the idea is to have the cache persist across invocations of jinja2, then I assume it might be desirable to have it persist across reboots as well.

As many systems clean /tmp on reboot, that would remove the cache as well.  In /var/run/jinja2/ there are no such problems.

I also believe that cache files are not temporary files, necessarily, so I don't know if /tmp is the most appropriate.

Functionally it's the same thing, you're right.  The biggest thing for me is persistence.

Comment 18 Tomas Hoger 2014-01-24 20:54:13 UTC
(In reply to Vincent Danen from comment #17)
> Persistence.  If the idea is to have the cache persist across invocations of
> jinja2, then I assume it might be desirable to have it persist across
> reboots as well.
> 
> As many systems clean /tmp on reboot, that would remove the cache as well. 
> In /var/run/jinja2/ there are no such problems.

Given that it's only cache meant to provide speed improvement, persistence across reboots may not be that important.  On the other hand, /var/run/jinja2/ would imply new unexpected world-writable directory for administrators to deal with.  There's a workaround to achieve better persistence by setting TMPDIR to point to a directory that isn't wiped on reboot.  I'd leave improving persistence out of scope of this security fix.  The same persistence issues existed before.

Comment 19 Tomas Hoger 2014-01-27 15:31:37 UTC
(In reply to Vincent Danen from comment #14)
> path = '/var/run/jinja2/%s' % os.getuid()
> try:
>     os.mkdir(path, 0700)
> except Exception as e:
>     raise e

This either needs a check for existence before mkdir (as you mentioned), or handle EEXIST error differently.

> if os.stat(path).st_uid is not os.getuid():
>     # error and exit
> 
> if oct(os.stat(path).st_mode & 0777) is not '0700':
>     # error and exit

os.lstat needs to be used instead of os.stat to avoid symlink + race issues.

Hence something like:

try:
    os.mkdir(path, 0700)
except OSError as e:
    if e.errno != errno.EEXIST:
        raise
    
path_stat = os.lstat(path)
if path_stat.st_uid != os.geteuid() \
    or not stat.S_ISDIR(path_stat.st_mode) \
    or (os.stat(path).st_mode & 0777) != 0700:
    # raise some "bad permissions" exception

Possibly with all stat checks within except.

Comment 20 Tomas Hoger 2014-01-27 15:51:11 UTC
(In reply to Vincent Danen from comment #9)
> Note that Arun Babu Neelicattu reported [1] that the fix above introduced a
> new flaw (temporary file handling) which was assigned the CVE name
> CVE-2014-0012.

The original issue falls into the "insecure temporary file use" category as well.  Both in terms of allowing file overwrite, and making jinja use a cache file from attacker.

Upstream fix changes situation a little - attacker needs to be able to create /tmp/_jinja2-cache-$UID directory before victim, but if they succeed in doing so, they can unlink cache files created by victim at any time.

There is a separate bug for CVE-2014-0012 - bug 1052102 - albeit all the real discussion happens here.

Comment 21 Bohuslav "Slavek" Kabrda 2014-01-27 16:05:41 UTC
Created attachment 856133 [details]
Fix for CVE-2014-0012

(In reply to Tomas Hoger from comment #18)
> (In reply to Vincent Danen from comment #17)
> > Persistence.  If the idea is to have the cache persist across invocations of
> > jinja2, then I assume it might be desirable to have it persist across
> > reboots as well.
> > 
> > As many systems clean /tmp on reboot, that would remove the cache as well. 
> > In /var/run/jinja2/ there are no such problems.
> 
> Given that it's only cache meant to provide speed improvement, persistence
> across reboots may not be that important.  On the other hand,
> /var/run/jinja2/ would imply new unexpected world-writable directory for
> administrators to deal with.  There's a workaround to achieve better
> persistence by setting TMPDIR to point to a directory that isn't wiped on
> reboot.  I'd leave improving persistence out of scope of this security fix. 
> The same persistence issues existed before.

Agreed. I'm attaching a patch that should fix the security problem - it checks for both dir ownership and proper permissions.
Tomas, Vincent, can anyone of you review the patch and confirm it's ok before I commit it to RHEL and submit upstream? Thanks!

Comment 22 Bohuslav "Slavek" Kabrda 2014-01-27 16:09:00 UTC
Whoops, I accidentally assigned this to myself. Sorry, resetting.

Comment 23 Tomas Hoger 2014-01-27 16:42:52 UTC
(In reply to Bohuslav "Slavek" Kabrda from comment #21)
> Agreed. I'm attaching a patch that should fix the security problem - it
> checks for both dir ownership and proper permissions.
> Tomas, Vincent, can anyone of you review the patch and confirm it's ok
> before I commit it to RHEL and submit upstream? Thanks!

- Use os.lstat, not os.stat.  os.stat follows symlinks, so attacker can create link that points to some victim's directory with proper permission at the time of the check, and later replace with real directory, or symlink to an attacker's directory.

- Consider checking if path actually is directory.

(See comment 19 for the both of the above comments.)

- Nitpick: use 0700 instead of 448.  448 is less obvious, are there significant benefits over 0700 to actually use it?  (I know upstream uses 448 elsewhere, so why not fix both while at it? :)

- A concern with the $UID directories is that it possibly makes it somewhat easier to "DoS" other local user by making Jinja2 raise exception because of cache file permissions.  But the problem is not new, older Jinja2 versions could raise exception too if they found cache files they could not use.

Comment 24 Jakub Wilk 2014-01-27 18:56:50 UTC
Jinja2 (>= 2.7) supports Python 2 and Python 3 with single codebase (no 2to3 involved). So upstream can't use 0700, because that's syntax error in Python 3. They could use 0o700 (which is syntactically valid since Python 2.6) or stat.S_IRWXU.

Comment 25 Tomas Hoger 2014-01-27 19:48:39 UTC
Ok, thank you for the clarification!

Comment 27 Bohuslav "Slavek" Kabrda 2014-01-28 09:35:15 UTC
(In reply to Tomas Hoger from comment #23)
> (In reply to Bohuslav "Slavek" Kabrda from comment #21)
> > Agreed. I'm attaching a patch that should fix the security problem - it
> > checks for both dir ownership and proper permissions.
> > Tomas, Vincent, can anyone of you review the patch and confirm it's ok
> > before I commit it to RHEL and submit upstream? Thanks!
> 
> - Use os.lstat, not os.stat.  os.stat follows symlinks, so attacker can
> create link that points to some victim's directory with proper permission at
> the time of the check, and later replace with real directory, or symlink to
> an attacker's directory.

Ok, will do.

> - Consider checking if path actually is directory.

I don't understand why that'd be needed. If the path is not a directory then there are two possibilities:
- It's owned by the user that jinja2 is running under - then Python will just fail when trying to create a file in this directory (which is actually not a directory).
- It's owned by another user - the check for owner will rise exception before any attempt to create a cached file.
What do we gain by checking that the path is directory?
 
> - Nitpick: use 0700 instead of 448.  448 is less obvious, are there
> significant benefits over 0700 to actually use it?  (I know upstream uses
> 448 elsewhere, so why not fix both while at it? :)

Because I want to have a patch acceptable for upstream. If upstream is keen on using 448, then that's what I want to do to keep consistent and to have an acceptable patch (also it's better because of the reason mentioned in comment #24 by Jakub Wilk).

> - A concern with the $UID directories is that it possibly makes it somewhat
> easier to "DoS" other local user by making Jinja2 raise exception because of
> cache file permissions.  But the problem is not new, older Jinja2 versions
> could raise exception too if they found cache files they could not use.

Yes, the problem is old and I don't think that fixing it is in the scope of this bug.


So to sum up, I think that the only alteration needed for my patch is using "lstat" instead of "stat". Is that correct?
Thanks a lot!

Comment 28 Tomas Hoger 2014-01-28 10:28:00 UTC
(In reply to Bohuslav "Slavek" Kabrda from comment #27)
> > - Consider checking if path actually is directory.
> 
> I don't understand why that'd be needed. If the path is not a directory then
> there are two possibilities:
> - It's owned by the user that jinja2 is running under - then Python will
> just fail when trying to create a file in this directory (which is actually
> not a directory).
> - It's owned by another user - the check for owner will rise exception
> before any attempt to create a cached file.
> What do we gain by checking that the path is directory?

I have no specific significant concern in mind.  The check can lead to better error report.  There should be no negative impact, if re-stat is avoided.

Comment 29 Bohuslav "Slavek" Kabrda 2014-01-28 10:39:13 UTC
Created attachment 856466 [details]
Fix for CVE-2014-0012 that uses lstat instead of stat

Ok, so I hope that [attached] is the final and correct patch for the CVE. I just replaced os.stat with os.lstat, no other change since the last patch.

Comment 30 Jakub Wilk 2014-01-28 13:10:46 UTC
The "is it a directory?" check is important, because we don't want to follow symlinks. Just because such a symlink is owned by the correct user, doesn't mean it was created by this user: it could have been hardlinked by an attacker.

Now, on Linux systems we are safe, because symlinks have always 0777 permissions, so they never pass the st_mode check. But it's not neccessarily the case on other systems.

Comment 31 Tomas Hoger 2014-01-29 11:47:19 UTC
Slavek, are you working on submitting pull request upstream?

Comment 32 Tomas Hoger 2014-02-20 20:34:21 UTC
I did a pull request with a fix that adds ownership, permission and type check for the temporary directory:

https://github.com/mitsuhiko/jinja2/pull/296

Comment 33 Tomas Hoger 2014-04-02 09:08:19 UTC
There's still no upstream feedback on additional fixes required to fix this issue, and no upstream commits since early Jan.  It seems we should proceed with our own fix without upstream.

Comment 34 Tomas Hoger 2014-05-29 18:51:27 UTC
Still no real activity upstream.

Comment 38 Tomas Hoger 2014-06-06 21:55:35 UTC
(In reply to Tomas Hoger from comment #32)
> I did a pull request with a fix that adds ownership, permission and type
> check for the temporary directory:
> 
> https://github.com/mitsuhiko/jinja2/pull/296

Applied upstream:

https://github.com/mitsuhiko/jinja2/commit/964c61ce79f6748ff8c583e2eb12ec54082bf188

Fixed in version 2.7.3.

Comment 39 Thomas Moschny 2014-06-07 09:02:39 UTC
I've updated rawhide to 2.7.3, and a build for F20 is underway.

We need to port the final patch to 2.6 (for F19 and RHEL7) and to 2.2.1 (for EPEL5 and RHEL6). Is someone from RH already working on this?

Comment 41 errata-xmlrpc 2014-06-11 17:13:42 UTC
This issue has been addressed in following products:

  Red Hat Software Collections 1 for Red Hat Enterprise Linux 6
  Red Hat Software Collections 1 for Red Hat Enterprise Linux 6.3 EUS
  Red Hat Software Collections 1 for Red Hat Enterprise Linux 6.4 EUS
  Red Hat Software Collections 1 for Red Hat Enterprise Linux 7

Via RHSA-2014:0748 https://rhn.redhat.com/errata/RHSA-2014-0748.html

Comment 42 errata-xmlrpc 2014-06-11 17:14:30 UTC
This issue has been addressed in following products:

  Red Hat Enterprise Linux 6

Via RHSA-2014:0747 https://rhn.redhat.com/errata/RHSA-2014-0747.html

Comment 43 Thomas Moschny 2014-06-12 19:26:16 UTC
I guess it's ok for me to take the patches from these packages mentioned in comment 41 and comment 42 and apply them to the F19 and EPEL5 packages, respectively?

Comment 44 Tomas Hoger 2014-06-12 19:41:57 UTC
Right, they are backports of upstream fix, feel free to use.

Comment 45 Thomas Moschny 2014-06-14 01:13:20 UTC
There's no tracking bug for python-jinja2-26, I've used bug 1051425 for that.

Comment 46 Fedora Update System 2014-06-22 23:56:23 UTC
python-jinja2-2.6-7.fc19 has been pushed to the Fedora 19 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 47 Fedora Update System 2014-06-22 23:57:30 UTC
python-jinja2-2.7.3-1.fc20 has been pushed to the Fedora 20 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 49 Fedora Update System 2014-06-30 03:37:53 UTC
python26-jinja2-2.5.5-5.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 50 Fedora Update System 2014-06-30 03:38:21 UTC
python-jinja2-26-2.6-3.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 51 Fedora Update System 2014-06-30 03:38:41 UTC
python-jinja2-2.2.1-2.el5 has been pushed to the Fedora EPEL 5 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.