Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stored Cross Site Scripting - (Authenticated) on Opencart Admin Dashboard #7810

Closed
corrupted-brain opened this issue Jan 9, 2020 · 13 comments
Labels
maintenance-branch 3.0.x.x Affects the 3.0.x.x maintenance version

Comments

@corrupted-brain
Copy link

What version of OpenCart are you reporting this for?
Opencart 3.0.3.2
Describe the bug
Stored Cross Site Scripting (XSS) - Authenticated is found in users image upload section in opencart admin panel. Opencart is accepting filenames with arbitrary code in it and not escaping them so the JavaScript get executed. Malicious script in the admin dashboard can be injected permanently and can be used to steal the user’s sensitive information like cookies, keystrokes, account information etc

To Reproduce
Steps to reproduce the behavior:

  1. Go to localhost.com/opencart/admin and login with credentials.

  2. Then navigate to System>Users>Users and click on Action button on top right corner.
    Screenshot from 2020-01-09 09-46-46

  3. Now in image field , click on image and upload a new image. Before this select any image file and rename with this XSS payload "><svg onload=alert("XSS")> and then upload it as new user profile image.

  4. After the upload completes the XSS pop-up executes as shown below and it will gets executed each time someone visits the Image manager section.
    Screenshot from 2020-01-09 09-58-24

Expected behavior
Escaping and sanitation of HTML tags/ Special characters before storing or processing them, so that the code does not executes. Although XSS was strictly filtered on other sections, here we were able to execute it because of filename so filenames, file extensions and headers should be analyzed to prevent XSS and other file upload vulnerabilities.

Screenshots / Screen recordings

opencart stored XSS.zip

Server / Test environment (please complete the following information):

  • Kali Linux 4.19.0
  • PHP version: 7.1.32
  • Apache version: 2.4.41
  • Browser(s) tested with: Mozilla Firefox Latest Build
@straightlight
Copy link
Contributor

straightlight commented Jan 9, 2020

I like this solution for single-dots aside from the fact that XSS vulnerabilities could be used with uploaded filenames: https://stackoverflow.com/a/17625652.

Let's try it and see if you can reproduce the same issue as described above:

In admin/controller/common/filemanager.php file,

find:

$filename = basename(html_entity_decode($file['name'], ENT_QUOTES, 'UTF-8'));

replace with:

$sanitize = array('~([^\w\s\d\.\-_~,;:\[\]\(\)]|[\.]{2,})~', '~XSS~i' );

				$filename = preg_replace($sanitize, '', html_entity_decode($file['name'], ENT_QUOTES, 'UTF-8'));					
				
				$filename = basename($filename);

@corrupted-brain
Copy link
Author

Hi , I used following regex to remove special characters from the filename, allowing dots, hyphen, A-Z, 0-9.

// Sanitize the filename
$filename = basename(html_entity_decode($file['name'], ENT_QUOTES, 'UTF-8'));
//Using regex to filter filename
$filename=preg_replace('/[^A-Za-z0-9\-\.]/','',$filename);
// Validate the filename length
if ((utf8_strlen($filename) < 3) || (utf8_strlen($filename) > 255)) {
$json['error'] = $this->language->get('error_filename');
}

And It worked, special characters were removed and file uploaded as shown below.

Screenshot from 2020-01-10 14-07-25

Initially filename was "><svg onload=alert("XSS_By_Kailash")>.jpg . I hope it has been resolved.

@ADDCreative
Copy link
Contributor

ADDCreative commented Jan 13, 2020

The payload also works if entered as a New Folder name. The real issue is lack of escaping before sending the data to the template at the following lines (for 3.0.3.2, now slightly different in the master branch).

'name' => implode(' ', $name),
'type' => 'directory',
'path' => utf8_substr($image, utf8_strlen(DIR_IMAGE)),

'thumb' => $this->model_tool_image->resize(utf8_substr($image, utf8_strlen(DIR_IMAGE)), 100, 100),
'name' => implode(' ', $name),
'type' => 'image',
'path' => utf8_substr($image, utf8_strlen(DIR_IMAGE)),
'href' => $server . 'image/' . utf8_substr($image, utf8_strlen(DIR_IMAGE))

Any line from the above that uses $name or $image should be wrapped in htmlspecialchars() (or maybe urlencode() in one case), as below. This will also fix other minor issues caused by special characters (such as parent folder being deleted if the sub folder to be deleted starts with a double quote).

htmlspecialchars('name'  => implode(' ', $name), ENT_QUOTES, 'UTF-8'),

There also looks like an attempt to filter in the master branch.

$filename = preg_replace('[/\\?%*:|"<>]', '', basename(html_entity_decode($file['name'], ENT_QUOTES, 'UTF-8')));

$folder = preg_replace('[/\\?%*:|"<>]', '', basename(html_entity_decode($this->request->post['folder'], ENT_QUOTES, 'UTF-8')));

However, the expression is wrong. Maybe it should of been '/[\/\?%*:|"<>*]/'. Even if the expression was correct, it would not help where the payload was already there, delivered via FTP or an extension that uploads images.

@straightlight
Copy link
Contributor

straightlight commented Jan 21, 2020

While the analysis above might be true, the only downside by adding a star lookup at the end, whether it's for FTP or an extension that uploads images, the directory structure name also gets renamed along with the filename by using the master branch and also checks for existing folder names. Any change of state not being identified within the $json['success'] becomes a renamed folder name where the admin is not consent about the returned file name since sprintf is not being used with the text_directory key of the $this->language->get . Therefore, the admin may not realize if the folder name originates from the file manager or by ... another source of origin especially since CHMOD 077 is being implied in the codes.

That being said; to add a sprintf function to notify the admins regarding the new created folder name would be highly recommended.

As for the regular expression, may be the following could be used:

preg_replace('/[^a-zA-Z0-9\_\.\?]/', '', basename(html_entity_decode($this->request->post['x'], ENT_QUOTES, 'UTF-8')));

where 'x' would be the folder and the filename wherever they need be.

Source: https://www.wordfence.com/learn/how-to-prevent-cross-site-scripting-attacks/

@straightlight
Copy link
Contributor

An alternative would be to use filter_var based on the provided URL above.

@ADDCreative
Copy link
Contributor

Yes, the asterisk was superfluous. I should of just said the expression was missing delimiters.

I think the aim of the santise filters are to make the names safe for writing to the file system (thinking Windows reserved characters), not to prevent XSS. You won't find a filter_var filter for that.

@straightlight
Copy link
Contributor

If no filter_var could be found for sanitizing filters, then users relying on the filter_var validations being used with custom fields with the REGEXP case would be all screwed, I presume. Granted, there has been reports regarding odd results regarding the regular expressions being used in there as well. However, workarounds were also provided in their commits accordingly.

@ADDCreative
Copy link
Contributor

There are no filter_var filters to sanitize a filename. The custom fields are using filter_var to validate not sanitize. There is a difference.

@ADDCreative
Copy link
Contributor

@corrupted-brain I see you have closed this issue. Has a patch been added to the master branch?

@corrupted-brain
Copy link
Author

No it's not. Sorry I was unaware of this procedure and closed the issue.

@danielkerr
Copy link
Member

this issue missed my attention.

you are wasting every ones time!!!!!! you clown! how are hackers supposed to upload code when they need the admin access!!!! the big what if! stop wasting peoples time. you should not be giving advice out to anyone! concentrate on your own life because you are not smart enough to be successful as a coder!

@corrupted-brain
Copy link
Author

Sir have you heard about CSRF attacks ? Issues like this can be chained with CSRF. Where a authenticated user can be exploited with a single click and performing request from victims (here admin's) side. I'm not smart to do secure coding but I think I found common coding mistake where basic user input validation is not done. This is not the advice, I get to know about this from OWASP. Sorry for wasting 2 more valuable minutes of your life.

@danielkerr
Copy link
Member

narcissist

@opencart opencart locked and limited conversation to collaborators Dec 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
maintenance-branch 3.0.x.x Affects the 3.0.x.x maintenance version
Projects
None yet
Development

No branches or pull requests

5 participants