142695 - Profile damaged after printing and browser restart

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect)

Description

[Keith Moe]

Ever since using RC1 and build 0426, after shutting down normally, including
shutting Warpzilla down normally, starting the browser results in the profile
being damaged and all password manager setting, home page, etc., being lost. 
Upon selecting a profile and clicking "Start Mozilla", I get the following error:

An error occurred reading the startup configuration.  Please contact your
administrator.

line 18:SyntaxError: unterminated string literal. %s"%s

(The line number has been different each time this has happenned.)

Scanning every file in the profile, I can find NO occurrance of the %s"%s string.

The only recourse I have is to create a completely new profile.  My Bookmarks,
History, and Cookies file are undamaged and can be copied from the old profile
to the new one, but this is otherwise very annoying.

Running MCP1, 18.G P-4, 512M.

Comment 1

[Keith Moe]

Update.  The profile is damaged after printing a web page.  The error occurs on
a line in the profile that defines the printer being used.

Here's the profile before damage:

# Mozilla User Preferences
// This is a generated file!

user_pref("browser.history.last_page_visited",
"http://bugzilla.mozilla.org/show_bug.cgi?id=142695");
user_pref("browser.startup.homepage", "http://pages.sbcglobal.net/kemoe/");
user_pref("browser.startup.homepage_override.mstone", "rv:1.0rc2");
user_pref("browser.toolbars.showbutton.go", true);
user_pref("general.open_location.last_url", "pages.sbcglobal.net/kemoe/");
user_pref("intl.charsetmenu.browser.cache", "ISO-8859-1");
user_pref("network.cookie.cookieBehavior", 0);
user_pref("prefs.converted-to-utf8", true);
user_pref("security.warn_submit_insecure", false);
user_pref("signon.SignonFileName", "20746724.s");
user_pref("timebomb.first_launch_time", "%d");
user_pref("update_notifications.provider.0.last_checked", 1021698201);
user_pref("wallet.caveat", true);

Here's the damaged profile:

# Mozilla User Preferences
// This is a generated file!

user_pref("browser.history.last_page_visited", "http://www.mozilla.org/ports/os2/");
user_pref("browser.startup.homepage", "http://pages.sbcglobal.net/kemoe/");
user_pref("browser.startup.homepage_override.mstone", "rv:1.0rc2");
user_pref("browser.toolbars.showbutton.go", true);
user_pref("general.open_location.last_url", "pages.sbcglobal.net/kemoe/");
user_pref("intl.charsetmenu.browser.cache", "ISO-8859-1");
user_pref("network.cookie.cookieBehavior", 0);
user_pref("prefs.converted-to-utf8", true);
user_pref("print.printer_Stylus_Color
740.print_command", "");
user_pref("print.printer_Stylus_Color
740.print_paper_height", " 11.00");
user_pref("print.printer_Stylus_Color
740.print_paper_size_type", 1);
user_pref("print.printer_Stylus_Color
740.print_paper_size_unit", 0);
user_pref("print.printer_Stylus_Color
740.print_paper_width", "  8.50");
user_pref("print.selection_radio_enabled", false);
user_pref("security.warn_submit_insecure", false);
user_pref("signon.SignonFileName", "20746724.s");
user_pref("timebomb.first_launch_time", "%d");
user_pref("update_notifications.provider.0.last_checked", 1021698201);
user_pref("wallet.caveat", true);

I suspect that Warpzilla does not handle a printer name with a special character
in it.  I alter the printer name that Warp Created to reduce the amount of
desktop space that the printer icon takes by putting a "new line" in it.
Warp installed the printer as:
Stylus Color 740

I changed it to:
Stylus Color
740

It shouldn't matter if I do this.  No other Warp application has a problem with
this change.

Comment 2

[Keith Moe]

After deleting the offending printer lines (with Warpzilla shut down), I get the
same type of error, only with a %s* (* is really a right pointing arrow).  This
continues until I delete the 

user_pref("update_notifications.provider.0.last_checked", 1021698201);

line.  Then Mozilla will start successfully with the profile.

Comment 3

[Matthias Versen [:Matti]]

-> Printing

Comment 4

[Roland Mainz]

I suggest to filter&&block such printer names which do contain "invalid" chars.
Trying to work around this kind of problem (like having a newline in a printer
name) will be very difficult...

... however... a possible fix to avoid the prefs corruption would be to _always_
mangle (using urlencoding or something else which "escapes" such chars) the pref
strings which are written to the profile...

Comment 5

[Roland Mainz]

--> prefs

We _must_ avoid any corruption in the prefs regardless what the code writes into
it. Using URLencoding for the strings is AFAIK backwards-compatible.

Comment 6

[rods (gone)]

What API do I use to encode them?

Comment 7

[Roland Mainz]

rods wrote:
> What API do I use to encode them?

Let the pref-gurus handle this issue, please...

This is - de facto - a crossplatform issue when some API sticks chars like
NEWLINE etc. into prefs values or names.

Note that this bug may be misused as a exploit when the code in prefs.js is
allowed to read the local disk.
Example:
-- snip --
user_pref("print.printer_Stylus_Color<newline>evil_code_formatting_disk();<newline>.blabla.blabla",
"bla bla blub");
-- snip --
This should look (in prefs.js) like this:
-- snip --
user_pref("print.printer_Stylus_Color
evil_code_formatting_disk();
.blabla.blabla", "bla bla blub");
-- snip --
(requires some testing how to make this useable as a exploit...)

The fix is to use URLencoding for both pref name and value...

Comment 8

[Jessica Blanco]

Attachment: nsDeviceContextSpecOS2.cpp-fixes prob on OS/2 where printers have newline in name

Made this change on OS/2 to fix the problem where printer names have newlines
in them. This fix to "GetPrinter" will always return a printer name minus the
newline (/r/n).

Comment 9

[Roland Mainz]

Comment on attachment 82685 [details] [diff] [review]
nsDeviceContextSpecOS2.cpp-fixes prob on OS/2 where printers have newline in name

The patch is nice but we _MUST_ fix the problem in the prefs backend, too.

Comment 10

[Mike Kaply [:mkaply]]

Comment on attachment 82685 [details] [diff] [review]
nsDeviceContextSpecOS2.cpp-fixes prob on OS/2 where printers have newline in name

r=mkaply
sr=blizzard (platform specific code)

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 82685 [details] [diff] [review]
nsDeviceContextSpecOS2.cpp-fixes prob on OS/2 where printers have newline in name

a=[e-mail] for drivers (for the OS/2-only fix).  Please commit today if
possible.

Comment 12

[Roland Mainz]

The bug may be used as a remote exploit to execute any commands under the same
priviledges as the mozilla-bin process (assuming that code in prefs.js can reach
the local disk) or is - at least - able to make Mozilla unuseable.
(The fix is very simple: "escape" any special chars like NEWLINE)

CC:'ing some people who may be interested in this issue...

Comment 13

[Christopher Blizzard (:blizzard)]

Two things:

1. How does remote content add prefs that contain code?  It looks like you might
be able to do this by changing the printer names on your local network, but
that's hardly the same as viewing a web page and having the same result.

2. Why is network data used to construct the preference names?  Why not do what
mail/news does and have something like:

user_pref("print.printer1.name", "Stylus_Color");
user_pref("print.printer1.foo", "bar");

Comment 14

[Mike Shaver (:shaver emeritus)]

Yeah, I didn't understand the part about hostile content being able to alter
printer settings.  Even without pref corruption, that would be a pretty serious
bug, I think: "Hey, let's make him print his stuff to _our_ network printer...".

I think Blizzard is onto the right track here with his suggestion that the
printing prefs ape mailnews.  Prefs need to escape characters in values, but
it's up to the pref-user to make sure that they put all user-specified data in
the value portion of the pref pair, or otherwise make sure that it's "pref safe".

(Prefs _do_ escape values, right?  I can have a home directory or other path
with newlines or quotation marks in it, right?)

Comment 15

[Christopher Blizzard (:blizzard)]

Even more succinctly put: "Don't use user entered data as a pref key."

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Fix checked into branch for OS/2 by mkaply last night (for RC2)

Comment 17

[Roland Mainz]

Mike Shaver write:
> Prefs _do_ escape values, right?  I can have a home directory or other path
> with newlines or quotation marks in it, right?)

AFAIK (please correct me if I am wrong) no.

Comment 18

[Roland Mainz]

Christopher Blizzard wrote:
> 1. How does remote content add prefs that contain code?  It looks like you 
> might be able to do this by changing the printer names on your local network, 
> but that's hardly the same as viewing a web page and having the same result.

I do not see the issue in the printing code, I see a general problem in the
prefs backend code. Please ignore the OS/2-specific issue for a moment - and
then think about these questions:
1. Do we really know all places where we write preferences to prefs.js ?
2. Does all code which writes preferences gurantee that the contents of the pref
names and values do not contain any "invalid" chars (like a "\",
\"\"\);\n"-sequence) ? Note that we even compose the _name_ of the pref items
dynamically out of other input in some places.

Comment 19

[Mike Shaver (:shaver emeritus)]

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.

I'll file another bug about the imperfect escaping and reference it here.

Comment 20

[Roland Mainz]

blizzard wwrote:
> 2. Why is network data used to construct the preference names?  Why not do 
> what mail/news does and have something like:
>
> user_pref("print.printer1.name", "Stylus_Color");
> user_pref("print.printer1.foo", "bar");

This is mainly used to store per-printer prefs (and no, we cannot use a
numbering scheme without making the code a royal pain since larger networks may
have a few hundred printers. Just crawling the list up and down will not work
very well...).

Comment 21

[Mike Shaver (:shaver emeritus)]

> 1. Do we really know all places where we write preferences to prefs.js ?

Yes.  Only the pref service serializes preference information.

> 2. Does all code which writes preferences gurantee that the contents of the pref
> names and values do not contain any "invalid" chars (like a "\",
> \"\"\);\n"-sequence) ?

No, but it can (see just-filed bug 143459).

> Note that we even compose the _name_ of the pref items
> dynamically out of other input in some places.

As I say above, echoing blizzard, you shouldn't do that unless you can be sure
that the name will be a legal pref key.  Mailnews deals with this by using their
heads and putting the dynamic data in a value, using only carefully generated keys.

> (and no, we cannot use a
> numbering scheme without making the code a royal pain since larger networks 
> may have a few hundred printers. Just crawling the list up and down will not 
> work very well...).

If you have per-printer prefs for a few hundred printers, maybe you just accept
that there's going to be a lot of pref data stored?

Otherwise, it's _your_ responsibility as generator of the pref key to make sure
that it's a legal pref key (the restrictions aren't very onerous; consider
URL-escaping).

Comment 22

[Roland Mainz]

Mike Shaver wrote:
> If you have per-printer prefs for a few hundred printers, maybe you just 
> accept that there's going to be a lot of pref data stored?

The amount of data is not the real problem. I was more thinking about making the
access to the prefs as easy as possible (in: printer name, name of var we want
to fetch; out: value) and as fast as possible. [please no discussion about the
pro/contra... the current printer code is working nearly perfectly - and it took
a very long time to get it into this state]

> Otherwise, it's _your_ responsibility as generator of the pref key to make 
> sure that it's a legal pref key (the restrictions aren't very onerous; 
> consider URL-escaping).

I thought (until I saw this bug) that the prefs backend does this job
automagically... well... looks that I was wrong... ;-(

Comment 23

[Brian Nesse (gone)]

So apparently this patch was landed on the branch. Are we planning to land it on
the trunk as well?

Comment 24

[Mike Kaply [:mkaply]]

The OS/2 specific fix has landed on both trunk and branch. As far as I am 
concerned, the OS/2 piece is done.

Is there any issue with prefs in general or does the shaver bug take care of 
that?

Comment 25

[Brian Nesse (gone)]

Yes, the bug shaver filed covers the issues with the data that might be stuck
into the preference value. If you landed on the trunk, this bug is closed.

Comment 26

[Mike Kaply [:mkaply]]

OS/2 fix is in.