Patchwork [STABLE] subrepo: avoid false unsafe path detection on Windows

login
register
mail settings
Submitter Matt Harbison
Date Feb. 6, 2019, 2:04 a.m.
Message ID <0e18c6ec895542394c0a.1549418640@Envy>
Download mbox | patch
Permalink /patch/38467/
State Superseded
Headers show

Comments

Matt Harbison - Feb. 6, 2019, 2:04 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1549417854 18000
#      Tue Feb 05 20:50:54 2019 -0500
# Branch stable
# Node ID 0e18c6ec895542394c0ad18c380bf3bbd4ba4d9b
# Parent  8b2892d5a9f2c06c998c977015a9ad3e3a3c9b5f
subrepo: avoid false unsafe path detection on Windows

Subrepo paths are not normalized for the OS, so what was happening in the
subsequent root path check was:

    root                  -> $TESTTMP\issue1852a\sub/repo
    util.expandpath(...)  -> $TESTTMP\issue1852a\sub/repo
    os.path.realpath(...) -> $TESTTMP\issue1852a\sub\repo
Matt Harbison - Feb. 6, 2019, 2:08 a.m.
On Tue, 05 Feb 2019 21:04:00 -0500, Matt Harbison <mharbison72@gmail.com>  
wrote:

> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1549417854 18000
> #      Tue Feb 05 20:50:54 2019 -0500
> # Branch stable
> # Node ID 0e18c6ec895542394c0ad18c380bf3bbd4ba4d9b
> # Parent  8b2892d5a9f2c06c998c977015a9ad3e3a3c9b5f
> subrepo: avoid false unsafe path detection on Windows

The fixes a last minute breakage in 4.9.  It doesn't look like TortoiseHg  
or the MSI installer has been generated yet, but the Inno installer is  
already posted.  Maybe this needs a 4.9.1?
Yuya Nishihara - Feb. 6, 2019, 11:54 a.m.
On Tue, 05 Feb 2019 21:04:00 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1549417854 18000
> #      Tue Feb 05 20:50:54 2019 -0500
> # Branch stable
> # Node ID 0e18c6ec895542394c0ad18c380bf3bbd4ba4d9b
> # Parent  8b2892d5a9f2c06c998c977015a9ad3e3a3c9b5f
> subrepo: avoid false unsafe path detection on Windows
> 
> Subrepo paths are not normalized for the OS, so what was happening in the
> subsequent root path check was:
> 
>     root                  -> $TESTTMP\issue1852a\sub/repo
>     util.expandpath(...)  -> $TESTTMP\issue1852a\sub/repo
>     os.path.realpath(...) -> $TESTTMP\issue1852a\sub\repo

Oops, my bad.

> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
> --- a/mercurial/subrepo.py
> +++ b/mercurial/subrepo.py
> @@ -405,7 +405,7 @@ class hgsubrepo(abstractsubrepo):
>          super(hgsubrepo, self).__init__(ctx, path)
>          self._state = state
>          r = ctx.repo()
> -        root = r.wjoin(path)
> +        root = os.path.realpath(r.wjoin(path))

Can we do r.wjoin(util.localpath(path)) instead? Even though symlinks and
".."s should be checked before, I want to avoid path resolution here for
extra safety.

What I'm not certain is whether realpath() does normalize long/short names
and lower/upper case stuff. os.path.realpath() appears to call
GetFullPathName() on Windows, and I guess it wouldn't do such normalization,
but I'm not sure.

https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-getfullpathnamea
Matt Harbison - Feb. 7, 2019, 4:16 a.m.
On Wed, 06 Feb 2019 06:54:02 -0500, Yuya Nishihara <yuya@tcha.org> wrote:

> On Tue, 05 Feb 2019 21:04:00 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1549417854 18000
>> #      Tue Feb 05 20:50:54 2019 -0500
>> # Branch stable
>> # Node ID 0e18c6ec895542394c0ad18c380bf3bbd4ba4d9b
>> # Parent  8b2892d5a9f2c06c998c977015a9ad3e3a3c9b5f
>> subrepo: avoid false unsafe path detection on Windows
>>
>> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
>> --- a/mercurial/subrepo.py
>> +++ b/mercurial/subrepo.py
>> @@ -405,7 +405,7 @@ class hgsubrepo(abstractsubrepo):
>>          super(hgsubrepo, self).__init__(ctx, path)
>>          self._state = state
>>          r = ctx.repo()
>> -        root = r.wjoin(path)
>> +        root = os.path.realpath(r.wjoin(path))
>
> Can we do r.wjoin(util.localpath(path)) instead? Even though symlinks and
> ".."s should be checked before, I want to avoid path resolution here for
> extra safety.

Early indications are yes.  If the rest of the tests pass, I'll resend.   
I'm not real sure what the reason for avoiding path resolution is, so feel  
free to add commentary to the patch if you queue it.

> What I'm not certain is whether realpath() does normalize long/short  
> names
> and lower/upper case stuff. os.path.realpath() appears to call
> GetFullPathName() on Windows, and I guess it wouldn't do such  
> normalization,
> but I'm not sure.
>
> https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-getfullpathnamea

It doesn't look like it changes either case:

C:\Users\Matt>py -2 -c "import os; print os.path.realpath('C:\PROGRAM  
FILES')"
C:\PROGRAM FILES

C:\Users\Matt>py -2 -c "import os; print os.path.realpath('C:\PROGRA~1')"
C:\PROGRA~1

C:\Users\Matt>py -2 -c "import os; print  
os.path.realpath('C:\PROGRA~1\..\PROGRA~1')"
C:\PROGRA~1

C:\Users\Matt>py -2 -c "import os; print  
os.path.realpath('C:\PROgRA~1\..\PROGRa~1')"
C:\PROGRa~1

C:\Users\Matt>py -2 -c "import os; print os.path.realpath('C:\WINDOWS')"
C:\WINDOWS

C:\Users\Matt>py -2 -c "import os; print os.path.realpath('C:\windows')"
C:\windows


I didn't have time to track this down, but I vaguely remember fixing some  
insane Windows path issue that somebody filed a bug about.  It was  
something along the lines of:

     cd D:\path\to\repo_pool
     cd C:\Users\foo
     hg -R D:repo status

... and expecting that to expand to D:\path\to\repo_pool\repo.  I don't  
remember if it was subrepo related, but I remember needing to use  
realpath() somewhere.  There definitely aren't tests for it.
Yuya Nishihara - Feb. 7, 2019, 11:30 a.m.
On Wed, 06 Feb 2019 23:16:43 -0500, Matt Harbison wrote:
> On Wed, 06 Feb 2019 06:54:02 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Tue, 05 Feb 2019 21:04:00 -0500, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1549417854 18000
> >> #      Tue Feb 05 20:50:54 2019 -0500
> >> # Branch stable
> >> # Node ID 0e18c6ec895542394c0ad18c380bf3bbd4ba4d9b
> >> # Parent  8b2892d5a9f2c06c998c977015a9ad3e3a3c9b5f
> >> subrepo: avoid false unsafe path detection on Windows
> >>
> >> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
> >> --- a/mercurial/subrepo.py
> >> +++ b/mercurial/subrepo.py
> >> @@ -405,7 +405,7 @@ class hgsubrepo(abstractsubrepo):
> >>          super(hgsubrepo, self).__init__(ctx, path)
> >>          self._state = state
> >>          r = ctx.repo()
> >> -        root = r.wjoin(path)
> >> +        root = os.path.realpath(r.wjoin(path))
> >
> > Can we do r.wjoin(util.localpath(path)) instead? Even though symlinks and
> > ".."s should be checked before, I want to avoid path resolution here for
> > extra safety.
> 
> Early indications are yes.  If the rest of the tests pass, I'll resend.
> I'm not real sure what the reason for avoiding path resolution is, so feel
> free to add commentary to the patch if you queue it.

This is a sanity check for bad subrepo paths, so I think it's better
to not trust that the given path is valid.

Patch

diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -405,7 +405,7 @@  class hgsubrepo(abstractsubrepo):
         super(hgsubrepo, self).__init__(ctx, path)
         self._state = state
         r = ctx.repo()
-        root = r.wjoin(path)
+        root = os.path.realpath(r.wjoin(path))
         create = allowcreate and not r.wvfs.exists('%s/.hg' % path)
         # repository constructor does expand variables in path, which is
         # unsafe since subrepo path might come from untrusted source.