Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-11331

Inform admin of incorrect avatar path instead of stripping unexpected parts from destination path

    Details

      Description

      When submitting an avatar the local avatar (gallery) path is stripped of the following unexpected content:

      $destination = str_replace(array('../', '..\\', './', '.\\'), '', $destination);

      This might lead to unexpected behavior. Either correctly handle those paths or inform the admin of an unexpepcted link when submitting the form with the settings.

        Activity

        Hide
        Marc Marc added a comment -

        I guess we could use filesystem::clean_path() right before this check in validate_config_vars() in order to be able to handle paths entered like this?

        Show
        Marc Marc added a comment - I guess we could use filesystem::clean_path() right before this check in validate_config_vars() in order to be able to handle paths entered like this?
        Hide
        Marc Marc added a comment -

        IRC discussion:

        [16:15:40] <nickvergessen> Marc: the idea is to not allow avatars to be outside of phpbb
        [16:15:49] <nickvergessen> otherwise one might be able to set it to root
        [16:15:59] <nickvergessen> and use config.php as avatar or something like that
        [16:17:30] <Marc> you won't be able to use config.php as avatar
        [16:17:35] <Marc> but I get the point of it
        [16:18:43] <Marc> hence why I thought we could use the filesystem clean_path method to make sure any crap like /images/avatars/gallery/../gallery are not causing it to be /images/avatasr/gallery/gallery
        [16:19:03] <Marc> cause that's what would happen right now
        [16:19:24] <nickvergessen> Marc: but it will circumvent the security restriction
        [16:20:23] <Marc> hm, right
        [16:20:31] <Marc> so is this even worth "fixing" then?
        [16:20:39] <nickvergessen> hmm
        [16:21:08] <nickvergessen> I guess it the description tells the admin that ../ is being stripped off its fine?
        [16:22:04] <Marc> nickvergessen: will add that to the description then

        Show
        Marc Marc added a comment - IRC discussion: [16:15:40] <nickvergessen> Marc: the idea is to not allow avatars to be outside of phpbb [16:15:49] <nickvergessen> otherwise one might be able to set it to root [16:15:59] <nickvergessen> and use config.php as avatar or something like that [16:17:30] <Marc> you won't be able to use config.php as avatar [16:17:35] <Marc> but I get the point of it [16:18:43] <Marc> hence why I thought we could use the filesystem clean_path method to make sure any crap like /images/avatars/gallery/../gallery are not causing it to be /images/avatasr/gallery/gallery [16:19:03] <Marc> cause that's what would happen right now [16:19:24] <nickvergessen> Marc: but it will circumvent the security restriction [16:20:23] <Marc> hm, right [16:20:31] <Marc> so is this even worth "fixing" then? [16:20:39] <nickvergessen> hmm [16:21:08] <nickvergessen> I guess it the description tells the admin that ../ is being stripped off its fine? [16:22:04] <Marc> nickvergessen: will add that to the description then

          People

          • Assignee:
            Marc Marc
            Reporter:
            Marc Marc
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development