Injection: GitLab Code Navigation relies solely on input validation of HTML by untrusted sources

Summary

Code Navigation is a feature that gives us "jump to definition" in files. It is gated by the :code_navigation feature flag. WIP docs are here: !26069 (merged)

While reviewing gitlab-workhorse!492 (diffs, comment 344333731) with @igor.drozdov , I noticed that we're doing input validation of values, escaping them, and outputting them into raw HTML that is then shown directly from the file by the frontend.

This merge request simply internalises code to workhorse that is already running within CI jobs - which is to say, the input validator is itself untrusted. At the moment, CI jobs are generating HTML fragments and packaging them into JSON files in the artifact.zip file we upload to GitLab. With the feature flag on, those fragments are being extracted from the artifact.zip and injected directly into the frontend, leading to XSS, SSRF, etc, vulnerabilities.

Steps to reproduce

• Fork a project with code intelligence enabled
• Modify .gitlab-ci.yml to generate malicious code-navigation data in the artifact.zip file
• Create a merge request
• Send the link to a project member

The frontend gets a link to a .json file inside the artifact.zip from the backend: https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/code_navigation_path.rb#L19 ; the frontend GETs that file, and interpolates the content of some of the items directly into the page, e.g. here: https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/javascripts/code_navigation/components/popover.vue#L86

UNTRUSTED CONTENT

Example Project

Not actually tested, this is dead reckoning at the moment

What is the current bug behavior?

Untrusted HTML, JS, etc, is rendered/executed.

What is the expected correct behavior?

Escaped malicious content, or better yet, nothing at all.

Output of checks

This bug happens on GitLab.com

Possible fixes

The workhorse MR at least moves the processing step to code we control and can (in theory) trust. However, I think we rely entirely on input validation at the moment, which is a worry. There's always a chance people will get in artifact.zip files that haven't been through this step, or they

Instead, we should be able to apply some form of output validation. If we keep the architecture where the artifact.zip contains raw HTML, this is fairly difficult. We could run it through the Banzai pipeline, perhaps?

Alternatively, we could rework this feature so it provides some sort of metadata that is converted to HTML in either the frontend or the backend, so we don't need to inject raw HTML directly into the page.

/cc @igor.drozdov @danielgruesso @gitlab-com/gl-security/appsec

added Category:Code Review Workflow devopscreate groupsource code security typebug labels

changed title from Injection: GitLab Code NAvigation relies solely on input validation of HTML from untrusted code to Injection: GitLab Code Navigation relies solely on input validation of HTML from untrusted sources

added priority3 severity3 labels

I've marked this as ~S3 because Code Navigation isn't a released feature yet, and we've disabled the feature flag on GitLab.com . We'll need to fix this before it's released, but I don't think it needs to form part of a security release.

changed title from Injection: GitLab Code Navigation relies solely on input validation of HTML from untrusted sources to Injection: GitLab Code Navigation relies solely on input validation of HTML by untrusted sources

We could run it through the Banzai pipeline, perhaps?

This would be my preferred option. It is possible that we can sanitize it on the frontend, however I worry that we will end up duplicating what has existed on the backend with a lot more tests etc.

changed the description

I don't mind the banzai approach; we already trust it to filter malicious HTML in issue descriptions, etc, so it's clearly up to the task.

It does imply that we'd replace the raw artifact path we send to the frontend with another URL that processes the JSON file on-the-fly, though.

changed the description

changed the description

changed due date to August 16, 2020

changed the description

marked this issue as related to #202105 (closed)

mentioned in issue #218929 (closed)

changed milestone to %13.3

Tentatively giving it %13.3 so we fall within the due date, but I expect we'd want to work on this much sooner since it blocks using the code navigation feature entirely. cc @danielgruesso on that one :)

@nick.thomas hm, I think I got distracted by the approach we followed on the first iteration and mimicked the highlighting logic we have for GitLab. I think it's simpler and safer to just send code-tokens to frontend and implement the following logic: https://gitlab.com/gitlab-org/gitlab-workhorse/-/blob/master/internal/lsif_transformer/parser/code_hover.go#L43 on the frontend. It'll be probably even simpler because we won't need to manually escape if I understand it correctly

@iamphill do you see obvious drawbacks with this approach? otherwise, I'm giving it %13.1 and I'll investigate tomorrow whether this approach is possible

Lets see, we might be able to easily solve this with sanitize-html that we already have https://gitlab.com/gitlab-org/gitlab/-/blob/master/package.json#L117

Edit: Though I should probably hold off for a little while as we are moving over to dompurify instead.

@nick.thomas @iamphill I've created a couple of merge requests to validate the idea:

• gitlab-workhorse!517 (merged)
• !33479 (merged)

since the issue is not exploitable (due to the feature flag), can we do this issue public, wdyt?

I'm happy with us fixing this in a normal merge request

I definitely prefer the no-html approach to the sanitisation approach

changed milestone to %13.1

added [deprecated] Accepting merge requests label

assigned to @igor.drozdov

mentioned in merge request gitlab-workhorse!517 (merged)

mentioned in merge request !33479 (merged)

removed [deprecated] Accepting merge requests label

made the issue visible to everyone

With !33479 (merged) and gitlab-workhorse!517 (merged) we can close this issue

closed

mentioned in merge request gitlab-shell!386 (merged)

changed the description

added groupcode review label and removed groupsource code label