Moodle
  1. Moodle
  2. MDL-29749

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      19266

      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

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

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          Alexander Kurakin added a comment - - edited

          Ok what about to release the patch?

          Show
          Alexander Kurakin added a comment - - edited Ok what about to release the patch?
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Alexander Kurakin added a comment -

          Ok. What's the next?

          Show
          Alexander Kurakin added a comment - Ok. What's the next?
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Frédéric Massart added a comment -

          Awesome, thanks Vadim!

          Show
          Frédéric Massart added a comment - Awesome, thanks Vadim!
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Frédéric Massart added a comment -

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

          Show
          Frédéric Massart added a comment - Raised MDL-37561 to improve file name compatibility when download/export/zipping, etc...
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

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

          Show
          Eloy Lafuente (stronk7) added a comment - Small comment, surely in your "wingrandparentdir.txt" tests (2), you should be using backslashes?
          Hide
          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
          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
          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
          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
          Alexander Kurakin added a comment -

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

          Show
          Alexander Kurakin added a comment - Thanks! Isn't it possible to integrate it 2.4?
          Hide
          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
          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
          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
          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
          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
          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
          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
          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: