New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix bug on upload file name #5571

Merged
merged 2 commits into from Dec 21, 2018

Conversation

7 participants
@lunny
Copy link
Member

lunny commented Dec 20, 2018

should fix #5569

@lunny lunny added the kind/bug label Dec 20, 2018

@lunny lunny added this to the 1.7.0 milestone Dec 20, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 20, 2018

Codecov Report

Merging #5571 into master will decrease coverage by <.01%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5571      +/-   ##
==========================================
- Coverage   37.55%   37.55%   -0.01%     
==========================================
  Files         321      321              
  Lines       47192    47206      +14     
==========================================
+ Hits        17723    17728       +5     
- Misses      26924    26931       +7     
- Partials     2545     2547       +2
Impacted Files Coverage Δ
routers/repo/editor.go 29.83% <40%> (+0.37%) ⬆️
modules/process/manager.go 76.81% <0%> (-4.35%) ⬇️
models/repo_list.go 64.55% <0%> (+1.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cb1d82...fbd2038. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 label Dec 20, 2018

@bkcsoft bkcsoft added lgtm/need 1 and removed lgtm/need 2 labels Dec 20, 2018

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Dec 20, 2018

Hmm... This maps "abc/../../def" to "abc/def" and doesn't really obey most filesystem semantics - this might break a virtual filesystem interpretation. Would it be better applying the path to its own virtual root eg. "abc/../../def" -> "../def" -> "def" instead?

@Bwko

Bwko approved these changes Dec 20, 2018

@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Dec 20, 2018

Setting this as blocked just because I do have a few thoughts about what @zeripath raised. While I am fine with the logic as it is right now, I think what @zeripath would be an improvement. I believe this because of the following https://play.golang.org/p/FIhECabR29W is how pure golang handles things, and I believe we should align with them, so there is no suprise between projects.

Of course, as always, my opinion is mine, and I am ok if others feel differently so please feel free to ignore this comment if you feel that way.

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Dec 20, 2018

If we do:

name = path.Join("/", path.Clean(name))[1:]

and perhaps

if string.HasPrefix(name, ".git/") || name == ".git" || len(name) == 0  {
   ctx.Error(500, "Upload file name is invalid")
   return
}

I guess that on windows we need to a little more to deal with its different separator?

@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Dec 20, 2018

@zeripath not sure that path.Clean works, due to the following: https://play.golang.org/p/arCS7K9TqeZ, using filepath.Abs and triming the leading / would probably handle more cases.

@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Dec 20, 2018

Per @lunny's comment in discord "welcome to change when I'm offline." I'm going to update to filepath.Abs

Edit: @zeripath reminded me that Abs doesn't play well on Windows, and that the leading / alleviates my issue that I had in my play link.

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Dec 20, 2018

@techknowlogick that's what the leading path.Join was for.

filepath.Abs won't work properly Windows I think.

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Dec 20, 2018

OK, thinking on. I think we should do:

name = path.Join("/", path.Clean( strings.Replace(name, "\\", "/", -1)))[1:]

I would guess that it's OK in general to disallow any use of backslash in uploaded filenames for every OS.

There will be some other paths that should be disallowed on Windows, for example COM LPT etc...


See https://en.wikipedia.org/wiki/Filename for reserved words, characters etc. I just don't know whether we need to actually look at these.

@snyff

This comment has been minimized.

Copy link

snyff commented Dec 21, 2018

It may be worth looking at the impact of being able to inject a .git directory if there is a submodule for this repo (instead of just searching for it in the prefix)

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Dec 21, 2018

Hmm... I guess the point is to protect the server not the downstream user. Remember in order to upload they can commit.

Now actually come to think of it I'm not certain why we're protecting the .git directory - we shouldn't actually be writing out a file to the repository rather, a commit to the repository so if someone wants to ruin their own repo then so be it. If we're actually writing out files under these addresses I'm a bit confused as it's clearly inefficient, similarly if we're ever reading out files without cleaning their path properly the issue isn't in the upload but in the reading part of the UI


The reason I say this is because if the issue is that we read files we shouldn't in UI it should be possible to commit a sufficiently weird repository that would replicate the bad upload process.

@snyff

This comment has been minimized.

Copy link

snyff commented Dec 21, 2018

The problem is that currently, the repo is cloned based on the repo_id, so you can create a file named .git/hooks/pre-commit and since it's not executable, no one will be able to upload a file (via the frontend) to this repository until the hook gets cleaned up.

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Dec 21, 2018

Ok. This whole section of code needs thoroughly rethinking. Cloning isn't and shouldn't be necessary to do this, bozaro/git-as-svn doesn't do that and it's converting from the SVN protocol to git on the fly.

@techknowlogick I'm happy for the current workaround to go in for now as we shouldn't leave this hole open any longer.

I will actually have to have a closer look at this code and figure out what is going on. There's likely to be a lot more problems here - not least with LFS.


@snyff do me a favour and try committing a repository with a file that has backslashes in it. A Linux based git should be happy with this file but I'm highly suspicious that gitea is going to do something bad.

@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Dec 21, 2018

I wasn't able to get access to a Windows VM to test the above workaround, so I will merge as is.

@techknowlogick techknowlogick merged commit 4a02a78 into go-gitea:master Dec 21, 2018

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details
@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Dec 21, 2018

No worries, I think I've established that this whole area of code needs rewriting.

@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Dec 21, 2018

backported to release/v1.6 in #5573

@lunny lunny deleted the lunny:lunny/fix_bug branch Dec 21, 2018

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Dec 21, 2018

It may be worth looking at the impact of being able to inject a .git directory if there is a submodule for this repo (instead of just searching for it in the prefix)

But somebody may put a test git repo on the git repo. So I think that should be allowed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment