Page MenuHomePhabricator

Security review for oyejorge/less.php
Closed, ResolvedPublic

Description

This library would replace leafo/lessphp for LESS compilation in MediaWiki. Its chief advantage is that its parse tree can be cached separately from the final CSS output and reused.

Library URL: https://github.com/oyejorge/less.php
MediaWiki/Core patch: https://gerrit.wikimedia.org/r/#/c/237730/
MediaWiki/Vendor patch: https://gerrit.wikimedia.org/r/#/c/237751/

Event Timeline

ori created this task.Sep 11 2015, 8:53 PM
ori updated the task description. (Show Details)
ori raised the priority of this task from to Needs Triage.
ori added a project: Security-Team-Reviews.
ori added a subscriber: ori.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 11 2015, 8:53 PM
ori updated the task description. (Show Details)Sep 11 2015, 8:56 PM
ori set Security to None.
ori updated the task description. (Show Details)Sep 11 2015, 9:13 PM
dpatrick triaged this task as Normal priority.Sep 11 2015, 10:11 PM
dpatrick claimed this task.
dpatrick added a project: Security-Team.
dpatrick moved this task from Backlog to Ready on the Security-Team board.
dpatrick moved this task from Ready to In Progress on the Security-Team board.Sep 16 2015, 9:06 PM

General Observations

  • Positive
    • Evaluation of JavaScript via backticks is not supported
    • Extensive test suite
    • Responsive developer(s)
    • No unresolved security issues in current bug list
  • Negative
    • Multiple remarks from less.php users regarding slow performance; possible DoS vector via repeated invocation of exposed endpoint allowing for forced regeneration (Less_Cache::Regen)?

Overall, no issues were observed that need be addressed before use.

Files

./bin/lessc

OK
- Positive
  - file operation functions do not allow for interaction with remote files

./lessc.inc.php

OK

./lib/Less/Autoloader.php

OK

./lib/Less/Cache.php

OK
- Positive
  - no possibility for collision with existing files (unless created by another PHP process running less.php and processing the same css files

./lib/Less/Colors.php

OK

./lib/Less/Configurable.php

OK

./lib/Less/Environment.php

OK

./lib/Less/Exception/Chunk.php

OK

./lib/Less/Exception/Compiler.php

OK

./lib/Less/Exception/Parser.php

OK

./lib/Less/Functions.php

OK
- Positive
  - no apparent vector to embed JavaScript in SVG resulting from svggradient()

./lib/Less/Mime.php

OK

./lib/Less/Output/Mapped.php

OK

./lib/Less/Output.php

OK

./lib/Less/Parser.php

OK
- Positive
  - file operation functions do not allow for interaction with remote files

./lib/Less/SourceMap/Base64VLQ.php

OK

./lib/Less/SourceMap/Generator.php

OK

./lib/Less/Tree/Alpha.php

OK

./lib/Less/Tree/Anonymous.php

OK

./lib/Less/Tree/Assignment.php

OK

./lib/Less/Tree/Attribute.php

OK

./lib/Less/Tree/Call.php

OK

./lib/Less/Tree/Color.php

OK

./lib/Less/Tree/Comment.php

OK

./lib/Less/Tree/Condition.php

OK

./lib/Less/Tree/DefaultFunc.php

OK

./lib/Less/Tree/DetachedRuleset.php

OK

./lib/Less/Tree/Dimension.php

OK

./lib/Less/Tree/Directive.php

OK

./lib/Less/Tree/Element.php

OK

./lib/Less/Tree/Expression.php

OK

./lib/Less/Tree/Extend.php

OK

./lib/Less/Tree/Import.php

OK
- Positive
  - no apparent vectors for directory traversal

./lib/Less/Tree/Javascript.php

OK

./lib/Less/Tree/Keyword.php

OK

./lib/Less/Tree/Media.php

OK

./lib/Less/Tree/Mixin/Call.php

OK

./lib/Less/Tree/Mixin/Definition.php

OK

./lib/Less/Tree/NameValue.php

OK

./lib/Less/Tree/Negative.php

OK

./lib/Less/Tree/Operation.php

OK

./lib/Less/Tree/Paren.php

OK

./lib/Less/Tree/Quoted.php

OK

./lib/Less/Tree/Rule.php

OK

./lib/Less/Tree/Ruleset.php

OK

./lib/Less/Tree/RulesetCall.php

OK

./lib/Less/Tree/Selector.php

OK

./lib/Less/Tree/UnicodeDescriptor.php

OK

./lib/Less/Tree/Unit.php

OK

./lib/Less/Tree/UnitConversions.php

OK

./lib/Less/Tree/Url.php

OK

./lib/Less/Tree/Value.php

OK

./lib/Less/Tree/Variable.php

OK

./lib/Less/Tree.php

OK

./lib/Less/Version.php

OK

./lib/Less/Visitor/extendFinder.php

OK

./lib/Less/Visitor/import.php

OK

./lib/Less/Visitor/joinSelector.php

OK

./lib/Less/Visitor/processExtends.php

OK

./lib/Less/Visitor/toCSS.php

OK

./lib/Less/Visitor.php

OK

./lib/Less/VisitorReplacing.php

OK
dpatrick moved this task from In Progress to Done on the Security-Team board.Sep 25 2015, 7:22 PM
ori added a comment.Sep 26 2015, 9:33 PM

== General Observations ==

  • Positive
    • Evaluation of JavaScript via backticks is not supported
    • Extensive test suite
    • Responsive developer(s)
    • No unresolved security issues in current bug list
  • Negative
    • Multiple remarks from less.php users regarding slow performance; possible DoS vector via repeated invocation of exposed endpoint allowing for forced regeneration (Less_Cache::Regen)?

      Overall, no issues were observed that need be addressed before use.

Thanks. This is impressively thorough and thoughtful.

csteipp closed this task as Resolved.Sep 30 2015, 11:57 PM
csteipp added a subscriber: csteipp.

Nothing to add from me. Thanks!