There is no possibility to use subdir for templates in template store.

All templates have to be placed in flat template store. If you have many
templates this may be a problem. 

The attached patch allow to use subdir in template store. For example,
with this patch you can use "issue?@template=mobile/index" URL for
"mobile/issue.index.html" template.

This feature turned on for Jinja2 template. 

P.S. The patch is not so elegant as it can be. This is because of
(name,view) -> template_filename logic was moved from template engine logic:

 Template selection code is moved from Loader classes into cgi.client
 limiting the responsibility of Loaders to compilation and rendering.
 Internally, templating.find_template is replaced with
 client.selectTemplate.

I don't know the the reason of this decision.

My impression was that you could use subdirectories if their names
matched the existing scheme, but that doing so introduced a path
traversal vulnerability (see issue 2550701).  How does this patch relate
to path traversal issues?

Hello, Joseph.

As for path traversal.

I  was  started  this patch by adding check for '..' in template name.
But  then  i  found that FileSystemLoader in Jinja2 engine already has
this  check.  I  remove this check from my patch in hope that less
intrusive patch has more chance to be commited.

So,  this  patch  turn  subdir feature on only for Jinja2 engine which
will raise TempateNotFound in case of path containing '..'.

If subdirs feature will be expanded to other template engines there is
a need to add check for '..' to LoaderBase.check() function.

> Joseph Myers added the comment:

> My impression was that you could use subdirectories if their names
> matched the existing scheme, but that doing so introduced a path
> traversal vulnerability (see issue 2550701).  How does this patch relate
> to path traversal issues?

> ----------
> nosy: +joseph_myers

> ________________________________________________
> Roundup tracker <issues@roundup-tracker.org>
> <http://issues.roundup-tracker.org/issue2550891>
> ________________________________________________

On Mon, Jun 22, 2015 at 02:01:27PM +0000, Anthony wrote:
> 
> There is no possibility to use subdir for templates in template store.
> 
> All templates have to be placed in flat template store. If you have many
> templates this may be a problem. 
> 
> The attached patch allow to use subdir in template store. For example,
> with this patch you can use "issue?@template=mobile/index" URL for
> "mobile/issue.index.html" template.

Have you considered this may have security implications if someone
specifies, e.g.,

issue?@template=../../../.....

You should check with abspath that the target is below the template
directory. (I haven't checked your code yet)

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   http://www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com
allmenda.com member                     email: rsc@allmenda.com

I  was  started  this patch by adding check for '..' in template name.
But  then  i  found that FileSystemLoader in Jinja2 engine already has
this  check.  I  remove this check from my patch in hope that less
intrusive patch has more chance to be commited.

So,  this  patch  turn  subdir feature on only for Jinja2 engine which
will raise TempateNotFound in case of path containing '..'.

If subdirs feature will be expanded to other template engines there is
a need to add check for '..' to LoaderBase.check() function.
> Ralf Schlatterbeck added the comment:

> On Mon, Jun 22, 2015 at 02:01:27PM +0000, Anthony wrote:
>> 
>> There is no possibility to use subdir for templates in template store.
>> 
>> All templates have to be placed in flat template store. If you have many
>> templates this may be a problem. 
>> 
>> The attached patch allow to use subdir in template store. For example,
>> with this patch you can use "issue?@template=mobile/index" URL for
>> "mobile/issue.index.html" template.

> Have you considered this may have security implications if someone
> specifies, e.g.,

> issue?@template=../../../.....

> You should check with abspath that the target is below the template
> directory. (I haven't checked your code yet)

> Ralf

I think any patch that goes in should work for any templating engine.
So a check for directory traversal needs to happen in this patch.

I would claim that the function reformTplName should do all the
security checks. This way we are protected even if we add another
templating engine someday.

I think that is preferable to adding a check for ../ to the
tal templating code.

As  i  mention  this  patch  looks ugly. It transform path (filename) string  back to (classname,
template_value) pair and then again to path string.

This is  because of decision made sometime ago:
"Template selection code is moved from Loader classes into cgi.client
 limiting the responsibility of Loaders to compilation and rendering.
 Internally, templating.find_template is replaced with
 client.selectTemplate."

 Logic  of  converting  (classname, template_value) to file(path) name
 was   moved  from  template  engine   to  client.selectTemplate
 function.

If  this  feature  is  accepted  then  we  need  to move the logic  back from
client.selectTemplate to template engine.

I'll  modify  class  templating.LoaderBase by
- adding    a    function   for   default   conversion    (as  client.selectTemplate);
- adding path traversal check in existing templating.LoaderBase.check function.

> John Rouillard added the comment:

> I think any patch that goes in should work for any templating engine.
> So a check for directory traversal needs to happen in this patch.

> I would claim that the function reformTplName should do all the
> security checks. This way we are protected even if we add another
> templating engine someday.

> I think that is preferable to adding a check for ../ to the
> tal templating code.

> ----------
> nosy: +rouilj

> ________________________________________________
> Roundup tracker <issues@roundup-tracker.org>
> <http://issues.roundup-tracker.org/issue2550891>
> ________________________________________________

This may be too simple but if we change cgi/templating.py's

TALLoaderBase::_find with:

class TALLoaderBase(LoaderBase):
    """ Common methods for the legacy TAL loaders."""

    def __init__(self, dir):
        self.dir = dir

    def _find(self, name):
        """ Find template, return full path and filename of the
            template if it is found, None otherwise."""

+       realsrc = os.path.realpath(self.dir)
        for extension in ['', '.html', '.xml']:
            f = name + extension
            src = os.path.join(self.dir, f)
+            realpath = os.path.realpath(src)
+            if string.find(realpath, realsrc) != 0:
+                return # will raise invalid template
            if os.path.exists(src):
                return (src, f)

(Don't forget to 'import string' at the top of the file if you apply
this.) The lines I added start with +.

This does the following:

Find the real path for the template directory removing all .., /./. // 
etc. from the name and resolve any symbolic links in the path.
Do the same for the template argument.

Then require the real path of the tracker directory to be a prefix of
the requested template starting at the root. Since nothing after the
realsrc directory can traverse above the realsrc root (since realpath
removed .. and links) I think that means the template has to be
under the tracker's html directory.

There are performance implications of this, but we may be able to turn
self.dir into realpath(self.dir) in __init__ and remove some calls to
realpath for it. Alternatively we can cache realsrc in a class
variable indexed by self.dir since I think we trust self.dir is not
malicious.

I think this will handle issue2550701 for the TAL based templates.
Looking at engine_chameleon.py it looks like it uses TALLoaderBase's
_find as well so....

Some rudimentary testing after creating a directory: html/issue.item
and a template value that via .. gets me to /etc/passwd was stopped by
this with the following traceback:

  File "roundup2.hg/roundup/cgi/client.py", line 1176, in selectTemplate
    tplname, generic))
NoTemplate: No template file exists for templating "issue" with
    template "subdir/../../../../../../../../etc/passwd"
    (neither "issue.subdir/../../../../../../../../etc/passwd" nor
    "_generic.subdir/../../../../../../../../etc/passwd")

Note I was in the directory roundup.hg (which is a symbolic link to
roundup.hg2)

Joseph Myers I am considering this to patch and close out
issue2550701.

Thoughts?

I  think  we  can  just throw away a parameter with double dot inside.
This is not a thing relative to URL, URL is always a classname. This is
parameter   of   class   viewer.  There is no need to parse it by path
rules.

I thing something like

              if name.find("..") != 0:
                return # will raise invalid template

will be enough.

> John Rouillard added the comment:

> This may be too simple but if we change cgi/templating.py's

> TALLoaderBase::_find with:

> class TALLoaderBase(LoaderBase):
>     """ Common methods for the legacy TAL loaders."""

>     def __init__(self, dir):
>         self.dir = dir

>     def _find(self, name):
>         """ Find template, return full path and filename of the
>             template if it is found, None otherwise."""

> +       realsrc = os.path.realpath(self.dir)
>         for extension in ['', '.html', '.xml']:
>             f = name + extension
>             src = os.path.join(self.dir, f)
> +            realpath = os.path.realpath(src)
> +            if string.find(realpath, realsrc) != 0:
> +                return # will raise invalid template
>             if os.path.exists(src):
>                 return (src, f)

> (Don't forget to 'import string' at the top of the file if you apply
> this.) The lines I added start with +.

> This does the following:

> Find the real path for the template directory removing all .., /./. //
> etc. from the name and resolve any symbolic links in the path.
> Do the same for the template argument.

> Then require the real path of the tracker directory to be a prefix of
> the requested template starting at the root. Since nothing after the
> realsrc directory can traverse above the realsrc root (since realpath
> removed .. and links) I think that means the template has to be
> under the tracker's html directory.

> There are performance implications of this, but we may be able to turn
> self.dir into realpath(self.dir) in __init__ and remove some calls to
> realpath for it. Alternatively we can cache realsrc in a class
> variable indexed by self.dir since I think we trust self.dir is not
> malicious.

> I think this will handle issue2550701 for the TAL based templates.
> Looking at engine_chameleon.py it looks like it uses TALLoaderBase's
> _find as well so....

> Some rudimentary testing after creating a directory: html/issue.item
> and a template value that via .. gets me to /etc/passwd was stopped by
> this with the following traceback:

>   File "roundup2.hg/roundup/cgi/client.py", line 1176, in selectTemplate
>     tplname, generic))
> NoTemplate: No template file exists for templating "issue" with
>     template "subdir/../../../../../../../../etc/passwd"
>     (neither "issue.subdir/../../../../../../../../etc/passwd" nor
>     "_generic.subdir/../../../../../../../../etc/passwd")

> Note I was in the directory roundup.hg (which is a symbolic link to
> roundup.hg2)

> Joseph Myers I am considering this to patch and close out
> issue2550701.

> Thoughts?

> ________________________________________________
> Roundup tracker <issues@roundup-tracker.org>
> <http://issues.roundup-tracker.org/issue2550891>
> ________________________________________________

Hi Anthony:

In message <1666126880.20160708143147@inbox.ru>,
Anthony writes:
>I  think  we  can  just throw away a parameter with double dot inside.
>[...]  There is no need to parse it by path
>rules.
>
>I thing something like
>
>              if name.find("..") != 0:
>                return # will raise invalid template
>
>will be enough.

It looks like a %2E in the url is converted to periods by the time I
get to _find and do the check above. So a simple attempt to defeat it
looks like a check for .. should work.

My patch accidently reimplemented the code that is used for sanitizing
the url's for serving up static files (@@file paths). I discovered the
code for checking static files after I posted my code. Static file
code uses normalize not realpath, but the idea is the same.  I'll bet
when it was written realpath didn't exist. However static_file is
called *a lot less* than the templating code, and I like the
speed/simplicity of searching for '..' but not if it allows another
attack vector.

I am still concerned if something in the url could be slipped
past. High bit encoded characters that get stripped during the path
conversion so the path ends up with .. even though it's not encoded
that way in the name. Maybe some conversion function will change the
path string before it gets passed to an open function or something.

I may just be paranoid, but I remember path traversal bugs related to
encoding issues.

https://www.owasp.org/index.php/File_System#Path_traversal suggests If
forced to use user input for file operations, normalize the input
before using in file io API's, such as
http://docs.oracle.com/javase/7/docs/api/java/net/URI.html#normalize().

Ditto in https://en.wikipedia.org/wiki/Directory_traversal_attack

Anybody else want to chime in here?

