Dev: add a function helper for #14650: Really throw error when user try to hack server #1248
Conversation
|
It's just a base idea : adding unlink, file_get_contents etc … all goes to App() with same restriction. After need to find all rmdirr call (but a grep and it's done) |
|
See 7b14cba#diff-f5392477bcb7ace25c65e8bd7abd39e9R384 for |
|
When error happen :
|
|
Shouldn't all rrmdir calls use the new function and remove the old one? |
|
Yes, it's why it's currently an idea , and need to be improved , update other accessand used in future … I make it after a security issue about template deletion where any user can remove any dir in server … it's fixed but without alert … Try to hack server => need a log way somewhezre (and in my opinion HTTP error is better). Related discussion about HTML errors : 9a8a031#comments |
|
@Shnoulle Can you go ahead and implement this for all rrmdir calls? |
Yes , clearly :) Maybe in https://github.com/LimeSurvey/LimeSurvey/blob/master/application/core/LSFileHelper.php and not in LS_Appliction ? My opinion : we must create a file loader helper too :) |
|
I don't think the right place to place this function would be in the LSYii_Application class. |
|
I leave it as draft, feel free for discussion. I surely rewrite all when i have time (some plugin make me work hard currently) |
There was a problem hiding this comment.
I don't think the right place to place this function would be in the LSYii_Application class.
@olleharstedt what do you think?
Maybe in https://github.com/LimeSurvey/LimeSurvey/blob/master/application/core/LSFileHelper.php and not in LS_Appliction ?
Unsue , FileHelper name is clear, but don't seems totally related.
| $baseDir = $this->getConfig('uploaddir'); | ||
| } | ||
| $dirPath = realpath($dirPath); | ||
| if(!is_dir($dirPath) || substr($dirPath, 0, strlen($baseDir)) !== $baseDir) { |
There was a problem hiding this comment.
@c-schmitz maybe just check substr($dirPath, 0, strlen($baseDir)) !== $baseDir to throw error ?
Just return false if not exist. A bad directory inside upload are not really an hack i think ?
There was a problem hiding this comment.
Is it possible to write a unit-test for this function? One test for happy path, one for failure. It makes it easier to understand the behaviour.
There was a problem hiding this comment.
Open to discussion : it's a 2 years old pull request …
But : decision about places before no ?
|
We already have rmdirr. Do we need two? 🤔 And yeah, I wouldn't expand the application class with helper functions. application/helpers/common_helper.php |
|
Oh, it's a wrapper for rmdirr, kind of. Let me read again. |
|
Yep : it's a wrapper When fixing : https://bugs.limesurvey.org/view.php?id=14617 Arbitrary FIle Download in LimeSurvey I think the fix solution are bad : because it's fix ONLY for this call. Then adding a wrapper to
Dev must use, if not : it's an issue. And in this wrapper : we can clearly send a Ecxeption : then server manager can have log of bad action. |
|
See the «fix» |
|
SO, either we finish this up or we close it. It's been around long enough. |
No description provided.