Last Comment Bug 143459 - Preference values containing newlines not properly escaped
: Preference values containing newlines not properly escaped
Status: VERIFIED FIXED
[adt1 rtm] [ETA 7/15] needs branch ch...
:
Product: Core
Classification: Components
Component: Preferences: Backend (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla1.1beta
Assigned To: Daniel Veditz [:dveditz]
: Rodney Velasco
:
Mentors:
: 144621 157014 (view as bug list)
Depends on:
Blocks: 143047
  Show dependency treegraph
 
Reported: 2002-05-10 07:17 PDT by Mike Shaver (:shaver -- probably not reading bugmail closely)
Modified: 2002-12-23 10:43 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase -- WARNING, THIS WILL MAKE YOUR prefs.js UNUSABLE (79 bytes, text/html)
2002-07-11 15:57 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
escape \n correctly and add escape for \r (2.47 KB, patch)
2002-07-13 21:10 PDT, Daniel Veditz [:dveditz]
bnesse: review+
alecf: superreview+
asa: approval+
Details | Diff | Splinter Review
Patch for 0.9.4 branch - needs review (1.48 KB, patch)
2002-09-12 18:29 PDT, Mitchell Stoltz (not reading bugmail)
dveditz: review+
alecf: superreview+
Details | Diff | Splinter Review

Description Mike Shaver (:shaver -- probably not reading bugmail closely) 2002-05-10 07:17:39 PDT
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 Roland Mainz 2002-05-10 07:38:48 PDT
shaver:
Can we use URL-encoding for both pref keys and prefs values to kill this issue
forever ?
Comment 2 Brian Nesse (gone) 2002-05-15 09:23:05 PDT
*** Bug 144621 has been marked as a duplicate of this bug. ***
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2002-07-11 15:57:38 PDT
Created attachment 91022 [details]
testcase -- WARNING, THIS WILL MAKE YOUR prefs.js UNUSABLE

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 Boris Zbarsky [:bz] (still a bit busy) 2002-07-11 15:59:44 PDT
raising severity, since this is certainly dataloss for the typical user -- they
lose their prefs.js and hence access to their mail.
Comment 5 Daniel Veditz [:dveditz] 2002-07-11 17:17:40 PDT
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.
Comment 6 Daniel Veditz [:dveditz] 2002-07-13 21:10:47 PDT
Created attachment 91252 [details] [diff] [review]
escape \n correctly and add escape for \r
Comment 7 Daniel Veditz [:dveditz] 2002-07-13 21:14:15 PDT
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 Brian Nesse (gone) 2002-07-15 09:29:32 PDT
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 Alec Flett 2002-07-15 09:36:10 PDT
Comment on attachment 91252 [details] [diff] [review]
escape \n correctly and add escape for \r

sr=alecf
Comment 10 Alec Flett 2002-07-15 09:49:24 PDT
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 Brian Nesse (gone) 2002-07-15 10:40:32 PDT
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 Alec Flett 2002-07-15 10:42:36 PDT
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 Brian Nesse (gone) 2002-07-15 10:57:49 PDT
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.
Comment 14 Daniel Veditz [:dveditz] 2002-07-15 12:11:38 PDT
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.
Comment 15 Asa Dotzler [:asa] 2002-07-15 13:14:32 PDT
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.
Comment 16 Mitchell Stoltz (not reading bugmail) 2002-07-15 15:01:53 PDT
This looks good from a security standpoint. r=mstoltz.
Comment 17 Mitchell Stoltz (not reading bugmail) 2002-07-15 15:54:59 PDT
Does this also fix bug 157014? Is that one a dup of this?
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2002-07-15 16:35:37 PDT
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).
Comment 19 Daniel Veditz [:dveditz] 2002-07-15 19:18:31 PDT
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.
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2002-07-15 19:23:59 PDT
*** Bug 157014 has been marked as a duplicate of this bug. ***
Comment 21 scottputterman 2002-07-16 11:15:41 PDT
rvelasco, can you verify this on the trunk today so we can try to land this.
Comment 22 Rodney Velasco 2002-07-16 11:24:05 PDT
working on it as I speak
Comment 23 Rodney Velasco 2002-07-16 15:48:13 PDT
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 Boris Zbarsky [:bz] (still a bit busy) 2002-07-16 17:02:37 PDT
On linux:

mkdir 'foo\
bar'

will work in most shells (with the carriage return coming right after the
backslash).
Comment 25 Rodney Velasco 2002-07-16 17:52:32 PDT
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
Comment 26 scottputterman 2002-07-16 18:55:37 PDT
adding adt1.0.1+.  Please get drivers approval before checking into the branch.
Comment 27 Judson Valeski 2002-07-17 11:43:57 PDT
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
Comment 28 Daniel Veditz [:dveditz] 2002-07-17 16:28:58 PDT
Checked into 1.0 branch
Comment 29 Rodney Velasco 2002-07-18 16:48:55 PDT
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

Comment 30 Mitchell Stoltz (not reading bugmail) 2002-09-12 18:29:16 PDT
Created attachment 98992 [details] [diff] [review]
Patch for 0.9.4 branch - needs review
Comment 31 Daniel Veditz [:dveditz] 2002-09-12 19:36:20 PDT
Comment on attachment 98992 [details] [diff] [review]
Patch for 0.9.4 branch - needs review

r=dveditz
Comment 32 Alec Flett 2002-09-13 08:21:23 PDT
Comment on attachment 98992 [details] [diff] [review]
Patch for 0.9.4 branch - needs review

sr=alecf

Note You need to log in before you can comment on or make changes to this bug.