Closed Bug 966141 Opened 6 years ago Closed 6 years ago

DownloadsAPI.jsm doesn't validate message names (leads to privilege escalation)

Categories

(Firefox OS Graveyard :: General, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.4+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.4 S2 (28feb)
blocking-b2g 1.4+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: pauljt, Assigned: reuben)

References

Details

(Keywords: sec-low, Whiteboard: [systemsfe][adv-main30-])

Attachments

(1 file, 1 obsolete file)

DownloadsAPI.jsm [1] does enforce permissions, but due to the way its handle messages from DownloadsIPC.jsm, I think a child with the downloads permission could execute arbitrary JavaScript in the parent. Not a huge risk, since only settings and system apps have this permission currently, but its a simple fix to make it secure. (Only really an issue if someone manages to compromise a child process which has the download permission)

The issue is in the recieveMessage function:

144   receiveMessage: function(aMessage) {
145     if (!aMessage.target.assertPermission("downloads")) {
146       debug("No 'downloads' permission!");
147       return;
148     }
149 
150     debug("message: " + aMessage.name);
151     // Removing 'Downloads:' and turning first letter to lower case to
152     // build the function name from the message name.
153     let c = aMessage.name[10].toLowerCase();
154     let methodName = c + aMessage.name.substring(11);
155     if (this[methodName] && typeof this[methodName] === "function") {
156       this[methodName](aMessage.data, aMessage.target);
157     } else {
158       debug("Unimplemented method:  " + methodName);
159     }
160   },

In a compromise situation, a compromised child process could send any aMessage.name/.data it wants - ie 

this[methodName](aMessage.data, aMessage.target) 

could be made to call any function in the current scope (I think). Most other APIs I have seen use a switch statement like:
http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1131

This would be safer I think. 

[1] http://mxr.mozilla.org/mozilla-central/source/dom/downloads/src/DownloadsAPI.jsm
Whiteboard: [systemsfe]
(In reply to Paul Theriault [:pauljt] from comment #0)
> This would be safer I think. 

Not to mention it'd make the code easier to read and follow. I'll take this when I have some cycles if no one else does it in the mean time.
blocking-b2g: --- → 1.4+
Target Milestone: --- → 1.4 S2 (28feb)
Assignee: nobody → reuben.bmo
Attachment #8377630 - Flags: review?(fabrice)
Attachment #8377630 - Flags: review?(fabrice) → review+
Attachment #8377630 - Attachment is obsolete: true
Attachment #8378342 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/58b495296672
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [systemsfe] → [systemsfe][adv-main30-]
You need to log in before you can comment on or make changes to this bug.