Page MenuHomePhabricator

Buffer overflow in luasandbox php extension (mediawiki/php/luasandbox)
Closed, ResolvedPublic

Description

Patch to fix the issue

The underlying cause in bug 49649 is a buffer overflow in malloced memory in the luasandbox PHP extension. In Lua's C API, the caller is responsible for calling lua_checkstack to expand the Lua's stack as necessary before pushing onto it, and the various lua_push* functions apparently don't do any checking unless you compile with certain debug options activated. The lua_checkstack call isn't being done when processing arguments when calling from PHP to Lua or when returning values from PHP to Lua, so if the existing Lua stack isn't big enough (default is space for 20 values unless some other call already made Lua allocate a bigger stack) it will overflow into the next data structure on the heap.

FYI, an easier reproduction in Scribunto than the one given in bug 49649 is the one liner mw.ustring.codepoint( string.rep( 'x', 1000 ), 1, -1); the limit on the 1000 depends on whether anything else has already increased the Lua stack size. An even simpler (but longer) one-liner is to replace the string.rep call with a literal string of sufficient length. It can also be reproduced from the command line, see the tests in the attached patch.

Deployment process is apparently to update the changelog then have someone with appropriate access (Ops?) run the wmf-build script that's in the mediawiki/php/luasandbox repo. Tim usually does all that, of course, but he's on vacation this week.


Version: unspecified
Severity: normal

attachment patch ignored as obsolete

Details

Reference
bz49705

Related Objects

StatusSubtypeAssignedTask
ResolvedNone

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 1:59 AM
bzimport added a project: Scribunto.
bzimport set Reference to bz49705.
bzimport added a subscriber: Unknown Object (MLST).

Created attachment 12566
Patch to update the changelog

attachment patch2 ignored as obsolete

How will we handle this one in Gerrit? Are we gonna submit it in public with the "fix buffer overflow" comment, but only right before deployment?

(In reply to comment #2)

How will we handle this one in Gerrit? Are we gonna submit it in public with
the "fix buffer overflow" comment, but only right before deployment?

I know this was already answered on IRC, but for the record:

We're handling it like any other security update, if possible: deploy the fix, then put it in Gerrit afterwards. The bug also gets reassigned to the appropriate non-Security product when it's closed. I don't know if there will be a security announcement too or what. Coordinate with Chris on the details and timing of the making-public bits, of course.

i added myself to labs project "packaging", and used instance "php-packaging" since it appears in the wmf-build script from the luasandbox repo.

i applied the patch and build 1.7-1, then manually copied from the labs instance to brewster and imported with reprepro.

since the package is "ensure latest" in puppet, it then got auto-updated on the cluster hosts.

as of this writing i'ts now "ii php-luasandbox 1.7-1 " on all 217 mw* hosts.

(In reply to comment #5)

and http://en.wiktionary.org/wiki/User:Mutante/foo did _not_ become
uneditable.
So that would be good, please confirm.

My tests work as well.

cool, claiming RESOLVED then.

This is not in gerrit yet, do we want to make it public now?

(In reply to comment #8)

This is not in gerrit yet, do we want to make it public now?

I'd say let's ask Chris.

The only other issue is if there are external users we would want to notify. I'm assuming that's not the case here (although Brad, let us know if you think otherwise), so I think we can release-- i.e., put the patch in gerrit and send a note to wikitech-l in case other people want to patch.

It's certainly possible that others have downloaded and installed luasandbox, we mention it on the extension page and people have filed bugs against it specifically.

Created attachment 13158
Rebased patch, and added one more check

It would be nice to get this released sometime soon. In the mean time, I rebased the patch and added an additional luaL_checkstack() call.

attachment diff ignored as obsolete

Created attachment 13159
And here's a version rebased on top of Gerrit change 63565

Also, since Gerrit change 63565 and this have conflicts, here's a version rebased on top of that change in case that gets merged first.

attachment diff2 ignored as obsolete

I'm planning to include this this with 1.21.2, currently scheduled for Aug 27th.

Created attachment 13226
Changelog for 1.7 and 1.8

attachment diff ignored as obsolete

Created attachment 13795
Rebased patch

Attached: