Created python-jinja2 tracking bugs for this issue:

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

Created python26-jinja2 tracking bugs for this issue:

Affects: epel-5 [bug 1051426]

CVE Request:
http://seclists.org/oss-sec/2014/q1/71

The fix for this issue:

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

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

(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).

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.

(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.

(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.

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.

(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.

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.

(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.

(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.

(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.

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!

Whoops, I accidentally assigned this to myself. Sorry, resetting.

(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.

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.

Ok, thank you for the clarification!

(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!

(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.

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.

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.

Slavek, are you working on submitting pull request upstream?

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

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.

Still no real activity upstream.

(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.

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?

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

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

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?

Right, they are backports of upstream fix, feel free to use.

There's no tracking bug for python-jinja2-26, I've used bug 1051425 for that.

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.

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.

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.

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.

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.