[CLOSED] RCE in add local applications form ‘ipaddr’ parameter (Critical)

[oti-tech]

Issue by areynold
Monday Sep 09, 2013 at 15:17 GMT
Originally opened as https://github.com/opentechinstitute/luci-commotion-apps/issues/10

---

The Commotion node web interface allows anonymous (unauthenticated)
users of the node to add local application URLs using Application
Portal. Parameters passed to this form are moderately sanitized
against XSS attacks. Some of the parameters (ipaddr and port) are then
used to execute OS command checking if the submitted app is available
using luci.sys.exec:

https://github.com/opentechinstitute/commotion-apps/blob/29f28511ca48cfdb208096c5e2426e06312689e2/lua/luci/controller/commotion/apps_controller.lua#L320-L340

If URL was submitted instead of IP address (that is, if sent value
does not match IP address syntax), protocol part of ipaddr value is
removed and the value is truncated on first : or / character (to
remove port/path). Before that though, initial sanitization routine
encodes the following characters: <>&\r\n” with html_encode helper
function:

https://github.com/opentechinstitute/commotion-apps/blob/29f28511ca48cfdb208096c5e2426e06312689e2/lua/luci/controller/commotion/apps_controller.lua#L292-L299

https://github.com/opentechinstitute/luci-commotion/blob/9acaeb04fe337da6b4059096a74eae7807182994/luasrc/commotion_helpers.lua

Such moderately transformed value is being passed to OS command
execution routine. By manipulating ipaddr parameter attacker can
execute arbitrary OS commands.

No CSRF tokens are needed to submit such a request. Users of the
commotion node could execute arbitrary OS commands with root
privileges on the device unknowingly just by visiting a website with
prepared payload.

Further investigation shows that the similar vulnerability was already
reported as fixed - https://code.commotionwireless.net/issues/548.
However, as demonstrated, new bypasses are still possible, and it’s
recommended to perform the strict input validation (e.g. allowing only
alphanumeric characters plus a chosen known-safe characters, depending
on the context).

Originally reported as WRT-01-001, iSEC-COMMO13-1

SHORT TERM SOLUTION: Increase the context-sensitive input validation for the ipaddr and port POST parameters. For port, return an error in addition to logging one. Only numeric parameters should be allowed for the port, and standard URL characters allowed for the URL. iSEC recommends using a white-list approach.

LONG TERM SOLUTION: Avoid direct calls to high-risk LuCI shell functions such as luci.sys.exec and luci.sys.call. Instead, replace these with a call to a wrapper library which uses a secondary layer of input validation. By centralizing high-risk functions, The Open Technology Institute can help prevent critical security bugs and allow for sensitive code to be well-vetted and audited in a central location.

[oti-tech]

Comment by dismantl
Monday Sep 09, 2013 at 18:04 GMT

---

Were any examples of arbitrary command execution given? I don't see how, currently, this can be done.

The reason I didn't limit field entries to alphanumeric is that I don't know how well this would work with internationalization and translation. Obviously, we can't limit entries to A-Za-z0-9 when a user doesn't even use the same character set. Unless I can get some advice on how to do this in a context-sensitive way, I don't know how to fix this issue.

[oti-tech]

Comment by dismantl
Monday Oct 14, 2013 at 15:17 GMT

---

I will add more input sanitizing for now, but in the long term, I imagine my shell call to nc will be replaced by the use of commotiond's (not-yet-implemented) socket library (with corresponding Lua bindings) to perform a connectivity check.

Does that seem reasonable?