WikiHiero/Hierator security review
Closed, InvalidPublic
Actions

Assigned To

Authored By

	MaxSem
	May 12 2015, 8:57 PM

Description

I'm preparing a major facelift to WikiHiero, to make it use images of rendered via the Hierator service, as opposed to the current PNGs of separate hieroglyphs combined through HTML tables. I would appreciate a security review for it before it goes into betalabs/prod.

WikiHiero: change up for review https://gerrit.wikimedia.org/r/#/c/178269/

Uses Hierator for generation of SVG images and their PNG fallbacks that are later stored by RESTBase.
Can be merged before Hierator is deployed because it would still use PHP rendering by default.
Fallbacks necessitate the use of inline CSS, unfortunately.

Hierator: hieroglyphic text rendering service, implemented as a Java servlet. Uses JSesh for actual rendering.

Related Objects
Search...

Status	Assigned	Task
Open	None	T3007 Cartouche ends do not scale to fit the enclosed hieroglyphs
Open	None	T33000 WikiHiero's handling of parentheses is broken
Declined	None	T93787 Switch WikiHiero to Hierator
Invalid	None	T93847 Make Hierator available via RESTBase
Invalid	• csteipp	T98921 WikiHiero/Hierator security review

Event Timeline

MaxSem created this task.May 12 2015, 8:57 PM

MaxSem raised the priority of this task from to Needs Triage.

MaxSem updated the task description. (Show Details)

MaxSem added projects: deprecated-security-team-reviews, WikiHiero.

MaxSem added subscribers: MaxSem, • csteipp.

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 12 2015, 8:57 PM

MaxSem merged a task: T98928: Security review for Hierator.May 12 2015, 10:31 PM

MaxSem added subscribers: • GWicke, • mobrovac, Ricordisamoa.

• GWicke assigned this task to • csteipp.May 12 2015, 10:32 PM

• GWicke set Security to None.

• csteipp added a project: Security-Team.May 13 2015, 5:52 PM

• csteipp moved this task from Incoming to Ready on the Security-Team board.

• mobrovac added a parent task: T93847: Make Hierator available via RESTBase.May 14 2015, 10:43 AM

Liuxinyu970226 subscribed.May 18 2015, 11:59 AM

For https://github.com/wikimedia/mediawiki-services-hierator

I would prefer if HieratorServlet::processRequest() escaped the exception message before outputting it. Since all of the exceptions are currently plain text strings, this isn't exploitable, but if the code is updated to output something attacker controlled in the exception message, xss would be possible.
The service should add all of the security headers that restbase sets (could actually be more strict in many cases). I think the following should cover legitimate uses of the service:
- X-Content-Type-Options = nosniff
- X-Frame-Options: DENY (unless you really need these iframed.. but it looks like they're all included via img tags)
- X-XSS-Protection: 1; mode=block
- Content-Security-Policy/X-Content-Security-Policy/X-WebKit-CSP = default-src 'none'; img-src 'self'; style-src 'unsafe-inline' 'self'; frame-ancestors 'self'