Fixing wrong path in Classifier

[hun7err]

Unfortunately the former PR contained a bug resulting in an exception on request handling, this PR should fix this.

BTW, would it be possible to fix Travis tests? I've tried to take a look but have no idea what causes failed builds (only that it's something with Zend linking in BFR) :/

[glaslos]

There are currently a couple of issues in Travis. I'll try to fix at least the obvious ones ASAP.

[hun7err]

If you need any help fixing those more time-consuming problems just let me know, I'll take a look once I have some spare time.

[glaslos]

Could you rebase your PR?

[katkad]

i can cherry-pick it, just give me time

[glaslos]

@katkad did you see you can squash PRs now? Doesn't that make the cherry picking unnecessary?

[katkad]

squash ? where is button for that ? :)

i don't see anything

[glaslos]

When this PR is rebased, the merge button has a drop down to select squash :)

[katkad]

OK, so when I click Merge pull request, it doesn't merge it right away, but there is a drop down ?

but I still see this commit "Merge pull request #272 from hun7err/master "

[katkad]

eh... now there are 4 commits in this PR

one commit looks exactly the same as other commit

[hun7err]

Sorry, seems I've broken something yet again. Trying to fix this while wrapping up something else wasn't a good idea after all, can I still fix that on my end?

[katkad]

well, first thing, don't send PR from master branch. create your own branch for your feature/fix

master branch should not be messed with. I mean rebasing, changing things in your commits and such.

I can cherry-pick it, it will be the easiest way

[glaslos]

@katkad yeah, cherry pick it for now. This is how the squash should work: https://github.com/blog/2141-squash-your-commits Not sure why we don't see it for this repo, it's enabled in the config and I can see the button in other repos...

[katkad]

@glaslos nice, I didn't know about it. I'm not sure how it would behave for this PR, as it included changes already present in our master.

edit: now it would seem okay

[glaslos]

That could be the issue. Usually it works :)

[glaslos]

@katkad do you have time to cherry pick it or shall I do it?

[katkad]

@glaslos I have to verify it works. It takes time.

[katkad]

and if it makes sense. it doesn't

see those lines:

full_file_path = os.path.abspath(os.path.join(self.server_files_path, requested_file))
if full_file_path.startswith(self.server_files_path) and os.path.isfile(full_file_path):

you make variable by joining self.server_files_path and requested_file then you test, if that variable starts with self.server_files_path

I think it's useless

[glaslos]

Yeah, you are right. Let's close this PR. I'll fix it myself.

[katkad]

the problem is that there is no self.data_dir as there is in file_server.py

originally there is no check for full_file_path.startswith(self.data_dir)

I think more testing is needed. using abspath could be enough to avoid path traversal.

[katkad]

yep, there has to be full_file_path.startswith(self.data_dir) my mistake, sorry.

os.path.join(self.server_files_path, requested_file): /opt/glastopf-runner/data/server_files/../../../../../../../../../../../../../../../../etc/passwd full_file_path: /etc/passwd

[katkad]

I cherry-picked the fixing commit, and also added commit with comments.

@hun7err thank you for fix, and for finding the vulnerability i did some testing, and works like a charm :sparkles:

[hun7err]

you're welcome! I'm genuinely sorry for breaking this PR, though. Next time it'll be fixed properly with a separate branch.

[katkad]

@hun7err I suggest reading this https://github.com/mushorg/conpot/blob/master/docs/source/development/guidelines.rst part "For contributors"

if you have any questions, you can write me an e-mail. You can always create test branch in git and test what you are doing when rebasing, and so on. So you don't break your working branch.