Deny invalid path

Review Request #2422 — Created April 4, 2023 and submitted

Information

grim/hgkeeper
default

Reviewers

If an authenticated user calls hg init hg.host.com/dummy/../../../etc
it will create the repository in another root directory if the process of
hgkeeper has permissions for this.
This could be an attack to the server.

Also hgkeeper admin repository can be overriden like this.
hg init ssh://hg.host.com/dummy/../hgkeeper/keys


 
Summary ID Author
Deny "hg init" for invalid path
If an authenticated user calls "hg init hg.host.com/dummy/../../../etc" it will create the repository in another root directory. If the process of hgkeeper as permissions it could lead to overwrite system files.
2003064d3ef00d9fa0eb452880164e1c8e26facb Andre Klitzing
Description From Last Updated

mh, that is a bad fix... let's retry...

aklitzing@gmail.comaklitzing@gmail.com

we probably want to use filepath.Clean here on repoName at a minimum.

grimgrim

not really a fan of a security related function being duplicated. I know this is because there's no good place …

grimgrim
aklitzing@gmail.com
aklitzing@gmail.com
aklitzing@gmail.com
  1. 
      
  2. In /r/2421/diff/1/#index_header I check if the repository is known.
    That should be checked here, too. It will always be a problem if someone creates a new repository in a sub-directory of another repository.

  3. 
      
aklitzing@gmail.com
aklitzing@gmail.com
  1. 
      
  2. Show all issues

    mh, that is a bad fix... let's retry...

  3. 
      
aklitzing@gmail.com
aklitzing@gmail.com
aklitzing@gmail.com
grim
  1. 
      
  2. ssh/commands/commands.go (Diff revision 5)
     
     
    Show all issues

    we probably want to use filepath.Clean here on repoName at a minimum.

  3. 
      
aklitzing@gmail.com
aklitzing@gmail.com
aklitzing@gmail.com
aklitzing@gmail.com
aklitzing@gmail.com
grim
  1. 
      
  2. ssh/commands/commands.go (Diff revision 9)
     
     
    Show all issues

    not really a fan of a security related function being duplicated. I know this is because there's no good place to put it right now. Maybe a normalize package?

    1. Yeah, I didn't like it, too. I moved it to new repositories.go as a normal func instead of another lambda call. So it uses the normal "canInit/canRead/canRemove" for permission which will fail for empty/invalid path.

  3. 
      
aklitzing@gmail.com
aklitzing@gmail.com
grim
  1. Ship It!
  2. Nice work, thanks!!

  3. 
      
grim
Review request changed
Status:
Completed