Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-29749

Extension of uploaded file changes due to multiple dots in the filename

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1
    • Fix Version/s: 2.5
    • Component/s: Files API, Repositories
    • Labels:
    • Environment:
      centos 5.x x86_64, postgres 8.4, apache 2.2.x php 5.3.x
    • Database:
      PostgreSQL
    • Testing Instructions:
      Hide

      Test pre-requisites

      • Feel destructive
      • Create the following files:
        • myfile...txt
        • myfile.f..txt
        • myfile..f.txt
        • . . .txt
        • blah.jpg...
      • Create a few other files with random dots inside.

      Test steps

      1. Go to your private files
      2. Upload the above created files
      3. Make sure each file keeps its original extension
      4. Make sure the file names are cleaned up as expected
      5. Upload the attached zip file `funkynames.zip`, and unzip it
      6. Make sure the files and folders names are as the originals (you'll have to click on each file/folder to check the white spaces)
      Show
      Test pre-requisites Feel destructive Create the following files: myfile...txt myfile.f..txt myfile..f.txt . . .txt blah.jpg... Create a few other files with random dots inside. Test steps Go to your private files Upload the above created files Make sure each file keeps its original extension Make sure the file names are cleaned up as expected Upload the attached zip file `funkynames.zip`, and unzip it Make sure the files and folders names are as the originals (you'll have to click on each file/folder to check the white spaces)
    • Workaround:
      Hide

      Rename the file after uploading, then the multiple dots work again.

      Show
      Rename the file after uploading, then the multiple dots work again.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-29749-master-4th

      Description

      What happens:
      -------------------------------------------
      Let's assume we have a filename with multiple dots at the end of the filename before the extension, like for example foo_bar_e.g..txt
      (the e.g. has a dot at the end and the dot of the extension follows). After the fileupload with the upload repository, the file is stored as foo_bar_e.gtxt.

      Expected Behaviour:
      -------------------------------------------
      Filenames with mutliple dots should be uploaded faithfully (as long as the filename is valid, of course).

      Remark:
      -------------------------------------------
      This is especially a problem if the multiple dots are near the extension so that the extension changes. If - for example - an extension changes from .a..pdf to .apdf, then the file is no longer identified as a pdf file and users are left confused.

      Replication steps:
      a.) Generate a file with multiple dots near the extension (examples: foo..bar.gz or foor_bar.e.g..pdf)
      b.) Upload using the upload repository
      c.) find the extension changed

        Gliffy Diagrams

        1. mdl-32370_22.patch
          1 kB
          Saswat Padhi

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting this.

            I've put that on the backlog.

            In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting this. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.
            Hide
            saswatpadhi Saswat Padhi added a comment -

            This was because of a regex replacement done in clean_param function in lib/moodlelib.php ..

            $param = preg_replace('~\.\.+~', '', $param);

            The check, specifically removes one or more consecutive periods in the final filename of saved file.

            I have uploaded a patch that disables the check.

            I don't think it should result in any security issues, unless the dots are surrounded by slashes. But slashes are thrown away in another check.
            So, the patch is safe.

            Pull Master Diff : https://github.com/SaswatPadhi/moodle/compare/master...MDL-32370
            Pull MDL2.2 Diff : https://github.com/SaswatPadhi/moodle/compare/MOODLE_22_STABLE...MDL-32370_22

            Show
            saswatpadhi Saswat Padhi added a comment - This was because of a regex replacement done in clean_param function in lib/moodlelib.php .. $param = preg_replace('~\.\.+~', '', $param); The check, specifically removes one or more consecutive periods in the final filename of saved file. I have uploaded a patch that disables the check. I don't think it should result in any security issues, unless the dots are surrounded by slashes. But slashes are thrown away in another check. So, the patch is safe . Pull Master Diff : https://github.com/SaswatPadhi/moodle/compare/master...MDL-32370 Pull MDL2.2 Diff : https://github.com/SaswatPadhi/moodle/compare/MOODLE_22_STABLE...MDL-32370_22
            Hide
            kuraga Alexander Kurakin added a comment - - edited

            Ok what about to release the patch?

            Show
            kuraga Alexander Kurakin added a comment - - edited Ok what about to release the patch?
            Hide
            vadimon Vadim Dvorovenko added a comment -

            The problem is much deeper.
            In windows its impossible to create folder or file with trailing dots and speces like 'test.','test ' or 'test ... '. All trailing speces are removed.
            But spaces and dots are normal in the beginning of filename. '.test',' test' or ' ... test' are normal filenames.
            Such behaviour i see in php and apache on windows. Even if i try to download file http://localhost/moodle/draftfile.php/5/user/draft/606933594/.....test.../file.txt in $_SERVER[PATH_INFO] i see http://localhost/moodle/draftfile.php/5/user/draft/606933594/.....test/file.txt
            So we should clean dots and spaces only at the end of filenames and path parts (before every '/').

            Show
            vadimon Vadim Dvorovenko added a comment - The problem is much deeper. In windows its impossible to create folder or file with trailing dots and speces like 'test.','test ' or 'test ... '. All trailing speces are removed. But spaces and dots are normal in the beginning of filename. '.test',' test' or ' ... test' are normal filenames. Such behaviour i see in php and apache on windows. Even if i try to download file http://localhost/moodle/draftfile.php/5/user/draft/606933594/.....test.../file.txt in $_SERVER [PATH_INFO] i see http://localhost/moodle/draftfile.php/5/user/draft/606933594/.....test/file.txt So we should clean dots and spaces only at the end of filenames and path parts (before every '/').
            Show
            vadimon Vadim Dvorovenko added a comment - patches here. It also fixes MDL-28276 2.2: https://github.com/vadimonus/moodle/commits/MDL-32370-22 https://github.com/vadimonus/moodle/commit/5264f91e6583ec5b56e87717740274ff2ce3e04f 2.3: https://github.com/vadimonus/moodle/tree/MDL-32370-23 https://github.com/vadimonus/moodle/commit/2edefdd7136c057846131bf8c5e876c99ccbc3ca 2.4: https://github.com/vadimonus/moodle/tree/MDL-32370-24 https://github.com/vadimonus/moodle/commit/060c4ea7e22a079cef9d639f72d8af88d256b469
            Hide
            kuraga Alexander Kurakin added a comment -

            I think it's wrong to talk about some platform and file renaming. It's not a file on Windows' disk but it's a resourse consists of a file in a site. And people don't know about these details of file renaming.

            We're talking about a name of resourse. But not about real filename with extension.

            Show
            kuraga Alexander Kurakin added a comment - I think it's wrong to talk about some platform and file renaming. It's not a file on Windows' disk but it's a resourse consists of a file in a site. And people don't know about these details of file renaming. We're talking about a name of resourse. But not about real filename with extension.
            Hide
            vadimon Vadim Dvorovenko added a comment -

            The apache issue is discussed here. https://issues.apache.org/bugzilla/show_bug.cgi?id=20036
            Idea is that windows does not see difference between 'test', 'test.' and 'test..'

            Even if you can create file \..test..\file.txt, and it is normal resource URI, you cannot download such file back from windows apache server, because it convert this path to \..test\file.txt. So your course with such file become platform-dependent.

            I think we should clean dots at the end of file and directory names for cross-platform compability. And so my patch does.

            The other way is to rewrite get_file_argument() (weblib.php) to use something other instead of $_SERVER['PATH_INFO'] but i think it will lead to many compability problems with other web-servers.

            Show
            vadimon Vadim Dvorovenko added a comment - The apache issue is discussed here. https://issues.apache.org/bugzilla/show_bug.cgi?id=20036 Idea is that windows does not see difference between 'test', 'test.' and 'test..' Even if you can create file \..test..\file.txt, and it is normal resource URI, you cannot download such file back from windows apache server, because it convert this path to \..test\file.txt. So your course with such file become platform-dependent. I think we should clean dots at the end of file and directory names for cross-platform compability. And so my patch does. The other way is to rewrite get_file_argument() (weblib.php) to use something other instead of $_SERVER ['PATH_INFO'] but i think it will lead to many compability problems with other web-servers.
            Show
            vadimon Vadim Dvorovenko added a comment - - edited I've rebased patches on latest releases. Take a look at https://github.com/vadimonus/moodle/tree/MDL-32370-24 https://github.com/vadimonus/moodle/tree/MDL-32370-23 https://github.com/vadimonus/moodle/tree/MDL-32370-22 diff https://github.com/vadimonus/moodle/commit/82c723ec2314ec2a697f9e1286cbfd2124c5d32c
            Hide
            kuraga Alexander Kurakin added a comment -

            Ok. What's the next?

            Show
            kuraga Alexander Kurakin added a comment - Ok. What's the next?
            Hide
            vadimon Vadim Dvorovenko added a comment - - edited

            Attached testing file.
            Instruction: unzip "dots & spaces.zip" on local computer and in moodle resourse. Compare local and remote filenames - some filenames have changed.

            Show
            vadimon Vadim Dvorovenko added a comment - - edited Attached testing file. Instruction: unzip "dots & spaces.zip" on local computer and in moodle resourse. Compare local and remote filenames - some filenames have changed.
            Hide
            fred Frédéric Massart added a comment -

            Thanks everyone for your work on this issue. So it appears that we simple remove any multiple dots with nothing, which is a problem when you name your file blah..jpg as it would become blahjpg. Unfortunately I did not use the patch nicely offered by Vadim because I think that it is not strict enough and that the existing logic wants to prevent any hack introducing path in file names for examples.

            The solution I am suggesting is to keep the strictness but to replace the multiple dots with only one. But before that, I ensured that paths would not have been hidden in the file name. Along with that are some more PHP Unit test cases to prove the efficiency of the code. I would have preferred not to add a new preg_replace, but that's the only way I could think of to stay as close as possible to the current behaviour, without changing the existing test cases.

            Thanks! Pushing for peer review.

            Show
            fred Frédéric Massart added a comment - Thanks everyone for your work on this issue. So it appears that we simple remove any multiple dots with nothing, which is a problem when you name your file blah..jpg as it would become blahjpg . Unfortunately I did not use the patch nicely offered by Vadim because I think that it is not strict enough and that the existing logic wants to prevent any hack introducing path in file names for examples. The solution I am suggesting is to keep the strictness but to replace the multiple dots with only one. But before that, I ensured that paths would not have been hidden in the file name. Along with that are some more PHP Unit test cases to prove the efficiency of the code. I would have preferred not to add a new preg_replace, but that's the only way I could think of to stay as close as possible to the current behaviour, without changing the existing test cases. Thanks! Pushing for peer review.
            Hide
            vadimon Vadim Dvorovenko added a comment -

            Hi, Frédéric. Your are offering incomplete solution.
            Just think. You're loading file `Strange....file.html` and `Strange....file.files` folder, created by IE, all together into a zip file. After your loading, files will look like `Strange.file.html` and `Strange.file.files`, that will break html links.
            In my opinion we should drop any `/./`, `/../`, `/.../` from folder names and these are the only combinations, that can cause security problems. (And shurely `..\` at the beginning, `\..` at the and of the string).
            As for filenames - they should not contain `/` at all, so they cannot contain `/../`.
            In addition, whe should strip all dots and spaces from the and of folders and files names - not for security, but for windows apache compability.

            As for my patch. Parsing directory path into parts with `/` and removing traling dots and spaces in each part will eliminate any appearence of `/./`, `/../` and so on. As for filenames, dropping `/` and trialing dots and spaces will be enough. In any case, any filepath, that is correct in Windows, whould be considered correct in Moodle.

            Show
            vadimon Vadim Dvorovenko added a comment - Hi, Frédéric. Your are offering incomplete solution. Just think. You're loading file `Strange....file.html` and `Strange....file.files` folder, created by IE, all together into a zip file. After your loading, files will look like `Strange.file.html` and `Strange.file.files`, that will break html links. In my opinion we should drop any `/./`, `/../`, `/.../` from folder names and these are the only combinations, that can cause security problems. (And shurely `..\` at the beginning, `\..` at the and of the string). As for filenames - they should not contain `/` at all, so they cannot contain `/../`. In addition, whe should strip all dots and spaces from the and of folders and files names - not for security, but for windows apache compability. As for my patch. Parsing directory path into parts with `/` and removing traling dots and spaces in each part will eliminate any appearence of `/./`, `/../` and so on. As for filenames, dropping `/` and trialing dots and spaces will be enough. In any case, any filepath, that is correct in Windows, whould be considered correct in Moodle.
            Hide
            fred Frédéric Massart added a comment - - edited

            Hi Vadim,

            you are right, I forgot about the use case where you upload an HTML file and the compatibility with Windows. The tricky part is that this code is used all over the place and the current logic has been used over and over in many places based on the current behaviour. Changing this is dangerous, and could lead to potential problems. IMO if we change the way dots are handled by allowing as many as we want as long as they're not the last character, then this should be a master only issue.

            I'll have another look at the issue after gathering some more feedback from others.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - - edited Hi Vadim, you are right, I forgot about the use case where you upload an HTML file and the compatibility with Windows. The tricky part is that this code is used all over the place and the current logic has been used over and over in many places based on the current behaviour. Changing this is dangerous, and could lead to potential problems. IMO if we change the way dots are handled by allowing as many as we want as long as they're not the last character, then this should be a master only issue. I'll have another look at the issue after gathering some more feedback from others. Cheers, Fred
            Hide
            fred Frédéric Massart added a comment -

            I'm taking this issue out of peer review to work a bit more on it. Vadim, in this issue I'd rather focus on the file names and let the other issue take care of the path issue.

            Show
            fred Frédéric Massart added a comment - I'm taking this issue out of peer review to work a bit more on it. Vadim, in this issue I'd rather focus on the file names and let the other issue take care of the path issue.
            Hide
            fred Frédéric Massart added a comment -

            Ok, here is the patch back for peer review. That time I used your logic over mine, but I've recreated the commit to exclude the changes related to the file path. I have also added a commit on top of yours to add multiple scenarios to the Unit Test.

            (That'd be nice if you could extract the rest of your original patch and create a branch dedicated to the other issue, easy for us to review, credit and push forward ).

            Thanks!
            Fred

            Show
            fred Frédéric Massart added a comment - Ok, here is the patch back for peer review. That time I used your logic over mine, but I've recreated the commit to exclude the changes related to the file path. I have also added a commit on top of yours to add multiple scenarios to the Unit Test. (That'd be nice if you could extract the rest of your original patch and create a branch dedicated to the other issue, easy for us to review, credit and push forward ). Thanks! Fred
            Hide
            vadimon Vadim Dvorovenko added a comment -

            No problem, i'll make new patch in some days. Currently there's no unittest for cleanparam with PARAM _PATH mode, i'll try to write some.
            Also i'l try to investigate places, where this mode is used.

            Show
            vadimon Vadim Dvorovenko added a comment - No problem, i'll make new patch in some days. Currently there's no unittest for cleanparam with PARAM _PATH mode, i'll try to write some. Also i'l try to investigate places, where this mode is used.
            Hide
            fred Frédéric Massart added a comment -

            Awesome, thanks Vadim!

            Show
            fred Frédéric Massart added a comment - Awesome, thanks Vadim!
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Hello Fred and Vadim,

            Patch looks good, although there are few things you might want to consider:

            1. As we are allowing some file names which are not compatible with windows then why we are adding https://github.com/FMCorz/moodle/compare/moodle:master...MDL-29749-master#L0R924
              1. File name staring with space ( . .dontltrim.me)
              2. File having * in file name
              3. Files having ? in file name
              4. Should we also consider filtering reserved words? http://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words
            2. ., .. (dot and double dot) are reserved by system but IMO ... (triple dot) should be allowed, if we are making it so lenient. Try add file in dropbox as ... and it is allowed file.

            Adding Petr as watcher to get his thoughts on this.

            FYI:
            With current patch blah.jpg... will become blah.jpg while . . .txt will retain it's name. I think tester will fail this, as he/she can create file as blag.jpg..., so expects to retain it's name.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Hello Fred and Vadim, Patch looks good, although there are few things you might want to consider: As we are allowing some file names which are not compatible with windows then why we are adding https://github.com/FMCorz/moodle/compare/moodle:master...MDL-29749-master#L0R924 File name staring with space ( . .dontltrim.me) File having * in file name Files having ? in file name Should we also consider filtering reserved words? http://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words ., .. (dot and double dot) are reserved by system but IMO ... (triple dot) should be allowed, if we are making it so lenient. Try add file in dropbox as ... and it is allowed file. Adding Petr as watcher to get his thoughts on this. FYI: With current patch blah.jpg... will become blah.jpg while . . .txt will retain it's name. I think tester will fail this, as he/she can create file as blag.jpg..., so expects to retain it's name.
            Hide
            fred Frédéric Massart added a comment -

            That's a good point Raj, I didn't think of the * and ?, but about multiple dots and spaces at the beginning of a file, it is Windows compliant (at least on 7, which I've tested). Also, ? and * can be used in the file name, but they will be removed. That also makes me think that file names should not be longer than 255. Regarding '...' I don't think it should be accepted as it ends with a . which is not Windows compliant.

            My point of view would be to adapt the same logic than Google Drive. Everything and anything is accepted because the file name is just an entry in a database which can store anything. The file is already identified by a unique ID (hash) so the file name has not much importance. The problem would occur when some files are zipped or downloaded on the user's machine, and that's at that point that we should secure the file names.

            Anyway, whether we extend the functionalities of the file area later on or not, there is still need for a stronger/stricter PARAM_FILE. I'm looking forward to read other's input on this issue as I'm getting confused on the goal that we're trying to achieve here. Just noting that the purpose of this issue has been fulfilled.

            (FYI Dropbox accepts '...' but not those characters: \ / : ? * < > " |)

            Show
            fred Frédéric Massart added a comment - That's a good point Raj, I didn't think of the * and ?, but about multiple dots and spaces at the beginning of a file, it is Windows compliant (at least on 7, which I've tested). Also, ? and * can be used in the file name, but they will be removed. That also makes me think that file names should not be longer than 255. Regarding '...' I don't think it should be accepted as it ends with a . which is not Windows compliant. My point of view would be to adapt the same logic than Google Drive. Everything and anything is accepted because the file name is just an entry in a database which can store anything. The file is already identified by a unique ID (hash) so the file name has not much importance. The problem would occur when some files are zipped or downloaded on the user's machine, and that's at that point that we should secure the file names. Anyway, whether we extend the functionalities of the file area later on or not, there is still need for a stronger/stricter PARAM_FILE. I'm looking forward to read other's input on this issue as I'm getting confused on the goal that we're trying to achieve here. Just noting that the purpose of this issue has been fulfilled. (FYI Dropbox accepts '...' but not those characters: \ / : ? * < > " |)
            Hide
            kuraga Alexander Kurakin added a comment -

            > Everything and anything is accepted because the file name is just an entry in a database which can store anything.
            > The problem would occur when some files are zipped or downloaded on the user's machine, and that's at that point that we should secure the file names.

            +1. I think Moodle may store anything as filename (resource name), but should secure them WHEN it's needed.

            For example I can use "<" symbol in this message ("filename"). But it's deprecated by HTML ("it's not Windows compliant"). Ok, system sanitizes it when DISPLAYs. But it's not deprecated.

            Show
            kuraga Alexander Kurakin added a comment - > Everything and anything is accepted because the file name is just an entry in a database which can store anything. > The problem would occur when some files are zipped or downloaded on the user's machine, and that's at that point that we should secure the file names. +1. I think Moodle may store anything as filename (resource name), but should secure them WHEN it's needed. For example I can use "<" symbol in this message ("filename"). But it's deprecated by HTML ("it's not Windows compliant"). Ok, system sanitizes it when DISPLAYs. But it's not deprecated.
            Hide
            fred Frédéric Massart added a comment -

            All right. This is getting a bit confusing and I'll try to summarise what Raj and I have been talking about.

            • Allowing all file names in the File Manager is great, but not easily possible and would require a lot of tests to make sure this does not introduce security issues, so this is not a solution for now;
            • We should not go for platforms specific rules. Even though Windows is one of the most popular non-Unix platform, there are other systems which allow/require different rules, so why not being all platform compliant, which is horrible and would not suit anyone?
            • Disallowing spaces and . at the end of the file names creates a regression for Unix users for whom such file names are valids.
            • Downloading a file with an invalid file name for the system used, will most certainly be handled by the OS and renamed accordingly (or prompts the user for a new name).

            In conclusion, I think we should keep the current logic that strips out the invalid characters while allowing multiple dots anywhere in the file name.

            Lots of back and forth here... sorry about that Vadim. I'll work on that patch again, probably including your changes for PARAM_PATH as they're required to test file unzipping.

            Show
            fred Frédéric Massart added a comment - All right. This is getting a bit confusing and I'll try to summarise what Raj and I have been talking about. Allowing all file names in the File Manager is great, but not easily possible and would require a lot of tests to make sure this does not introduce security issues, so this is not a solution for now; We should not go for platforms specific rules. Even though Windows is one of the most popular non-Unix platform, there are other systems which allow/require different rules, so why not being all platform compliant, which is horrible and would not suit anyone? Disallowing spaces and . at the end of the file names creates a regression for Unix users for whom such file names are valids. Downloading a file with an invalid file name for the system used, will most certainly be handled by the OS and renamed accordingly (or prompts the user for a new name). In conclusion, I think we should keep the current logic that strips out the invalid characters while allowing multiple dots anywhere in the file name. Lots of back and forth here... sorry about that Vadim. I'll work on that patch again, probably including your changes for PARAM_PATH as they're required to test file unzipping.
            Hide
            fred Frédéric Massart added a comment -

            Pushing one more time for peer review. This time I have introduced the changes related to PARAM_PATH. After discussing with Petr and Raj, here is the conclusion made:

            • We should not be stricter while cleaning the file names as they're used in the hash generated by the file API;
            • Disallowing more characters (trimming the path/files) is being more strict in a way, also if we're specific to Windows, we should be specific for many OS;
            • Backporting is dangerous and not worth the risk;
            • PARAM_PATH is using the PARAM_FILE to clean each element of the path. One exception is made for the first element, when it is a dot. This to maintain the existing behaviour.
            • file_correct_path() is an obscure function, which I'm not sure to understand, but that's the one responsible of trimming folder names while navigating through the File Manager. All characters trimmed should have been allowed, that's what I did. My +1 to deprecate this function and remove it as much as we can (or force it to use PARAM_PATH)!

            Vadim, I am sorry I couldn't make use of your patch, the main reason being that we don't want to be stricter and platform specific, when the goal of this issue has been reached.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Pushing one more time for peer review. This time I have introduced the changes related to PARAM_PATH. After discussing with Petr and Raj, here is the conclusion made: We should not be stricter while cleaning the file names as they're used in the hash generated by the file API; Disallowing more characters (trimming the path/files) is being more strict in a way, also if we're specific to Windows, we should be specific for many OS; Backporting is dangerous and not worth the risk; PARAM_PATH is using the PARAM_FILE to clean each element of the path. One exception is made for the first element, when it is a dot. This to maintain the existing behaviour. file_correct_path() is an obscure function, which I'm not sure to understand, but that's the one responsible of trimming folder names while navigating through the File Manager. All characters trimmed should have been allowed, that's what I did. My +1 to deprecate this function and remove it as much as we can (or force it to use PARAM_PATH)! Vadim, I am sorry I couldn't make use of your patch, the main reason being that we don't want to be stricter and platform specific, when the goal of this issue has been reached. Cheers, Fred
            Hide
            fred Frédéric Massart added a comment -

            Raised MDL-37561 to improve file name compatibility when download/export/zipping, etc...

            Show
            fred Frédéric Massart added a comment - Raised MDL-37561 to improve file name compatibility when download/export/zipping, etc...
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Patch looks good now, some points for you before you push it for integration.

            FYI:

            1. Not sure if (.) dot is required at the end of this comment. https://github.com/FMCorz/moodle/compare/moodle:master...MDL-29749-master-4th#L1R936
            2. Personally I would just remove (.) from https://github.com/FMCorz/moodle/compare/moodle:master...MDL-29749-master-4th#L0L553 and leave it till someone with special need want it in start and end of file. But leaving it for you to decide.
            3. In PARAM_PATH we are allowing ~ (root), but not .. (parent directory). Also, look at lib/filestorage/stored_file.php - line 128 (// path must start and end with '/'), not sure if it will lead to any regression. Same in lib/filestorage/file_storage.php Line 725
            Show
            rajeshtaneja Rajesh Taneja added a comment - Patch looks good now, some points for you before you push it for integration. FYI: Not sure if (.) dot is required at the end of this comment. https://github.com/FMCorz/moodle/compare/moodle:master...MDL-29749-master-4th#L1R936 Personally I would just remove (.) from https://github.com/FMCorz/moodle/compare/moodle:master...MDL-29749-master-4th#L0L553 and leave it till someone with special need want it in start and end of file. But leaving it for you to decide. In PARAM_PATH we are allowing ~ (root), but not .. (parent directory). Also, look at lib/filestorage/stored_file.php - line 128 (// path must start and end with '/'), not sure if it will lead to any regression. Same in lib/filestorage/file_storage.php Line 725
            Hide
            fred Frédéric Massart added a comment -

            Thanks Raj, I am pushing for integration now.

            1. Good catch, I've added it
            2. I decided to remove everything that was permitted in the File Manager. Note that this only trims and the end and beginning of a path, meaning that we tolerate them in the middle... does it make sense? Also, I'd rather change this now in master, and be aware of the silly regression that this could create before 2.5 lands. Also, not changing them means that folders ending/starting with @$# are not accessible in the File Manager.
            3. I took great care to respect the exact previous behaviour. If you try those Unit Test, they will all pass except for the ones related to this issue, which are the ones with multiple dots in path. I am not expecting any regression coming from the use of this PARAM_ except if someone used it to avoid multiple dots...

            Many thanks!
            Fred

            Show
            fred Frédéric Massart added a comment - Thanks Raj, I am pushing for integration now. 1. Good catch, I've added it 2. I decided to remove everything that was permitted in the File Manager. Note that this only trims and the end and beginning of a path, meaning that we tolerate them in the middle... does it make sense? Also, I'd rather change this now in master, and be aware of the silly regression that this could create before 2.5 lands. Also, not changing them means that folders ending/starting with @$# are not accessible in the File Manager. 3. I took great care to respect the exact previous behaviour. If you try those Unit Test, they will all pass except for the ones related to this issue, which are the ones with multiple dots in path. I am not expecting any regression coming from the use of this PARAM_ except if someone used it to avoid multiple dots... Many thanks! Fred
            Hide
            vadimon Vadim Dvorovenko added a comment - - edited

            Hello everybody
            I'm happy that old poblem soon will be solved. But let's look thoroughly.

            If we are going to be not as strict as windows, we should allow more symbols in filenames.
            Let's look at unittests:
            currently clean_param('b\'a<d`\/fi:l>e.t"x|t', PARAM_FILE) = 'badfile.txt'
            and i think it should result in clean_param('b\'a<d`\/fi:l>e.t"x|t', PARAM_PATH) == 'b\'a<d`fi:l>e.t"x|t' , because i can easyly create such filename in unix.
            Even if we try to be windows friendly, this should be clean_param('b\'a<d`\/fi:l>e.t"x|t', PARAM_PATH) == 'b\'ad`file.txt'.
            ` and ' can be used both with windows and unix
            also it's not good, that we do not have unittsets containing * and ? symbols, that also have special meaning in many OS

            as file_correct_path(), +1 to deprecate it

            Show
            vadimon Vadim Dvorovenko added a comment - - edited Hello everybody I'm happy that old poblem soon will be solved. But let's look thoroughly. If we are going to be not as strict as windows, we should allow more symbols in filenames. Let's look at unittests: currently clean_param('b\'a<d`\/fi:l>e.t"x|t', PARAM_FILE) = 'badfile.txt' and i think it should result in clean_param('b\'a<d`\/fi:l>e.t"x|t', PARAM_PATH) == 'b\'a<d`fi:l>e.t"x|t' , because i can easyly create such filename in unix. Even if we try to be windows friendly, this should be clean_param('b\'a<d`\/fi:l>e.t"x|t', PARAM_PATH) == 'b\'ad`file.txt'. ` and ' can be used both with windows and unix also it's not good, that we do not have unittsets containing * and ? symbols, that also have special meaning in many OS as file_correct_path(), +1 to deprecate it
            Hide
            fred Frédéric Massart added a comment -

            I agree with you, and as said above we should probably allow any character in the file names. The problem here is that we are using PARAM_FILE is different contexts which expect different results. Ideally, PARAM_FILE should be used when we get a file name from a $_GET parameter and want to ensure that it is not hiding a path to another file (/home/fred/../../etc/shadow). Also, the problem in allowing more characters from now is the possibility of breaking everything as this logic has been there for a while. I'd give my +1 to the current solution which solves the issue, and would suggest to raise an issue to extend the character set in the File Manager without touching at PARAM_FILE.

            Show
            fred Frédéric Massart added a comment - I agree with you, and as said above we should probably allow any character in the file names. The problem here is that we are using PARAM_FILE is different contexts which expect different results. Ideally, PARAM_FILE should be used when we get a file name from a $_GET parameter and want to ensure that it is not hiding a path to another file (/home/fred/../../etc/shadow). Also, the problem in allowing more characters from now is the possibility of breaking everything as this logic has been there for a while. I'd give my +1 to the current solution which solves the issue, and would suggest to raise an issue to extend the character set in the File Manager without touching at PARAM_FILE.
            Hide
            poltawski Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Small comment, surely in your "wingrandparentdir.txt" tests (2), you should be using backslashes?

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Small comment, surely in your "wingrandparentdir.txt" tests (2), you should be using backslashes?
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks!

            I really hope this won't break old path hashes but for really border-case ones.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks! I really hope this won't break old path hashes but for really border-case ones.
            Hide
            fred Frédéric Massart added a comment -

            Thanks Eloy, you are right, it should have been backslashes in both test scenarios. I have added a commit on top of my branch if you wish to pull that in. Cheers!

            Show
            fred Frédéric Massart added a comment - Thanks Eloy, you are right, it should have been backslashes in both test scenarios. I have added a commit on top of my branch if you wish to pull that in. Cheers!
            Hide
            kuraga Alexander Kurakin added a comment -

            Thanks! Isn't it possible to integrate it 2.4?

            Show
            kuraga Alexander Kurakin added a comment - Thanks! Isn't it possible to integrate it 2.4?
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Fred: I've cherry-picked your 2nd commit (3b72ebe), because something strange has happened with the 1st one. Yesteday, when I integrated this, it was caf16a57e5 but today, your branch shows 115e6e2. Have you rebased or done anything with it? Just to be sure the integrated one is valid.

            Alexander: I'd propose to have this patch working some time (weeks) under master and if we don't find any regression, consider backporting it to 24_STABLE. My main concern is that we may be breaking some border case with stores files and their hashes and my hopes are that it will be detected soon in master.

            Could you please create one separate issue named "Extension of uploaded file changes due to multiple dots in the filename (backport of MDL-29749)" and link both. Then, in some weeks, we can re-evaluate the problem. Of course, in the mean time, if you can be trying the current solution against 2.4 (I bet it will apply clean, tell us otherwise)... and report any problem... that will help for sure.

            Thanks for your support!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Fred: I've cherry-picked your 2nd commit (3b72ebe), because something strange has happened with the 1st one. Yesteday, when I integrated this, it was caf16a57e5 but today, your branch shows 115e6e2. Have you rebased or done anything with it? Just to be sure the integrated one is valid. Alexander: I'd propose to have this patch working some time (weeks) under master and if we don't find any regression, consider backporting it to 24_STABLE. My main concern is that we may be breaking some border case with stores files and their hashes and my hopes are that it will be detected soon in master. Could you please create one separate issue named "Extension of uploaded file changes due to multiple dots in the filename (backport of MDL-29749 )" and link both. Then, in some weeks, we can re-evaluate the problem. Of course, in the mean time, if you can be trying the current solution against 2.4 (I bet it will apply clean, tell us otherwise)... and report any problem... that will help for sure. Thanks for your support!
            Hide
            fred Frédéric Massart added a comment -

            Eloy: Thanks for cherry-picking the small fix. Yes, I had rebased my branch, so no worries you had integrated the correct code! Cheers!

            Show
            fred Frédéric Massart added a comment - Eloy: Thanks for cherry-picking the small fix. Yes, I had rebased my branch, so no worries you had integrated the correct code! Cheers!
            Hide
            phalacee Jason Fowler added a comment -

            Awesome work Fred. Chrome under linux has a flaw where files starting with . lose their first . but that is Chrome's fault, not Moodle's. Tested it under windows too, and it was fine there

            Show
            phalacee Jason Fowler added a comment - Awesome work Fred. Chrome under linux has a flaw where files starting with . lose their first . but that is Chrome's fault, not Moodle's. Tested it under windows too, and it was fine there
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

            Closing as fixed, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                9 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/13