(Also Anthony, can you trim your responses. I realize using a cell
phone makes this a pain but if you can trim, please do. Especially if
you are top posting and don't need to include the original post as the
tracker doesn't trim quoted material.)

Thanks for your feedback. It nicely addresses the performance issue
that I was worried about.

Applied the patch from msg5777 in changeset: d22eb1d40d0e

It uses the standard way of fixing the issue and is more resilient to
attempts to defeat it.

We now return to discussion fo the original patch for

Anthony does this patch fill your needs?

Given an @template=subdir/edit it passes :

   subdir/query.edit
   subdir/_generic.edit

to _find() or check(). The tal code then looks for a file with no
extension, .html and .xml.

This modifies roundup/cgi/client.py::selectTemplate() to look for the
last / in the template argument. Then it inserts the class name after
the / or _generic after the /.

If I have a directory html/subdir:

  html/subdir/issue.item.html
  html/subdir/query.edit.html -> ../../../query.edit.html
  html/subdir/user.item.html

http://.../issue?@template=subdir/item uses html/subdir/issue.item.html
http://.../user?@template=subdir/item uses html/subdir/user.item.html

http://.../query?@template=subdir/edit returns an error:

  NoTemplate: No template file exists for templating "query" with
   template "subdir/edit" (neither "subdir/query.edit" nor
   "subdir/_generic.edit")

because html/subdir/query.edit.html is a link to
../../../query.edit.html which falls outside of the html subdirectory.

I think this should work for your template engine as well right?

I am not sure if supporting sub-directories can have some bad
interaction/leakage with the @@file mechanism for accessing files
stored under the html subdir. But I claim there shouldn't be anything
stored there that is not publicly accessible anyway.

Здравствуйте, John.

Вы писали 15 июля 2016 г., 2:48:38:

> John Rouillard added the comment:

> Anthony does this patch fill your needs?

> Given an @template=subdir/edit it passes :

>    subdir/query.edit
>    subdir/_generic.edit

> to _find() or check(). The tal code then looks for a file with no
> extension, .html and .xml.

> This modifies roundup/cgi/client.py::selectTemplate() to look for the
> last / in the template argument. Then it inserts the class name after
> the / or _generic after the /.

> If I have a directory html/subdir:

>   html/subdir/issue.item.html
>   html/subdir/query.edit.html -> ../../../query.edit.html
>   html/subdir/user.item.html

> http://.../issue?@template=subdir/item uses html/subdir/issue.item.html
> http://.../user?@template=subdir/item uses html/subdir/user.item.html

> http://.../query?@template=subdir/edit returns an error:

>   NoTemplate: No template file exists for templating "query" with
>    template "subdir/edit" (neither "subdir/query.edit" nor
>    "subdir/_generic.edit")

> because html/subdir/query.edit.html is a link to
> ../../../query.edit.html which falls outside of the html subdirectory.

> I think this should work for your template engine as well right?

> I am not sure if supporting sub-directories can have some bad
> interaction/leakage with the @@file mechanism for accessing files
> stored under the html subdir. But I claim there shouldn't be anything
> stored there that is not publicly accessible anyway.

> ________________________________________________
> Roundup tracker <issues@roundup-tracker.org>
> <http://issues.roundup-tracker.org/issue2550891>
> ________________________________________________

> In message <1666126880.20160708143147@inbox.ru>,

> I am still concerned if something in the url could be slipped
> past. High bit encoded characters that get stripped during the path
> conversion so the path ends up with .. even though it's not encoded
> that way in the name. Maybe some conversion function will change the
> path string before it gets passed to an open function or something.

> I may just be paranoid, but I remember path traversal bugs related to
> encoding issues.

> Anybody else want to chime in here?

I  think  that  all  decoding  is  done  in the upper level and we are
working   with   character   string  representing  a path part.  If the
bad things (double  period)  is  slipped past in some encoded form it will not make
sense  because a system calls do not care about encoding. I think that
fopen("%2F%2E%2E%2F%2F%2E%2E%2Fpasswd") will fail anyway.
These are more likely my feelings than results of analyzing.

Dear, John

First of all I'd like to express a regret about my late and incomplete replies.

> Anthony does this patch fill your needs?

Yes, I think.  I use references:
 <a href="user?@template=users/index">All</a>
 <a href="user?@template=users/new">New</a>

which show

 users/user.index.html
 users/user.new.html
accordingly.

> I think this should work for your template engine as well right?

Of course the best way is to check by applying the patch in my system.
 I'll try to review the patch in the next week.

> I am not sure if supporting sub-directories can have some bad
> interaction/leakage with the @@file mechanism for accessing files
> stored under the html subdir. But I claim there shouldn't be anything
> stored there that is not publicly accessible anyway.

I  fully  agree.  Things  under  html  dir  have  to  be considered as
publicly available.

> ________________________________________________
> Roundup tracker <issues@roundup-tracker.org>
> <http://issues.roundup-tracker.org/issue2550891>
> ________________________________________________

Hi Anthony:

In msg5857 you said:
>Of course the best way is to check by applying the patch in
>my system.I'll try to review the patch in the next week.

That sounds good. If you can verify it works for you I will check in the
patch, the new tests I coded and the doc/upgradng.txt and
CHANGES.txt entries.

I don't think the jinja code path has any actual tests.
The test/test_jinja.py file looks like it has some setup/teardown and
a test that asserts that True is True. If you have some jinja tests,
please provide the patches and I will get them added.

In msg5856 you said:
> I  think  that  all  decoding  is  done  in the upper level and
> we are working   with   character   string  representing a path
> part. [...]
> These are more likely my feelings than results of analyzing.

That's my feeling as well but I don't know the effects of the path when
passed to the OS. Does it strip the 8th bit under some locale/encoding
settings? How are the paths represented/converted for windows system
calls etc.

Since this is beyond my ability to analyse, I went with the safer way:
using the code to normalize the paths and determine the conversions. At
least doing it this way I don't look incompetent for following best
practices if it does not provide the protection we need.

I'll look forward to your report when trying the patch.

-- rouilj

> That sounds good. If you can verify it works for you I will check in the
> patch, the new tests I coded and the doc/upgradng.txt and
> CHANGES.txt entries.

Is the patch in the roundup-tracker source?

In message <1209535905.20160719112902@inbox.ru>,
Anthony writes:
>
>Anthony added the comment:
>
>> That sounds good. If you can verify it works for you I will check in the
>> patch, the new tests I coded and the doc/upgradng.txt and
>> CHANGES.txt entries.
>
>Is the patch in the roundup-tracker source?

Not yet. I don't commit work in progress.  See the patch:
allow_subdir_for_templates.diff attached to the ticket at

  http://issues.roundup-tracker.org/file1616/allow_subdir_for_templates.diff

Anthony, have you had a chance to try the patch?

It should apply cleanly against 1.5.1 (with an offset).
Also it should apply cleanly to head without an offset.

Note that you will have a path traversal issue with it on
1.5.1 since the generated paths are not normalized and checked.

If it works for you I would like to commit it this weekend
and close out this ticket.

-- rouilj

Dear, John

I'll check it today/tomorrow.  I'll give a reply before Sunday .

Dear John,

I've tried the patch and it work for me.
Also I appreciate your precise and elegant coding.

Patch
http://issues.roundup-tracker.org/file1616/allow_subdir_for_templates.diff
applied docs updates, test added.

See changeset: f608eeecf638