Persistent XSS in Note objects

How it was found

Summary: Some cache invalidation and project import logic issues enable an attacker to import a project with XSS payloads in places like MR discussions and similar places where a Note object exists.

Description: There are basically 3 issues causing the XSS here: All attributes of Note objects are controllable in project.json, for example note_html and cached_markdown_version.

Now I can control the value of note_html to contain my XSS payload, but the problem is that the value of this field is a CacheMarkdownField, it's regenerated from the value of note during new object creation (when note_object.note_html_invalidated? returns true). The next question is how to trick GitLab that the field does not need to be regenerated.

in app/models/concerns/cache_markdown_field.rb

      define_method(invalidation_method) do
        changed_fields = changed_attributes.keys
        invalidations  = changed_fields & [markdown_field.to_s, *INVALIDATED_BY]
        invalidations.delete(markdown_field.to_s) if changed_fields.include?("#{markdown_field}_html")

        !invalidations.empty? || !cached_html_up_to_date?(markdown_field)
      end

There are 2 checks here (also the last 2 issues): the first one is:

        INVALIDATED_BY = %w[author project].freeze
...
        invalidations  = changed_fields & [markdown_field.to_s, *INVALIDATED_BY]
        invalidations.delete(markdown_field.to_s) if changed_fields.include?("#{markdown_field}_html")

note_object.changed_attributes.keys
=> ["note", "noteable_type", "author_id", "created_at", "updated_at", "project_id", "line_code", "position", "original_position", "note_html", "cached_markdown_version", "change_position", "attachment"]

This check is, unfortunately, voided because

• Neither author nor project is in the changed_attributes list, but author_id and project_id
• note is deleted from invalidations because note_html is also changed So invalidations is empty.

and the other one is:

!cached_html_up_to_date?(markdown_field)

It basically checks whether attribute cached_markdown_version equals to latest_cached_markdown_version This is really interesting, because I found that latest_cached_markdown_version is always 917504 in my GitLab instance (also gitlab.com). Looks like local_version is always 0 for at least Notes in MR.

  def latest_cached_markdown_version
    @latest_cached_markdown_version ||= (CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16) | local_version
  end

  def local_version
    return local_markdown_version if has_attribute?(:local_markdown_version)

    settings = Gitlab::CurrentSettings.current_application_settings

    if settings.respond_to?(:local_markdown_version)
      settings.local_markdown_version
    else
      0
    end
  end

Finally, I could set note_html to the XSS payload, and cached_markdown_version to the magic number to avoid my payload being overwritten by GitLab. :P

Steps To Reproduce:

(Add details for how we can reproduce the issue)

1. Create an export of a project with at least 1 discussion in at least 1 merge request.
2. Modify the project.json, add field note_html and cached_markdown_version

      "notes": [
        {
          "id": 1,
          "note": "interesting note here",
          "note_html": "<img src=\"test\" onerror=\"alert(document.domain)\"></img>html overwritten",
          "cached_markdown_version": 917504,

1. Import the modified project
2. View the only discussion of the imported project.

Supporting Material/References:

Check https://gitlab.com/Nyangawa/xss/merge_requests/1, you should be able to see a pop-up.

Impact

This is a typical persistent XSS issue and the link I mentioned above is accessible publicly, so all GitLab users are vulnerable theoretically.