Page MenuHomePhabricator

Restrict editing of Vega spec to a small set of users
Closed, DeclinedPublic

Description

The consensus on the current state of the Graph extension seems to be that it would be irresponsible to leave it enabled in its current form where it's embedded in article wikitext and any editor can make it execute an arbitrary graph specification. Vega (the specification language the extension relies on) wasn't really meant to be safely usable with untrusted, user-generated specification, and there have been a number of XSS vulnerabilities that are easy to exploit when it's used like that. T335450: Decide whether to use vega-interpreter package to avoid runtime eval() doesn't seem to fundamentally change that, and T222807: Sandbox Graph extension into an iframe isn't something we want to trust the safety of Wikimedia users on.

One way forward would be to treat Vega specifications like other dangerous content (Javascript, CSS) and restrict editing it to a small, trusted set of users. More specifically:

  • add some new parameters to the <graph> tag so it can specify a graph template and a graph data source: <graph template="MediaWiki:Piechart.vega" data="commons:Data:GDP.tab" /> (straw proposal, details to be figured out - the data could come from the contents of the tag etc).
  • disable the <graph> tag when not used with those parameters
  • the template parameter needs to point to a MediaWiki namespace page with some unique naming pattern (*.vega? Vega-*.json?); the editing of such pages would be restricted to users with the vega-editor right, which would initially only be available to trusted users (admins? interface admins?)
  • that page would be a graph template: JSON page matching the Vega spec, with some carefully restricted substitution mechanism for plugging in data, labels, formatting etc. from wikitext authored by less trusted users
  • the other parameters would provide the part of the spec to be substituted (data, maybe i18n, maybe things like colors or labels although those all could probably also be provided as data streams)
  • there would be some guidance to vega-editors similar to the one for interface administrators, to ensure they manage Vega specifications carefully and don't include anything that could result in data being executed as code.
Pro
  • This seems relatively easy to keep secure, as long as we trust users with the vega-editor right to know what they are doing (and we already have other user groups with similarly sensitive abilities) - admittedly the part about parameter substitution is very handwavy, but I think it can be figured out.
  • Declaring data in a way that's understood by the MediaWiki parser would be a huge step in a more maintainable direction for the extension; data could be cached and invalidated when needed (right now the graph just makes user-defined API requests for the data from readers' browsers, which is a bit of a scalability nightmare), data sources would play nicely with Special:WhatLinksHere.
  • With the Vega spec being separate from wikitext, it would be easy to provide a dedicated developer experience (syntax highlighting, TemplateSandbox-style functionality for testing changes, maybe using the Vega editor).
  • If we chose to resurrect graphoid (admittedly this is unlikely to happen), it would probably get rid of the most problematic aspects of the MediaWiki -> graphoid -> MediaWiki dependency loop (see discussion in T211881: graphoid: Code stewardship request).
Con
  • It would be harder to gain the ability to edit graph specifications (ie. anyone could change the data in a graph but few people could do fundamental changes to how the graph looks or behaves). I don't think this is a big problem - Vega is not a very user-friendly language so not many people edit these anyway, and they should easily be able to get permission.
  • The specification would be less flexible - right now you can use templates or Lua to make the specification dynamically depend on the parameters of the graph, this would become impossible. Other than the data and maybe a few other predefined modification points, specifications would be static. Not sure if this would significantly impact real-world usage.
  • In the past there was something like this (a dedicated namespace for graph definitions) and it got rolled back: T98365: Add / use a Graph namespace, T124747: Deprecate use of Graph namespaces. Haven't found any discussion on that but maybe there was a good reason they didn't work well?
See also

Event Timeline

About the handwavy part: I'd assume that any necessary input (raw data, i18n strings, labels, colors etc) can be provided as data streams and the Vega specification can assign them the appropriate meaning (I'm not an experienced Vega user and would appreciate feedback/corrections on this), so the only thing we'd need to plug into the Vega templates would be the data. There are two ways to do that:

  • The template could simply omit the data field (or maybe include test / example data), and the server-side logic could replace the field with a new data specification generated from the other template parameters.
  • The data field could use custom data URLs (e.g. parameter://colors), and we could configure the Vega loader to handle such URLs by plugging in the relevant template parameter (probably made available as a JS variable).

We'd probably still want to do T222807: Sandbox Graph extension into an iframe as hardening. In theory it would be compatible with T335450: Decide whether to use vega-interpreter package to avoid runtime eval() as well but I'm not convinced how much value that has.

@Tgr I am a bit surprised that Vega cannot be made safe -- to my knowledge, the primary authors of Vega (especially Jeff) were specifically trying to make it as safe as possible.

If external data is the only problem, I would recommend using the same method as used in Elasticsearch's Kibana (mentioned here) -- and expand it a bit.

Basic idea: Vega pre-parser can dynamically replace url with inlined hardcoded data:

url: { type: "wikitable", page: "Page_on_commons.tab" }

is substituted by the pre-parser with

data: [ <hardcoded inlined data copy/pasted from the wiki tab page> ]

The specification would be less flexible - right now you can use templates or Lua to make the specification dynamically depend on the parameters of the graph, this would become impossible. Other than the data and maybe a few other predefined modification points, specifications would be static. Not sure if this would significantly impact real-world usage.

This combined with directly specified on page simple graphs (i think from the VE plugin) are like 99% of graphs.

I assume this solution for the simple directly on page graphs wouldn't work either as i think (but haven't validated) that most of those are not made by power and this would be really high friction.

A while ago, i made an analysis on how graph was used on wiki - https://www.mediawiki.org/wiki/User:Bawolff/Reflections_on_graphs . Some time has passed, but i would assume things have not significantly changed. The vast majority were lua based.

If this is what we have to do to make graphs safe, i would question if it is worth it given how our users use the graph extension. Maybe vega based graphs just aren't viable.

This is a really interesting proposal. It could open the way to universal templates and modules! Having the Vega templates centralized is a good way to make things easier for all languages.

@Tgr I am a bit surprised that Vega cannot be made safe -- to my knowledge, the primary authors of Vega (especially Jeff) were specifically trying to make it as safe as possible.

If external data is the only problem

This isn't related to external data (which is problematic in terms of performance, but that's a much less pressing matter). I appreciate the efforts of the Vega authors, but Vega nevertheless had multiple XSS issues and it's hard to be confident it won't have more in the future. It just operates in a domain (transforming very flexible and powerful specifications to executable code) where XSS vulnerabilities can happen easily. I think it's understandable that the authors don't spend a ton of extra effort to make sure such vulnerabilities 100% never happen - they aren't a big risk with how Vega is commonly used. I don't think there are many other sites which throw untrusted user-generated specifications at Vega while standing to lose a lot in the case of XSS. It's an atypical usage pattern and we should abandon it.

The specification would be less flexible - right now you can use templates or Lua to make the specification dynamically depend on the parameters of the graph, this would become impossible. Other than the data and maybe a few other predefined modification points, specifications would be static. Not sure if this would significantly impact real-world usage.

This combined with directly specified on page simple graphs (i think from the VE plugin) are like 99% of graphs.

Are they? Something like {{Map with marks}} seems entirely compatible with a specification JSON template approach to me. (Migrating existing graph templates would certainly be unpleasant work, though.)

Would the recent issues have occurred if graphs were still server-side rendered? XSS doesn't sound like the kind of issue that presents itself in that scenario.

(I realize that path loses interactivity but gains some semblance of visibility for no-JS environments, from a feature perspective.)

Are they? Something like {{Map with marks}} seems entirely compatible with a specification JSON template approach to me. (Migrating existing graph templates would certainly be unpleasant work, though.)

Good point. In my head i was making the assumption that just because people didn't do that option in the past, they wouldn't use it in the future if they had to, but that is a bad assumption.

Would the recent issues have occurred if graphs were still server-side rendered? XSS doesn't sound like the kind of issue that presents itself in that scenario.

(I realize that path loses interactivity but gains some semblance of visibility for no-JS environments, from a feature perspective.)

[Speaking generally, i am not familiar with the specific details of the issue at hand]

Historically graphoid was used only for static graphs and initial previews. Interactive graphs still rendered on client side, so you would have to be willing to give that up.

Second - an xss vuln on the client side generally turns into an RCE vuln (much worse than xss) on the server side (with node.js). There are different options with isolation and sandboxing though on the server side.

[Speaking generally, i am not familiar with the specific details of the issue at hand]

I also am not familiar, but

Second - an xss vuln on the client side generally turns into an RCE vuln (much worse than xss) on the server side (with node.js). There are different options with isolation and sandboxing though on the server side.

is a good comment for my background understanding regardless of whether it's relevant to the specific situation of the extension before the SSR got removed. I had a sneaking suspicion RCE was possible in that kind of case but hadn't connected the dots to what it would be called.

I instead propose to introduce introduce a vega-spec sanitizer so only some safe specs are whitelisted.

Previously I have proposed T334953: Introduce an SVG Sanitizer, though this is for SVG in general.

And maybe some tools to sanitize user input form parameters (e.g. disallow characters that would break strings). Maybe some Lua utils?

For example this should only set a title: "title": "{{{y-axis|Population}}}".

I instead propose to introduce introduce a vega-spec sanitizer so only some safe specs are whitelisted.

This would essentially have to sanitize (or really just prohibit) any feature that used vega-expression in any way. So we'd have to entirely disable signals support and potentially many other features, as vega expressions are used throughout many parts of the vega codebase.

Restrict editing of Vega spec to a small set of users

I'm not sure this is a viable route either. It sounds way too complex to me and its not like interface admins being a separate group is really encouraging authorship and maintenance, its a necessary evil at best.

Maybe the problem is that WMF sites and their userdata are simply too sensitive, and the logical conclusion for content like this is to avoid them ?

  • There could be a toolforge-like tool alla VegaEditor on a separate domain to author your graph, where you don't/can't have access to WMF session information or anything
  • Allow ppl to ingest data from whitelisted public WMF locations
  • Download an SVG and a spec file
  • Manually upload both to Commons
  • Do a simple SVG transclusion to insert the graph.
  • If you want to edit, copy paste the spec file back into the editor.

That seems much simpler. You'd loose interactivity of graphs, but let's be honest... the interactivity-part has been an 8 year long nightmare. Maybe its time to put that to bed and accept defeat. I'm sure it would piss off many people, but the entire organization is over committed, choices unfortunately have to be made.

Most graphs on wiki are simple bar/pie/line charts. These could be produced quite easily using even a language like Lua. Since at the current point we have already broken with the old and present on-wiki syntax, wouldn't it be more feasible to incorporate another (less complex) library into the Graph extension (or, at worst; develop one from scratch)?

We're now at a point when none of the existing graphs is displayed and every of them uses an obsolete syntax that's no longer supported (either directly through <graph> tag or indirectly, using a template). That means, regardless of the exact future steps, every graph invocation will have to be cared of.

Having done a large amount of both script development and map-making that relies on {{Map with marks}} I remain in awe of its ability to add huge amount of content to the Open Street Maps, above and beyond anything available in Mapframe etc. Adding a few text labels alongside dots has probably been its main use, but it has also produced (to my mind) high quality, editable mapping additions to pages which it would be a real shame to lose. For the maps, interactivity has never been available, and until recently was just a bitmap on the page, so returning to that (in any form, but especially at a higher resolution) would be a huge plus compared to facing a total loss. In the meantime I may have a go at putting a stop-gap mapframe (plus explanation?) to show the base-map on the {{OSM Location map}} pages where the graph version would normally go, so it is at least not just a 'fail' message, while we see what may be possible.

That seems much simpler. You'd loose interactivity of graphs, but let's be honest... the interactivity-part has been an 8 year long nightmare. Maybe its time to put that to bed and accept defeat. I'm sure it would piss off many people, but the entire organization is over committed, choices unfortunately have to be made.

I would also add - it would be different if this was a widely used feature. However graphs never really got that much use relatively speaking [edit: i should clarify i only checked enwiki mainspace]. Its been 8 years, they are only used on a small number of articles, they are definitely not used to their full potential instead having a few simple types that are reused. The maintenance burden of graphs extension seems much higher than most extensions. Maybe its simply time to consider it an experiment, that while a good effort, never really succeded in its vision. Time to figure out lessons learned and move on to something else.

Unrelatedly to that though - if we want to keep interactive graphs i think browser level sandboxing (iframe) is probably the only viable way forward (if we dont care about interactivity then server generated images w/ appropriate sandboxing also sounds viable). Trying to blacklist bad features of vega would be a nightmare. Having trusted users is a bit of a band-aid - it is very high friction and admins will probably just copy from untrusted sources anyways.

If we discuss usage, here are my two pennies. In ruwiki, interactive Lua-based graphs are used in more than 26000 articles about settlements and administrative units through https://ru.wikipedia.org/wiki/Module:Statistical (also, more than 8000 on ukwiki, etc.). The Vega template is already rewritten for Vega 5 and can be extracted easily, but the data is processed dynamically, and it is quite hard to change that (although hopefully possible, in principle). Interactivity is highly desirable.

There are two potential replacements for wikis. I just replaced graphs in some demographic articles on plwiki. Maybe this will help some other wiki.

  • Bar charts can be replaced with the timeline. Had one of ours migrated here for demography.
  • Simple pie charts can be done with CSS. Two value pie implemented here: Wykres Smooth Pie 2. You can do multiple values with conic-gradient, but the lines are less smooth (no antialiasing for no apparent reason).

True, EasyTimeline is more reliable and stable, but it is also relying on a software, ploticus, that was last released 10 years ago. Last time the servers where upgraded, ploticus was compiled specifically, and that is not something WMF does regularly. Apparantly the author of ploticus moved on to datadraw, another non-interactive graph software. That one is in python, server side and outputs SVGs. IMO a change like that would still need to happen.

@Nux can https://www.mediawiki.org/wiki/Template:Graph:PageViews be replaced with the Timeline on something other? It is very high use case in ruwiki, pageviews graphs on talk pages.

Once an SVG sanitizer is introduced, T66460: Dynamically generate files with Scribunto/T334372: Add support for inline SVG and Lua-generated SVG chart may be an alternative to Graph.

There are two potential replacements for wikis. I just replaced graphs in some demographic articles on plwiki. Maybe this will help some other wiki.

  • Bar charts can be replaced with the timeline. Had one of ours migrated here for demography.
  • Simple pie charts can be done with CSS. Two value pie implemented here: Wykres Smooth Pie 2. You can do multiple values with conic-gradient, but the lines are less smooth (no antialiasing for no apparent reason).

This is extremely helpful, I was able to use Timeline to create the bar graphs for the pages I maintain. Thank you!

@Nux can https://www.mediawiki.org/wiki/Template:Graph:PageViews be replaced with the Timeline on something other? It is very high use case in ruwiki, pageviews graphs on talk pages.

Not directly, no. Timeline is static, so no API calls...

May I propose disabling editing graphs (to mitigate the potential risk of someone exploiting this vulnerability, now that it's been discovered), and then leaving it up to the user if they want to view a potentially unsafe graph from before this vulnerability (which tbh I know nothing about) was discovered?

After all, a "lazy" user is just as likely to visit an unsafe site on Google, are they not?

EDIT: apologies, I meant to post this on T334940

The main options which seem to be proposed are:

  • Treat vega as dangerous content, like gadgets or sitewide JS, with limits on how it can be edited/used. That's this task (T336595), and there are a number of different mechanisms that could be employed.
  • Sanitize vega input, as @sbassett suggests. I've written a JavaScript bytecode compiler/interpreter before, so I'm probably best qualified to tackle this if it is the approach taken, but the variety of novel XSS mechanisms make me a little dubious at coming up with something really bullet-proof.
  • Sandbox vega in an iframe, possible in addition to other suggestions.
  • Sandbox vega as server-side content. This would give up interactivity.
  • A varient of the above would use vega to generate SVG output. *In theory* this could allow interactivity, expressed as an interactive SVG, and then security vega reduces to T334953: Introduce an SVG Sanitizer. But IMO that seems like a "now we have two problems" sort of result.

The main reason i would argue for iframe sandboxing is that it ties it to the browser same origin policy.

Custom sanitization of complex formats is extremely tricky to get right (after all,vega failing at it is how we landed in this situation). In comparison, there are tons of people looking for ways to bypass browser SOP, and those types of bugs tend to have big payouts, so there is reasonable assurance it is very difficult to bypass iframe based sandboxing.

Aside the vega and security problems, would there another possibility to call and show directly the result of a wikidata sparql query, for instance, https://w.wiki/6i7n inside a wikiarticle?

Aside the vega and security problems, would there another possibility to call and show directly the result of a wikidata sparql query, for instance, https://w.wiki/6i7n inside a wikiarticle?

Lets keep this task on topic please. Proposals for other things can be discussed elsewhere.

Some of the solutions proposed here, especially the main task description, are really clever and would solve our current problem. But I side with @Msz2001 in thinking that our actual requirements are much simpler than what <graph> tries to do. So it seems to me supporting <graph> or fixing it in any way is more effort than it's worth.

My proposal is to work together on a new version of the Graph extension. We can start with a simple set of charts: bar, line, pie, and world/country maps. I have to migrate Wikistats 2 from an old UI library to Codex. While I do that, I can implement these charts into Codex as reusable components. We can then build the new Graph extension using those. In the meantime, we can figure out how to manage and serve data so it's easy to work with and easy to reference from a graph.

Would anyone be interested in collaborating? I can make tasks and project manage.

Well, it's not true that we are supporting very basic things. In Basque Wikipedia we are showing climate graphs with graph extension. Upgrading to Vega5 would let other interactive features, that is something really needed.

Well, it's not true that we are supporting very basic things. In Basque Wikipedia we are showing climate graphs with graph extension. Upgrading to Vega5 would let other interactive features, that is something really needed.

Unfortunately it will not be a simple upgrade path to Vega 5, as we had originally hoped. Much re-work would be required just to get it to a reasonable security posture for Wikimedia production, likely involving an overhaul of the allowed features and means of use. Currently, we would not be able to support basic users having direct access to edit Vega JSON data, at least not anything like what was previously allowed.

Those are two different discussions: one is who will be able to create graphs and how they will be accessed. The other one is if having those is relevant or not. The first discussion is about security, the second one is about product.

I don't have enough information to talk about the first one, but the solution of centralizing those seems not only good, also a good path for global templates and modules.

I have an opinion on the second one, as we are using Vega graphs massively and the possibilites of them (i.e. interactivity, using commons data...) makes it desirable.

My proposal is to work together on a new version of the Graph extension. We can start with a simple set of charts: bar, line, pie, and world/country maps. I have to migrate Wikistats 2 from an old UI library to Codex.

Apart from Codex, that is the same thing as Vega-Lite.

I think Vega-Lite could replace `<graph template="MediaWiki:Piechart.vega">` and make it unnecessary. It already defines graph types, and a user can not really change those that much. I think the graph template suggests an Vega-Lite change, without mentioning Vega-Lite.

Rights:
https://quarry.wmcloud.org/query/68347

the editing of such pages would be restricted to users with the vega-editor right, which would initially only be available to trusted users (admins? interface admins?)

This enwiki query shows that interface-admin is in the 4th bottom spot when it comes to edits to templates with graphs and templates that transclude graphs. I have no faith with those numbers that interface-admins can deal with an serious issue in an responsive manner, there is just not enough overlap between Vega knowledge and people with that right.

Limiting Vega to extendedconfirmed would cause the minimum amount of disruption. Extendedconfirmed limitation would be extendedconfirmed where it exists, and admins elsewhere. Limiting to admins is much more preferable than interface admins. Limiting Vega to extendedconfirmed would mean dusting off the idea of an Vega namespace.

I think Vega-Lite could replace `<graph template="MediaWiki:Piechart.vega">` and make it unnecessary. It already defines graph types, and a user can not really change those that much. I think the graph template suggests an Vega-Lite change, without mentioning Vega-Lite.

Vega-Lite would also not pass a security review at this time.

Vega-Lite would also not pass a security review at this time.

Not that my opinion holds any weight, but In that case, my opinion is to sunset Vega, upgrade EasyTimeline to datadraw (made by the same author as ploticus using similar syntax) (or keep porting Ploticus to new debian releases), and close T137291 as invalid. After all regarding EasyTimeline vs. Vega:

The fact that there has been close to no issues in 20 years, where Graph has had many issues even before the current one.

We'll take a different approach.