Is str_replace function will be enough to secure this code?
I want to allow users read files in their directory (and subdirectories) with GET param. I have to secure this part of code, so they cannot go to upper directories with two dots. Will it be enough?
$filePath = $userDir . '/' . str_replace( '..', '', $_GET['path'] );
if(!is_file($filePath)) {
die();
}
readfile($filePath);
php security
asked Jan 3 at 10:24
StanStan
6
add a comment |
I want to allow users read files in their directory (and subdirectories) with GET param. I have to secure this part of code, so they cannot go to upper directories with two dots. Will it be enough?
$filePath = $userDir . '/' . str_replace( '..', '', $_GET['path'] );
if(!is_file($filePath)) {
die();
}
readfile($filePath);
php security
asked Jan 3 at 10:24
StanStan
6
3
This may be personal preference, but I wouldn't let them choose the path at all (directly anyway). My suggestion is store the filenames (and fake path if you like) in a database, with a link to the real file on your filesystem. If they put in a valid file, thenreadfilethe real file... if that makes sense.
– Jonnix
Jan 3 at 10:25
Thank you for your reply. Yes, I agree with you, but unfortunately I'm not able to change database structure.
– Stan
Jan 3 at 10:33
I'd suggest replacing ../ and .., since .. by itself could be part of a valid filename like "abc..xyz.txt"
– jhilgeman
Jan 3 at 20:06
add a comment |
I want to allow users read files in their directory (and subdirectories) with GET param. I have to secure this part of code, so they cannot go to upper directories with two dots. Will it be enough?
$filePath = $userDir . '/' . str_replace( '..', '', $_GET['path'] );
if(!is_file($filePath)) {
die();
}
readfile($filePath);
php security
asked Jan 3 at 10:24
StanStan
6
I want to allow users read files in their directory (and subdirectories) with GET param. I have to secure this part of code, so they cannot go to upper directories with two dots. Will it be enough?
$filePath = $userDir . '/' . str_replace( '..', '', $_GET['path'] );
if(!is_file($filePath)) {
die();
}
readfile($filePath);
php security
php security
asked Jan 3 at 10:24
StanStan
6
asked Jan 3 at 10:24
StanStan
6
asked Jan 3 at 10:24
StanStan
6
asked Jan 3 at 10:24
StanStan
6
asked Jan 3 at 10:24
StanStan
6
6
3
This may be personal preference, but I wouldn't let them choose the path at all (directly anyway). My suggestion is store the filenames (and fake path if you like) in a database, with a link to the real file on your filesystem. If they put in a valid file, thenreadfilethe real file... if that makes sense.
– Jonnix
Jan 3 at 10:25
Thank you for your reply. Yes, I agree with you, but unfortunately I'm not able to change database structure.
– Stan
Jan 3 at 10:33
I'd suggest replacing ../ and .., since .. by itself could be part of a valid filename like "abc..xyz.txt"
– jhilgeman
Jan 3 at 20:06
add a comment |
3
This may be personal preference, but I wouldn't let them choose the path at all (directly anyway). My suggestion is store the filenames (and fake path if you like) in a database, with a link to the real file on your filesystem. If they put in a valid file, thenreadfilethe real file... if that makes sense.
– Jonnix
Jan 3 at 10:25
Thank you for your reply. Yes, I agree with you, but unfortunately I'm not able to change database structure.
– Stan
Jan 3 at 10:33
I'd suggest replacing ../ and .., since .. by itself could be part of a valid filename like "abc..xyz.txt"
– jhilgeman
Jan 3 at 20:06
3
3
This may be personal preference, but I wouldn't let them choose the path at all (directly anyway). My suggestion is store the filenames (and fake path if you like) in a database, with a link to the real file on your filesystem. If they put in a valid file, then
readfile the real file... if that makes sense.– Jonnix
Jan 3 at 10:25
This may be personal preference, but I wouldn't let them choose the path at all (directly anyway). My suggestion is store the filenames (and fake path if you like) in a database, with a link to the real file on your filesystem. If they put in a valid file, then
readfile the real file... if that makes sense.– Jonnix
Jan 3 at 10:25
Thank you for your reply. Yes, I agree with you, but unfortunately I'm not able to change database structure.
– Stan
Jan 3 at 10:33
Thank you for your reply. Yes, I agree with you, but unfortunately I'm not able to change database structure.
– Stan
Jan 3 at 10:33
I'd suggest replacing ../ and .., since .. by itself could be part of a valid filename like "abc..xyz.txt"
– jhilgeman
Jan 3 at 20:06
I'd suggest replacing ../ and .., since .. by itself could be part of a valid filename like "abc..xyz.txt"
– jhilgeman
Jan 3 at 20:06
add a comment |
1 Answer
1
active
oldest
votes
Be careful here!
Even if you replace "../" and "..", make sure you do it recursively. Because if you don't, a malicious user can submit the following to bypass your security:
"..././"
$path = '..././';
var_dump(str_replace('../', '', $path));
// string(3) "../"
See the problem here?
You can use a do/while loop to sanitise the user input in a secure manner:
$path = '..././';
do {
$path = str_replace(['../', '..\'], '', $path, $count);
} while ($count > 0);
var_dump($path);
// string(0) ""
But I'm with Jon on this. It's usually not a good idea to allow this, and avoid it whenever possible. Be as restrictive as you can with the filenames. Always prepend the user path instead of passing the full path via URL param.
answered Jan 5 at 10:59
NicoNico
11
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "1"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: true,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: 10,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fstackoverflow.com%2fquestions%2f54020366%2fis-str-replace-function-will-be-enough-to-secure-this-code%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
Be careful here!
Even if you replace "../" and "..", make sure you do it recursively. Because if you don't, a malicious user can submit the following to bypass your security:
"..././"
$path = '..././';
var_dump(str_replace('../', '', $path));
// string(3) "../"
See the problem here?
You can use a do/while loop to sanitise the user input in a secure manner:
$path = '..././';
do {
$path = str_replace(['../', '..\'], '', $path, $count);
} while ($count > 0);
var_dump($path);
// string(0) ""
But I'm with Jon on this. It's usually not a good idea to allow this, and avoid it whenever possible. Be as restrictive as you can with the filenames. Always prepend the user path instead of passing the full path via URL param.
answered Jan 5 at 10:59
NicoNico
11
add a comment |
Be careful here!
Even if you replace "../" and "..", make sure you do it recursively. Because if you don't, a malicious user can submit the following to bypass your security:
"..././"
$path = '..././';
var_dump(str_replace('../', '', $path));
// string(3) "../"
See the problem here?
You can use a do/while loop to sanitise the user input in a secure manner:
$path = '..././';
do {
$path = str_replace(['../', '..\'], '', $path, $count);
} while ($count > 0);
var_dump($path);
// string(0) ""
But I'm with Jon on this. It's usually not a good idea to allow this, and avoid it whenever possible. Be as restrictive as you can with the filenames. Always prepend the user path instead of passing the full path via URL param.
answered Jan 5 at 10:59
NicoNico
11
add a comment |
Be careful here!
Even if you replace "../" and "..", make sure you do it recursively. Because if you don't, a malicious user can submit the following to bypass your security:
"..././"
$path = '..././';
var_dump(str_replace('../', '', $path));
// string(3) "../"
See the problem here?
You can use a do/while loop to sanitise the user input in a secure manner:
$path = '..././';
do {
$path = str_replace(['../', '..\'], '', $path, $count);
} while ($count > 0);
var_dump($path);
// string(0) ""
But I'm with Jon on this. It's usually not a good idea to allow this, and avoid it whenever possible. Be as restrictive as you can with the filenames. Always prepend the user path instead of passing the full path via URL param.
answered Jan 5 at 10:59
NicoNico
11
Be careful here!
Even if you replace "../" and "..", make sure you do it recursively. Because if you don't, a malicious user can submit the following to bypass your security:
"..././"
$path = '..././';
var_dump(str_replace('../', '', $path));
// string(3) "../"
See the problem here?
You can use a do/while loop to sanitise the user input in a secure manner:
$path = '..././';
do {
$path = str_replace(['../', '..\'], '', $path, $count);
} while ($count > 0);
var_dump($path);
// string(0) ""
But I'm with Jon on this. It's usually not a good idea to allow this, and avoid it whenever possible. Be as restrictive as you can with the filenames. Always prepend the user path instead of passing the full path via URL param.
answered Jan 5 at 10:59
NicoNico
11
edited Jan 5 at 11:06
answered Jan 5 at 10:59
NicoNico
11
answered Jan 5 at 10:59
NicoNico
11
answered Jan 5 at 10:59
NicoNico
11
11
add a comment |
add a comment |
Thanks for contributing an answer to Stack Overflow!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fstackoverflow.com%2fquestions%2f54020366%2fis-str-replace-function-will-be-enough-to-secure-this-code%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
3
This may be personal preference, but I wouldn't let them choose the path at all (directly anyway). My suggestion is store the filenames (and fake path if you like) in a database, with a link to the real file on your filesystem. If they put in a valid file, then
readfilethe real file... if that makes sense.– Jonnix
Jan 3 at 10:25
Thank you for your reply. Yes, I agree with you, but unfortunately I'm not able to change database structure.
– Stan
Jan 3 at 10:33
I'd suggest replacing ../ and .., since .. by itself could be part of a valid filename like "abc..xyz.txt"
– jhilgeman
Jan 3 at 20:06