Mitigate stored XSS through user file upload (GHSL-2024-184)

[texpert]

Thanks GHSL team member @p- for disovering and reporting this!

Stored XSS through user file upload (GHSL-2024-184) vulnerability reported:

A stored cross-site scripting has been found in the image upload functionality that can be used by normal registered users: It is possible to upload a SVG image containing JavaScript and it's also possible to upload a HTML document when the format parameter is manually changed to documents or a string of an unsupported format. If an authenticated user or administrator visits that uploaded image or document malicious JavaScript can be executed on their behalf (e.g. changing or deleting content inside of the CMS.)

This PR fixes the vulnerability by introducing the file_content_unsafe? method, which is scanning the file content for unsafe expressions and patterns in the upload_file method of the CamaleonCms::UploaderHelper.

[owen2345]

Left a comment

[owen2345]

What do you think about this?

def scan_file_for_malicious_content(file_path)
  # Read the file content
  file_content = File.read(file_path)

  # Check for suspicious patterns
  suspicious_patterns = [
    /<script[\s>]/i,  # Script tags
    /on\w+\s*=/i,     # Inline event handlers like onload, onclick, etc.
    /javascript:/i,   # JavaScript in href/src attributes
    /<iframe[\s>]/i,  # Iframes
    /<object[\s>]/i,  # Object tags
    /<embed[\s>]/i,   # Embed tags
    /<base[\s>]/i,    # Base tags (can be used to manipulate URLs)
    /data:/i          # data: URLs (which can include scripts)
  ]

  suspicious_patterns.each do |pattern|
    if file_content =~ pattern
      puts "Potentially malicious content found: #{pattern.inspect}"
      return true
    end
  end
end

[texpert]

Yeah, brilliant, thanks, @owen2345 !

Changed in cad7c8e

One interesting observation is that some jpeg files were failing to load checking for the script with the previous implementation. With the current implementation they are not failing on /<script[\s>]/i, but on the /on\w+\s*=/i instead. Which make me wonder what is my iPhone adding to the photos??

[texpert]

Also, I am thinking of implementing a Content Security Policy, which will allow scripts and other potentially unsafe things loaded only from the assets/ path, making media/ scripts unrenderable, WDYT?

[owen2345]

Content Security Policy

It makes sense, but are you thinking of creating another controller->action to check the security policy first?

Regarding "One interesting observation is that some...", can you change the puts "Potentially..." to print the part that matched the expression? Perhaps we need to improve the expression.

[texpert]

The CSP is set in a Rails initializer and could be fine tuned there. But nope, it isn't at all reliable. Despite being version 3 already, the browsers are still struggling to support it. Especially with SVG, for example - mdn/content#34592

[texpert]

Regarding "One interesting observation is that some...", can you change the puts "Potentially..." to print the part that matched the expression? Perhaps we need to improve the expression.

Yes, the matching part is oNA=������, see the screenshot from RubyMine:

[owen2345]

LGTM