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

XSS in changed paths when reviewing revision #311

Closed
Sjord opened this issue Jan 2, 2023 · 15 comments
Closed

XSS in changed paths when reviewing revision #311

Sjord opened this issue Jan 2, 2023 · 15 comments
Labels

Comments

@Sjord
Copy link

Sjord commented Jan 2, 2023

Describe the bug

The filename is not properly escaped, and HTML in a filename is rendered in the browser. This is when viewing the changed file in a revision, behind the "View Changeset" link.

Steps to reproduce the behavior

  1. Create a SVN repository with a file in it named like this:
<h1 onmouseover=alert`XSS`>XSS
  1. Visit http://localhost:49152/viewvc/Development?revision=1&view=revision

The path shows XSS in big letters, and when the mouse moves over it a JavaScript popup shows.

Expected behavior
Path shows <h1 onmouseover=alert'XSS'>XSS.

Additional context

Screenshot 2023-01-02 at 21 34 50

This is in the current master version, cf136ba.

@Sjord Sjord added the bug label Jan 2, 2023
futatuki added a commit that referenced this issue Jan 3, 2023
* lib/viewvc.py: (view_revision)
@futatuki
Copy link
Collaborator

futatuki commented Jan 3, 2023

Thank you for the report. It seems older version of viewvc has same problem.
I've submitted a PR #312 against this issue. For 1.2.x and 1.1.x, I'll also make PRs.

futatuki added a commit that referenced this issue Jan 3, 2023
* lib/viewvc.py: (view_revision)
futatuki added a commit that referenced this issue Jan 3, 2023
* lib/viewvc.py: (view_revision)
cmpilato added a commit that referenced this issue Jan 3, 2023
…hangeset

1.2.x: issue #311: HTML escape paths in change set.
cmpilato added a commit that referenced this issue Jan 3, 2023
…hangeset

1.1.x: issue #311: HTML escape paths in change set.
cmpilato added a commit that referenced this issue Jan 3, 2023
@cmpilato
Copy link
Contributor

cmpilato commented Jan 3, 2023

Fixes merged. I'm requesting a CVE to cover this vulnerability.

@Cryptonics1
Copy link

#

@cmpilato
Copy link
Contributor

cmpilato commented Jan 3, 2023

Workaround: Users can edit their ViewVC EZT view templates to manually HTML-escape changed paths during rendering. Locate in your template set's revision.ezt file references to those changed paths, and wrap them with [format "html"] and [end]. For most users, that means that references to [changes.path] will become [format "html"][changes.path][end]. (This workaround should be reverted after upgrading to a patched version of ViewVC, else changed path names will be doubly escaped.)

@Sjord
Copy link
Author

Sjord commented Jan 3, 2023

Many template engines automatically encode all variables. This way, one wouldn't have to remember to encode every variable manually. Would that work for ViewVC?

@cmpilato
Copy link
Contributor

cmpilato commented Jan 3, 2023

Strictly speaking, ViewVC is using a template engine that's maintained elsewhere (EZT - https://github.com/gstein/ezt). In an ideal world, we'd be publishing packages that pulled in ezt as a dependency, but as it stands today we have an old replica of that code living in our codebase. So, we could diverge from what is the semantic upstream (and probably have done so already...). I'm not sure if a global encode-all-variables would introduce problems elsewhere, though -- just haven't done the exploration necessary to make that call.

@Sjord
Copy link
Author

Sjord commented Jan 3, 2023

It seems the ezt in viewvc already has some support for HTML encoding, so perhaps not that many changes are needed.

@carnil
Copy link

carnil commented Jan 3, 2023

CVE-2023-22456 was assigned for this issue.

GHSA-j4mx-f97j-gc5g

@futatuki
Copy link
Collaborator

futatuki commented Jan 4, 2023

I overlooked that copied from path should be also escaped. It was suggested by @jun66j5 on twitter.

futatuki added a commit that referenced this issue Jan 4, 2023
* lib/viewvc.py (view_revision)

found by: Jun Omae <jun66j5{_AT_}gmail.com>
futatuki added a commit that referenced this issue Jan 4, 2023
* lib/viewvc.py (view_revision)

found by: Jun Omae <jun66j5{_AT_}gmail.com>
futatuki added a commit that referenced this issue Jan 4, 2023
* lib/viewvc.py (view_revision)

found by: Jun Omae <jun66j5{_AT_}gmail.com>
futatuki added a commit that referenced this issue Jan 4, 2023
* lib/viewvc.py (view_revision)

found by: Jun Omae <jun66j5{_AT_}gmail.com>
futatuki added a commit that referenced this issue Jan 4, 2023
* lib/viewvc.py (view_revision)

found by: Jun Omae <jun66j5{_AT_}gmail.com>
futatuki added a commit that referenced this issue Jan 4, 2023
* lib/viewvc.py (view_revision)

found by: Jun Omae <jun66j5{_AT_}gmail.com>
@futatuki
Copy link
Collaborator

futatuki commented Jan 4, 2023

I overlooked that copied from path should be also escaped.

... and now fixed. @cmpilato I'm very sorry, but could you make new release again?

@cmpilato
Copy link
Contributor

cmpilato commented Jan 4, 2023

I overlooked that copied from path should be also escaped.

... and now fixed. @cmpilato I'm very sorry, but could you make new release again?

. o O ( now where did I put that "sobbing obnoxiously" emoji? )

Yes, I can do so. It'll have to wait until tomorrow though.

@cmpilato
Copy link
Contributor

cmpilato commented Jan 4, 2023

I've requested another CVE for this new variant.

@cmpilato
Copy link
Contributor

cmpilato commented Jan 4, 2023

For the sake of those cruising this issue in the future, there are two low-severity cross-site scripting (XSS) attack vectors tracked in this one issue. Both require an attacker to have commit access to your Subversion repository, and to use that access to create versioned files with wonky names, and then to convince a user to navigate to the revision view in which said files were added, deleted, or modified. The second vector involves a further step, where the wonky-named file is copied or renamed inside Subversion.

  • CVE-2023-22456 - covers the situation where the name of the changed file itself results in DOM injection. We introduced fixes for this in ViewVC 1.1.29 and ViewVC 1.2.2. Prior versions in those release streams manifest the problem.
  • CVE-2023-22464 - covers the situation where the "copyfrom path" of the changed file results in DOM injection. We introduced fixes for this in ViewVC 1.1.30 and ViewVC 1.2.3. Prior versions in those release streams manifest the problem.

@cmpilato cmpilato closed this as completed Jan 4, 2023
@cmpilato
Copy link
Contributor

cmpilato commented Jan 4, 2023

Thank you, @Sjord and @jun66j5, for reporting these issues, and @futatuki for providing the fixes.

@Sjord
Copy link
Author

Sjord commented Jan 5, 2023

To HTML-encode variables by default in ezt, pass the base_format parameter in lib/viewvc.py:

return ezt.Template(cfg.path(tname), base_format=ezt.FORMAT_HTML)

Of course, without any further changes this now double-encodes all variables.

LeSuisse added a commit to Enalean/tuleap that referenced this issue Jan 5, 2023
…CVE-2023-22464

Upstream issue: viewvc/viewvc#311

For CentOS/RHEL7, we add to our repository the ViewVC 1.1.30 RPM that is currently in
testing on EPEL into our repository until it reaches stable.

To test rebuild the package and deploy it on your dev instance. It
should still work.

Change-Id: I32c0985a752e7bf6f77fe7ff2f891e9ef0444477
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants