Closed
Bug 143459
Opened 22 years ago
Closed 22 years ago
Preference values containing newlines not properly escaped
Categories
(Core :: Preferences: Backend, defect)
Core
Preferences: Backend
Tracking
()
VERIFIED
FIXED
mozilla1.1beta
People
(Reporter: shaver, Assigned: dveditz)
References
Details
(Whiteboard: [adt1 rtm] [ETA 7/15] needs branch check-in)
Attachments
(3 files)
79 bytes,
text/html
|
Details | |
2.47 KB,
patch
|
bnesse
:
review+
alecf
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
1.48 KB,
patch
|
dveditz
:
review+
alecf
:
superreview+
|
Details | Diff | Splinter Review |
From bug 142695: Well, they try. If I have a profile named |thing"thing| in a directory called |foo\nbar| (where \n is a newline), then I get this in my prefs.js: user_pref("browser.cache.disk.parent_directory", "/home/shaver/test/foo\ bar/thing\"thing/5h10p4dc.slt"); So, it does try to escape things (newlines and quotation marks), but you can't just escape newlines that way in JS. Unfortunately, the improperly-escaped newlines lead, as seen in 142695 -- to busted pref-loading. I'll see if I can fix this quickly so we can hit RC3 with it.
Comment 1•22 years ago
|
||
shaver: Can we use URL-encoding for both pref keys and prefs values to kill this issue forever ?
Comment 2•22 years ago
|
||
*** Bug 144621 has been marked as a duplicate of this bug. ***
Comment 3•22 years ago
|
||
This web page causes us to write out the browser.history.last_page_visited pref to the profile with a newline embedded in the value. This makes no prefs coming after "browser.history" in alphabetical order (ie most of them) be loaded. I have also filed bug 157014 on history regarding this issue.
Comment 4•22 years ago
|
||
raising severity, since this is certainly dataloss for the typical user -- they lose their prefs.js and hence access to their mail.
Severity: major → critical
Assignee | ||
Comment 5•22 years ago
|
||
now that this bug includes a demonstration exploit that anyone can use to attack folks (including via mail) I think we need to close it. Sorry. Since it's still marked NEW I'm assuming Brian's not working on it, so I'll take it.
Assignee: bnesse → dveditz
Group: security?
Keywords: mozilla1.1,
nsbeta1+
Whiteboard: [adt1 rtm]
Target Milestone: --- → mozilla1.1beta
Updated•22 years ago
|
Assignee | ||
Updated•22 years ago
|
Whiteboard: [adt1 rtm] [ETA Needed] → [adt1 rtm] [ETA 7/15]
Assignee | ||
Comment 6•22 years ago
|
||
Assignee | ||
Comment 7•22 years ago
|
||
alecf and bnesse: could you look this over with an eye toward any future plans to store prefs in a format other than js. There are hints that some prefs were converted to utf8, but the scriptable frozen interfaces use "string" and the js is currently compiled as Latin-1 not utf8.
Comment 8•22 years ago
|
||
The complex string values are converted from UCS2 to UTF8 for storage and back again when loaded... is that what you're thinking of? In bug 142695 I was under the impression that both Blizzard (comment 15) and Shaver (comment 21) had advocated *not* escaping the PrefKey value, as one should not be using user supplied data for pref keys. If we are going to escape the key, I think we should do it in pref_HashPref when it is created... not everytime we write it out...
Comment 9•22 years ago
|
||
Comment on attachment 91252 [details] [diff] [review] escape \n correctly and add escape for \r sr=alecf
Attachment #91252 -
Flags: superreview+
Comment 10•22 years ago
|
||
sorry, sr='ed this before I read the earlier comments.. in any case, the "string" aspect of prefs is intentional: When you call GetCharPref you should be getting an ASCII string back. If you are expecting a UTF8 value, then you should be using GetComplexValue() (This is similar to how you should call GetIntPref() if you are expecting the pref to be an integer value..) So the simple answer is: prefs were not converted to UTF8.
Comment 11•22 years ago
|
||
Thinking some more about this... following my previous comment to it's logical end, we should escape the string in pref_SetValue when it is entered. The problem with both of these is this... If we do the escaping when we create the preference key/value it will translate into a startup performance hit (because everything gets "created" at starup... If we translate on save... we take a hit anytime we save prefs (exiting the Prefs dialog, exiting the application, and during some of the profile migration code... possibly a couple of other places.) I guess the real question is "where do we want to take this hit?"
Comment 12•22 years ago
|
||
no question in my mind: when we write it to disk. better to take a hit on shutdown + an action that may or may not happen (i.e. opening/closing the prefs window) than on an action that is guaranteed to happen (i.e. starting up the product)
Comment 13•22 years ago
|
||
Comment on attachment 91252 [details] [diff] [review] escape \n correctly and add escape for \r Ok, alec and I have discussed this... check it in as is. r=bnesse.
Attachment #91252 -
Flags: review+
Assignee | ||
Comment 14•22 years ago
|
||
while I agree that pref users /should/ be creating valid pref keys, I think it's bad policy for the pref module to trust them to do so. We can fix prefs once or we have to trust every pref-using component (including 3rd party add-ons) not to make the same mistake, and it's an easy naive mistake to make.
Status: NEW → ASSIGNED
Keywords: adt1.0.1
Comment 15•22 years ago
|
||
Comment on attachment 91252 [details] [diff] [review] escape \n correctly and add escape for \r a=asa (on behalf of drivers) for checkin to the 1.1 trunk.
Attachment #91252 -
Flags: approval+
Comment 16•22 years ago
|
||
This looks good from a security standpoint. r=mstoltz.
Comment 17•22 years ago
|
||
Does this also fix bug 157014? Is that one a dup of this?
Comment 18•22 years ago
|
||
This fixes the "exploit" problem in that bug (or should). The other bug is still valid -- history should be storing a complex value for the URL, not an ascii string (which cannot represent some URLs).
Assignee | ||
Comment 19•22 years ago
|
||
The remaining aspects of bug 157014 are all in your head, Boris. The history guys are going to test the example and then close it as fixed, wfm, or dupe. It's worth leaving open, but you'll need to beef up your description of what needs changing. Fix checked into the trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Whiteboard: [adt1 rtm] [ETA 7/15] → [adt1 rtm] [ETA 7/15] needs branch check-in
Comment 20•22 years ago
|
||
*** Bug 157014 has been marked as a duplicate of this bug. ***
Comment 21•22 years ago
|
||
rvelasco, can you verify this on the trunk today so we can try to land this.
Comment 22•22 years ago
|
||
working on it as I speak
Comment 23•22 years ago
|
||
can anyone tell me how to create a directory with a newline in it for windows, linux, and mac? I'm trying to use shaver's |foo\nbar| example?
Comment 24•22 years ago
|
||
On linux: mkdir 'foo\ bar' will work in most shells (with the carriage return coming right after the backslash).
Comment 25•22 years ago
|
||
Thanks Boris, your example works for me on linux. The browser is escaping correctly for the "browser.cache.disk.parent_directory" as well as the "browser.history.last_page_visited preference". Using boris's test case the corruption of the prefs.js is no longer visible using todays (20020716) trunk builds. Verified on trunk Linux 2.4.7-10 2002071605 Mac OS 10.1.5 2002071608 Mac OS 9.2.2 2002071608 Windows 2000 2002071608
Status: RESOLVED → VERIFIED
Comment 26•22 years ago
|
||
adding adt1.0.1+. Please get drivers approval before checking into the branch.
Comment 27•22 years ago
|
||
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+
Comment 29•22 years ago
|
||
verified on commercial branch Linux 2.4.7-10 2002071808 Mac OS 10.1.5 2002071805 Mac OS 9.2.2 2002071805 Windows XP 2002071808 Internal testcase used, modified boris's attached html file, as well as boris's suggestion for newline contained in directory example for linux. Testcase: http://jazz/users/rvelasco/publish/bugzilla/143459/bad.html
Keywords: fixed1.0.1 → verified1.0.1
Updated•22 years ago
|
Group: security?
Updated•22 years ago
|
Group: security?
Comment 30•22 years ago
|
||
Assignee | ||
Comment 31•22 years ago
|
||
Comment on attachment 98992 [details] [diff] [review] Patch for 0.9.4 branch - needs review r=dveditz
Attachment #98992 -
Flags: review+
Comment 32•22 years ago
|
||
Comment on attachment 98992 [details] [diff] [review] Patch for 0.9.4 branch - needs review sr=alecf
Attachment #98992 -
Flags: superreview+
Updated•22 years ago
|
Group: security
You need to log in
before you can comment on or make changes to this bug.
Description
•