[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 Oliver Keyes oke...@wikimedia.org changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 --- Comment #16 from MZMcBride b...@mzmcbride.com --- What's the status of this bug report? Given that bug 56506 is marked resolved/fixed, I hope that this bug is largely resolved/fixed as well. :-) -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 --- Comment #17 from Oliver Keyes oke...@wikimedia.org --- As I understand it, done assuming all of the patches are reviewed, but I'd like to keep it open until I can poke the devs on Monday. -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 --- Comment #15 from Erik Bernhardson ebernhard...@wikimedia.org --- Escape $options in RevisionStorage:findInternal - https://gerrit.wikimedia.org/r/100521 Handle username suppression and renames - For this one we took a different path, we removed all usernames from the flow database and intead query the wiki's user and ipblocks tables directly - https://gerrit.wikimedia.org/r/99789 -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 --- Comment #12 from Chris Steipp cste...@wikimedia.org --- Ok, I've finished reviewing all of the codes, so this should be the last of the issues: includes/View/PostActionMenu.php * Document getAction()'s $content is unescaped includes/View/History/HistoryRenderer.php * Document that keys of getTimespans are passed raw into html output templates/history-line.html.php * please escape $class templates/post.html.php * document that the return of AbstractRevision::getModerationState() is written to raw html, or escape it here * exploitable xss: escape usernames when used in attribute * double quote all attributes (htmlspecialchars does not escape ') includes/View/Post.php * editPostButton(), hidePostButton, deletePostButton, etc, use escaped() instead of plain(), to comply with assumptions of PostActionMenu::getButton() templates/topic.html.php * double quote attributes * document AbstractRevision::getModerationState() is written to raw html, or escape it here * use escaped() messages in call to getButton() * echo wfMessage( 'flow-topic-comments', $comments )-text(); should use escaped() In general for the templates, it would be great if you all could standardize where the escaping for the items written into the template are escaped. There's a mix of escaping parameter before you render the template, in the template, and relying on functions called from within the templates to produce correct html for the context. That makes it really hard to catch errors. -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 --- Comment #13 from Chris Steipp cste...@wikimedia.org --- (In reply to comment #9) * Usernames containing a ' - Could you provide more details on where this causes issues? I ran through the various pages and actions and don't see anything yet, will be looking more thoroughly. I found at least one place (in post.html.php) where you were putting the username into a '-quoted attribute. There were a couple places on http://ee-flow.wmflabs.org/wiki/User_talk:CSTest where I replied to a message by a user with a ' in the name, and the generated user link left off everything after the '. Looks like this is fixed now. -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 --- Comment #14 from Erik Bernhardson ebernhard...@wikimedia.org --- includes/Model/UUID * removed debugging backtrace in https://gerrit.wikimedia.org/r/99285 includes/Repository/SelectQueryBuilder * removed unused class in https://gerrit.wikimedia.org/r/99282 includes/Data/BoardHistoryStorage * share query sanitization with BasicDbStorage https://gerrit.wikimedia.org/r/99035 includes/Data/RevisionStorage * RevisionStorage::findInternal $attributes, RevisionStorage::insert $rev, RevisionStorage::insertRelated $tree and HeadeRevisionStorage::insertRelated -validate query conditions - https://gerrit.wikimedia.org/r/99035 * $options in RevisionStorage:findInternal was missed, patch is not yet in gerrit but is coming soon * RevisionStorage::findMostRecent - was broken, $keys replaced with static value (from self::relatedPk method which returns static string in all implementations) - https://gerrit.wikimedia.org/r/99559 includes/View/PostActionMenu * documented getAction()'s $content is unescaped - https://gerrit.wikimedia.org/r/99561 includes/View/History/HistoryRenderer * docuemnted getTimespans return values as raw html - https://gerrit.wikimedia.org/r/99562 templates/history-line.html.php * escaped $class - https://gerrit.wikimedia.org/r/99293 templates/post.html.php * escaped $post-getModerationState() - https://gerrit.wikimedia.org/r/99564 * double quote all html attributes - https://gerrit.wikimedia.org/r/99573 * exploitable xss - https://gerrit.wikimedia.org/r/99287 includes/View/Post * use escaped() in editPostButton, hidePostButton, deletePostButton, etc instead of plain() to comply with (now) documented assumptions of PostActionMenu::getButton - https://gerrit.wikimedia.org/r/99561 templates/topic.html.php * double quote all attributes - https://gerrit.wikimedia.org/r/99573 * escape AbstractRevision::getModerationState() - https://gerrit.wikimedia.org/r/99564 and https://gerrit.wikimedia.org/r/99287 * escape calls to PostActionMenu::getButton - https://gerrit.wikimedia.org/r/99561 * escape flow-topic-comments message - https://gerrit.wikimedia.org/r/99585 -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 --- Comment #7 from Erik Bernhardson ebernhard...@wikimedia.org --- Hooks.php line 234 - addressed in https://gerrit.wikimedia.org/r/99019 The board-history is also now working again, you shoulsd be able to run fuzz testing. Thanks for digging through this stuff, we will be addressing these other issues shortly. -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 --- Comment #8 from Chris Steipp cste...@wikimedia.org --- (In reply to comment #5) * Different users are getting the same token value This was unrelated -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 --- Comment #9 from Erik Bernhardson ebernhard...@wikimedia.org --- * includes/ParsoidUtils.php disable external entities - https://gerrit.wikimedia.org/r/99164 * includes/Templating.php - display of suppressed revision - https://gerrit.wikimedia.org/r/99166 * Username suppression - working on this, it will be a day or two to complete as we display the names in a variety of placs. * Usernames containing a ' - Could you provide more details on where this causes issues? I ran through the various pages and actions and don't see anything yet, will be looking more thoroughly. * RecentChanges/Formatter - escaping link text - https://gerrit.wikimedia.org/r/99170 Looking over the code involved, we are going to be holding off on the deployment until either tomorrow or next week, probably next week. -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 --- Comment #10 from Chris Steipp cste...@wikimedia.org --- includes/Model/UUID.php * only show backtrace if $wgShowExceptionDetails is true includes/Repository/SelectQueryBuilder.php * escape or validate table, field and op in query() includes/Data/BoardHistoryStorage.php * findTopicListHistory(): validate/filter $queries, or address in RevisionStorage includes/Data/RevisionStorage.php * RevisionStorage::findInternal() - need to validate or sanitize $attributes and $options * RevisionStorage::insert() - validate $rev keys * RevisionStorage::findMostRecent() - broken?? $keys is undefined, needs to be validated if it's not static * PostRevisionStorage::insertRelated() - validate $tree keys * HeaderRevisionStorage::insertRelated() - validate $header keys -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 --- Comment #11 from MZMcBride b...@mzmcbride.com --- (In reply to comment #0) The Flow extension deployment to a handful of pages on mediawiki.org is scheduled for Wednesday Dec 4. (In reply to comment #2) [Setting this to high priority during to the Wed Dec 4 schedule.] Re-scheduled for Tuesday, December 10, 2013. -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270
--- Comment #3 from Chris Steipp cste...@wikimedia.org ---
I'm still working through this, but wanted to get these documented so they can
be fixed sooner.
* {{done}} The sql handling really needed extra sanitization (otherwise,
prevention of sqli depended on several classes all correctly sanitizing and
calling the db handler a certain way). This was addressed in gerrit 98759.
* includes/ParsoidUtils.php. Needs to disable external entities in createDOM
* includes/Templating.php, Line 349, a suppressed revision can be displayed by
removing the message. Unlikely it can be maliciously exploited, but not good
code.
--
You are receiving this mail because:
You are on the CC list for the bug.
___
Wikibugs-l mailing list
Wikibugs-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 --- Comment #4 from Chris Steipp cste...@wikimedia.org --- And from our in-person meeting: * Username suppression needs to be checked (check for a block with ipb_deleted/mHideName set to 1) * (not a blocker for deployment) User renaming needs to be handleable by the RenameUser extension -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 --- Comment #5 from Chris Steipp cste...@wikimedia.org --- While doing some blackbox testing, I'm also noticing that * Different users are getting the same token value * Usernames containing a ' cause a lot of problems -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 --- Comment #6 from Chris Steipp cste...@wikimedia.org --- A few more specific issues: Hooks.php * Line 234 - please escape $action in query includes/RecentChanges/Formatter.php * Should use Linker instead of building a's yourself. Not a blocker. * Please use escaped() instead of text() for messages in topicHistoryLink, postHistoryLink, topicLink, postLink; or use Html::element instead of rawElement In general, I'm working on reviewing the templates, although the structure makes them very difficult to review. I'm probably not going to be able to complete the code review by tomorrow. I've been doing some testing on the frontend, I'm happy with the xss filtering for the page itself and recent changes, but I'm not able to use the board-history, either in the labs ee-flow instance, or my local dev which is running both master and the version I'm review from last Friday. If that can be fixed before tomorrow, I'll work on fuzzing it. If not, then I'm assuming the deployment will be held off for it anyway? -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 Andre Klapper aklap...@wikimedia.org changed: What|Removed |Added Priority|Unprioritized |High CC||aklap...@wikimedia.org --- Comment #2 from Andre Klapper aklap...@wikimedia.org --- [Setting this to high priority during to the Wed Dec 4 schedule.] -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 57270] security review of Flow extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=57270 --- Comment #1 from sp...@wikimedia.org --- The WMF core features team tracks this bug on Mingle card https://mingle.corp.wikimedia.org/projects/flow/cards/495, but people from the community are welcome to contribute here and in Gerrit. -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
