Page MenuHomePhabricator

Security review of Ex:JsonConfig/Ex:Kartographer interaction
Closed, ResolvedPublic5 Story Points

Description

Project Information

Has this project been reviewed before?

Kartographer has been reviewed (T121058). However, JsonConfig appears not to have been reviewed. This review was prompted by T163166.

Post-deployment

Reading Infrastructure

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 25 2017, 8:49 PM
dpatrick updated the task description. (Show Details)Apr 25 2017, 8:54 PM
dpatrick updated the task description. (Show Details)Apr 25 2017, 9:00 PM
dpatrick updated the task description. (Show Details)Jun 6 2017, 4:13 PM
debt added a subscriber: debt.
debt moved this task from Backlog to Stalled/Waiting on the Maps-Sprint board.Jun 6 2017, 7:43 PM

Moving to waiting to see if there is anything this team needs to do to help.

Reedy added a subscriber: Reedy.Jul 20 2017, 5:10 PM

Where is/what is the extent of he integration?

All I can obviously see in Kartographer...

				if ( !class_exists( 'JsonConfig\\JCSingleton' ) ) {
					return Status::newFatal( 'kartographer-error-service-name', $object->service );
				}
				$jct = JCSingleton::parseTitle( $object->title, NS_DATA );
				if ( !$jct || JCSingleton::getContentClass( $jct->getConfig()->model ) !==
							  JCMapDataContent::class
				) {
					return Status::newFatal( 'kartographer-error-title', $object->title );
				}
Yurik added a subscriber: Yurik.Jul 20 2017, 7:55 PM

@Reedy yep, that's pretty much all of it. Also, JsonConfig is mostly a refactoring of the original ZeroAccess extension, which was originally implemented ~7? years ago, and later had many changes. It grew so big that at one point i simply split it into 3 extensions - JsonConfig, ZeroPortal, and ZeroBanner.

How is the security review going?

Was it done?

Mholloway changed the task status from Open to Stalled.Oct 29 2018, 4:47 PM
Mholloway updated the task description. (Show Details)Nov 7 2018, 2:39 PM
sbassett claimed this task.Jan 7 2019, 6:56 PM
sbassett set the point value for this task to 5.
sbassett added a subscriber: Bawolff.
sbassett added a comment.EditedFeb 8 2019, 5:02 PM

Update: Sorry for the crazy delays on all of this. I'm officially starting in on this today (2/8).

sbassett added a comment.EditedMar 7 2019, 11:16 PM

Update: Still working on this :) I've run some automated scans/tests against the JsonConfig code and didn't find much beyond basic linter-y issues ($wgParser global and wfLoadExtension entrypoints being deprecated and the like.) Phan-taint-check didn't come up with anything either, which makes sense as there aren't any direct db or shell interactions, other smells like unserialize() or obviously unsafe uses of Html::rawElement().

That said, in evaluating some of the previous known security issues (T163166, T134719) I'd like to focus a bit more on the specific interaction between Kartographer/JsonConfig and areas such as JCUtil::sanitizeRecursive, JCDataContent->renderDescription() and the valueToHtml() functions within the JC*ContentView.php files. Should have a report on any resulting issues by tomorrow or early next week.

Security Review Summary - March 2019
Per T163827#5010048, after running some automated scanning tools and digging into the code of JsonConfig a bit more, specifically how the extension is currently used by Kartographer, I wasn't able to find anything obviously concerning. Given that JsonConfig has been in production for nearly 5 years now, I'm going to assume some level of stability and that the issues which prompted the creation of this task (XSS within Kartographer) have been resolved within those separate tasks. I may do a little more local testing against the extension just for my own curiosity, but for now I'm going to mark this task as resolved.

sbassett closed this task as Resolved.Mar 14 2019, 10:17 PM