Moodle
  1. Moodle
  2. MDL-21300

Unable to upload of user profile pictures in bulk when username contains special characters

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.7
    • Fix Version/s: 1.9.8, 2.0
    • Component/s: Administration
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE

      Description

      if username contains special characters (extendedusernamechars = true), user picture will not be uploaded.

        Gliffy Diagrams

          Activity

          Hide
          Rossiani Wijaya added a comment -

          removing the validation for clean_param for filename.

          Sam what's your opinion with the solution?

          Rosie

          Show
          Rossiani Wijaya added a comment - removing the validation for clean_param for filename. Sam what's your opinion with the solution? Rosie
          Hide
          Sam Hemelryk added a comment -

          Hi Rossi,
          By the looks of it the clean_param is required to ensure that the files contained in the uploaded zip files don't have malicious file names.
          I certainly think we need to clean/check them, perhaps it can be done in the same way that we process usernames when uploading users, was there a PARAM_ that you used for that?
          I've added Petr as watcher:
          Petr, perhaps you could advise us about the security issues of using a more lenient check?

          Show
          Sam Hemelryk added a comment - Hi Rossi, By the looks of it the clean_param is required to ensure that the files contained in the uploaded zip files don't have malicious file names. I certainly think we need to clean/check them, perhaps it can be done in the same way that we process usernames when uploading users, was there a PARAM_ that you used for that? I've added Petr as watcher: Petr, perhaps you could advise us about the security issues of using a more lenient check?
          Hide
          Rossiani Wijaya added a comment -

          Thanks for the review Sam.

          The reason, i removed it because with 'User attribute to use to match pictures: username' the file name is used to match with the username in DB. if a user was created with extendedusernamechars enabled, then username may contain special characters such as #$~. if we use validation on the file name to eliminate malicious character, the system will skip uploading profile picture for the user.

          I did create PARAM_USERNAME to validate username, however it relies on the current setting for extenedusernamechars. a user maybe created with the extendedchars enable and disable it right after. in this case, uploading profile picture will be skip for that user.

          Note for petr: PARAM_USERNAME has not been committed yet. Please refer to MDL-16919 for reference.

          Show
          Rossiani Wijaya added a comment - Thanks for the review Sam. The reason, i removed it because with 'User attribute to use to match pictures: username' the file name is used to match with the username in DB. if a user was created with extendedusernamechars enabled, then username may contain special characters such as #$~. if we use validation on the file name to eliminate malicious character, the system will skip uploading profile picture for the user. I did create PARAM_USERNAME to validate username, however it relies on the current setting for extenedusernamechars. a user maybe created with the extendedchars enable and disable it right after. in this case, uploading profile picture will be skip for that user. Note for petr: PARAM_USERNAME has not been committed yet. Please refer to MDL-16919 for reference.
          Hide
          Petr Skoda added a comment -

          The file names are cleaned during unzipping, I doubt there is any way to compensate for loss of this information later, so the current code can not detect it. The patch makes sense to me, and the problematic pictures have to end with warning notify(get_string('uploadpicture_usernotfound', 'admin', $a));

          I personally think that people should not use funny characters in usernames, and if they do they should setup some meaningful user IDnumbers and use them instead of usernames when uploading pictures.

          So +1 for commit

          Show
          Petr Skoda added a comment - The file names are cleaned during unzipping, I doubt there is any way to compensate for loss of this information later, so the current code can not detect it. The patch makes sense to me, and the problematic pictures have to end with warning notify(get_string('uploadpicture_usernotfound', 'admin', $a)); I personally think that people should not use funny characters in usernames, and if they do they should setup some meaningful user IDnumbers and use them instead of usernames when uploading pictures. So +1 for commit
          Hide
          Rossiani Wijaya added a comment -

          Petr, Thank you for the input.

          committed patch to 19_stable and HEAD.

          Show
          Rossiani Wijaya added a comment - Petr, Thank you for the input. committed patch to 19_stable and HEAD.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: