search

opentechinstitute / commotion-router

The build system for the OpenWRT-based Commotion firmware.
https://commotionwireless.net
GNU General Public License v3.0
121 stars 43 forks source link

Missing Secure Flag for sysauth Cookie #33

Open areynold opened 10 years ago

areynold commented 10 years ago

FINDING ID: iSEC-COMMO13-5

TARGETS: The lack of a Secure flag on the sysauth administrative session cookie.

DESCRIPTION: The Secure flag, when set by the web application for modern browsers, indicates the session cookie should never be sent via a plaintext HTTP connection. This can offer defense in depth against network attacks when the application or administrative portions of the Commotion router uses HTTPS.

EXPLOIT SCENARIO: An attacker may be able to cause the sysauth cookie to be leaked via a plaintext HTTP request. In one example, an attacker could create a plaintext HTTP link to the Commotion router as a local application icon. If an administrator is authenticated to the site over SSL and visits the application list, the browser will issue the plaintext, non-SSL request and automatically include the admin's current session token. A network attacker may be able to capture this value via network sniffing and perform subsequent actions on the administrator's behalf.

SHORT TERM SOLUTION: Use a documented method within LuCI to set the secure flag if it exists or apply the flag when setting the sysauth cookie via luci.http.header.

LONG TERM SOLUTION: Ensure the default administrative interface uses HTTPS via TLS and does not accept plaintext connections. Review the Commotion web applications and attack surface for standard web application best practices. Consider implementing a regression test to ensure this issue does not resurface in future releases once fixed.

areynold commented 10 years ago

Sysauth cookie seems to be built in dispatcher.lua, line 385. Untested, but seems to be a likely starting point.

Would prefer to submit upstream to luci, but this may need to be a commotion-only fix since openwrt doesn't use https by default.

Can also be set at the theme level by adding <httpCookies requireSSL="true" /> to page header. See commotion-openwrt-theme.

areynold commented 10 years ago

dispatcher.lua code is duplicated in luci-commotion-linux's apps_controller.lua. Pull request opened.

areynold commented 10 years ago

Fixed in luci-commotion 183c11ceba330407a4db7531fb86bf3a89126d7e

areynold commented 10 years ago

This issue was created before the correct repo was identified, and was assigned to commotion-openwrt as a placeholder.

This issue has been addressed in commotion-openwrt by way of opentechinstitute/luci-commotion#28.

Because luci-commotion-linux is a fork of luci, the same issue exists in that code, and is fixed in opentechinstitute/luci-commotion-linux#1.

I am closing this issue and reopening it in luci-commotion as a request to submit a patch to luci. The patch is unlikely to be accepted since openwrt does not run ssl by default, but it's worth trying.

areynold commented 9 years ago

There's a regression in the Commotion 1.1 release. It looks like the luci core libs are being compiled instead of installed as full source, so the patch to dispatcher.lua is being rejected.

Short term fix would be to build the luci core libs as source and check that the patch still applies cleanly.

Long term fix would be to submit an upstream patch