diff options
author | Aleksander Machniak <alec@alec.pl> | 2017-11-08 13:01:16 +0300 |
---|---|---|
committer | Aleksander Machniak <alec@alec.pl> | 2017-11-08 13:03:59 +0300 |
commit | c90ad5a97784fb32683b8e3c21d6c95baab6d806 (patch) | |
tree | 5f819b99a1f41a009fecea12635cf6048c82767a /plugins | |
parent | 581fab9d84873697f4555f19d441e8c9e7ab89a3 (diff) |
Fix file disclosure vulnerability caused by insuficient input validation in relation with attachment plugins (#6026)
Diffstat (limited to 'plugins')
-rw-r--r-- | plugins/database_attachments/database_attachments.php | 7 | ||||
-rw-r--r-- | plugins/filesystem_attachments/filesystem_attachments.php | 47 |
2 files changed, 50 insertions, 4 deletions
diff --git a/plugins/database_attachments/database_attachments.php b/plugins/database_attachments/database_attachments.php index 91335d4cf..5e3268d80 100644 --- a/plugins/database_attachments/database_attachments.php +++ b/plugins/database_attachments/database_attachments.php @@ -84,6 +84,8 @@ class database_attachments extends filesystem_attachments if ($args['data'] === false) { return $args; } + + $args['path'] = null; } $data = base64_encode($args['data']); @@ -130,10 +132,13 @@ class database_attachments extends filesystem_attachments $cache = $this->get_cache(); $data = $cache->read($args['id']); - if ($data) { + if ($data !== null && $data !== false) { $args['data'] = base64_decode($data); $args['status'] = true; } + else { + $args['status'] = false; + } return $args; } diff --git a/plugins/filesystem_attachments/filesystem_attachments.php b/plugins/filesystem_attachments/filesystem_attachments.php index 4bca47e29..7501e5533 100644 --- a/plugins/filesystem_attachments/filesystem_attachments.php +++ b/plugins/filesystem_attachments/filesystem_attachments.php @@ -7,12 +7,19 @@ * attachments of messages currently being composed, writing attachments * to disk when drafts with attachments are re-opened and writing * attachments to disk for inline display in current html compositions. + * It also handles uploaded files for other uses, so not only attachments. * * Developers may wish to extend this class when creating attachment * handler plugins: * require_once('plugins/filesystem_attachments/filesystem_attachments.php'); * class myCustom_attachments extends filesystem_attachments * + * Note for developers: It is plugin's responsibility to care about security. + * So, e.g. if the plugin is asked about some file path it should check + * if it's really the storage path of the plugin and not e.g. /etc/passwd. + * It is done by setting 'status' flag on every plugin hook it uses. + * Roundcube core will trust the returned path if status=true. + * * @license GNU GPLv3+ * @author Ziba Scott <ziba@umich.edu> * @author Thomas Bruederli <roundcube@gmail.com> @@ -107,7 +114,7 @@ class filesystem_attachments extends rcube_plugin */ function remove($args) { - $args['status'] = @unlink($args['path']); + $args['status'] = $this->verify_path($args['path']) && @unlink($args['path']); return $args; } @@ -118,7 +125,7 @@ class filesystem_attachments extends rcube_plugin */ function display($args) { - $args['status'] = file_exists($args['path']); + $args['status'] = $this->verify_path($args['path']) && file_exists($args['path']); return $args; } @@ -129,6 +136,10 @@ class filesystem_attachments extends rcube_plugin */ function get($args) { + if (!$this->verify_path($args['path'])) { + $args['path'] = null; + } + return $args; } @@ -147,7 +158,7 @@ class filesystem_attachments extends rcube_plugin } foreach ((array)$files as $filename) { - if(file_exists($filename)) { + if (file_exists($filename)) { unlink($filename); } } @@ -182,4 +193,34 @@ class filesystem_attachments extends rcube_plugin } } } + + /** + * For security we'll always verify the file path stored in session, + * as session entries can be faked in various ways e.g. #6026. + * We allow only files in Roundcube temp dir + */ + protected function verify_path($path) + { + if (empty($path)) { + return false; + } + + $rcmail = rcube::get_instance(); + $temp_dir = $rcmail->config->get('temp_dir'); + $file_path = pathinfo($path, PATHINFO_DIRNAME); + + if ($temp_dir !== $file_path) { + rcube::raise_error(array( + 'code' => 403, + 'file' => __FILE__, + 'line' => __LINE__, + 'message' => sprintf("%s can't read %s (not in temp_dir)", + $rcmail->get_user_name(), substr($path, 0, 512)) + ), true, false); + + return false; + } + + return true; + } } |