Kai Engert (:KaiE:) Assignee
	Description • 17 years ago

mozilla/embedding/browser/gtk/src/EmbedCertificates.cpp

That code is built on my machine when I build Firefox for Linux.

I don't know under which circumstances that code is being used, but I'm scared, because it uses problematic defaults.

Whenever it encounters a CA cert for downloading, it will silently import it as "trusted for all purposes".

Whenever a web site triggers creation of a key pair in the browser, and asks for "escrow", the code will silently allow the software to hand the private key to the server.

	Nelson Bolyard (seldom reads bugmail)
	Comment 1 • 17 years ago

All downloaded/imported certs are silently trusted for all purposes?  OMG!
Let's hope it's dead code.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 17 years ago

It looks like this code landed with MICROB_20061031_BRANCH.

I don't think it's "dead code" in that it's meant to be used.  I don't think any Mozilla.org products use it, however.

timeless would now for sure.

	Nelson Bolyard (seldom reads bugmail)
	Comment 3 • 17 years ago

This should be a P1 blocker bug for whatever product uses this code, if any.
If any products are already running this code, they need an update ASAP,
and this needs to go into the CVE vulnerability tracking system, etc.

Silently accepting all downloaded certs as trusted means that a malicious 
site can TRIVIALLY inject false root CA certs, and then conduct MASSIVE
MITM attacks on the user.   Any ISP or Wireless HotSpot provider could 
easily(!) do this.  

So, Timeless, what products are using this code?

	Doug Turner (:dougt)
	Comment 4 • 17 years ago

I am contacting NOKIA now.  Timeless in on a flight.

	Kai Engert (:KaiE:) Assignee
	Comment 5 • 17 years ago

A first attempt to change the code to saver defaults.

(Of course, if someone already got tricked and silently installed a CA cert as trusted, that mess must be cleaned up in some other way.)

	Nelson Bolyard (seldom reads bugmail)
	Comment 6 • 17 years ago

See bug 388550 for some history about this.

	Kai Engert (:KaiE:) Assignee
	Comment 7 • 17 years ago

it's better to spell out the "0" and use nsIX509CertDB::UNTRUSTED

	Daniel Veditz [:dveditz]
	Comment 8 • 17 years ago

Comment on attachment 292510 [details] [diff] [review]
Patch v2

r/sr=dveditz

	Kai Engert (:KaiE:) Assignee
	Comment 9 • 17 years ago

added comments, no code change from v2.

carrying forward r/sr

	Daniel Veditz [:dveditz]
	Comment 10 • 17 years ago

It wouldn't hurt to add the same comments in nsNSSDialog.cpp in case other people copy from there afresh rather than with EmbedCertificates.cpp

	Window Snyder
	Comment 11 • 17 years ago

Please wait until we hear from Nokia tomorrow before checking in.  We don't want to accidentally make the issue obvious and put their users at risk.

	Doug Turner (:dougt)
	Comment 12 • 17 years ago

the more we look at this file, the more we are worried.  Basically the class EmbedCertificates is stubbed out and not returning the right values in many cases.  

We are thinking about removing this file from the build all together.  Possibly commenting out

http://lxr.mozilla.org/mozilla/source/embedding/browser/gtk/src/EmbedPrivate.cpp#308 to line 349 or so?

	Kai Engert (:KaiE:) Assignee
	Comment 13 • 17 years ago

I used this patch to test whether firefox and seamonkey still compile with embedcertificates completely disabled. Following Doug's proposal I disabled the code in EmbedPrivate, and in addition I removed the file from the makefile. I ran "make clean" before the test.

	Kai Engert (:KaiE:) Assignee
	Comment 14 • 17 years ago

Comment on attachment 292520 [details] [diff] [review]
Patch v2 + comments [checked in]

I checked in this patch.

Not sure if we should close the bug - or keep open to work on the remaining details.

	Doug Turner (:dougt)
	Comment 15 • 17 years ago

I think we should fix this using v3 or something close to that.  however, i am not sure of the impact.

	Doug Turner (:dougt)
	Comment 16 • 17 years ago

we need to contact cve/cert about this -- the patch is public, but there are not bits on the wire for all devices.

nelson, dan, can you help or do you have any objection?

	Kai Engert (:KaiE:) Assignee
	Comment 17 • 17 years ago

timeless considers to modify NSS on their embedding device to use filename cert8a.db (instead of cert8.db) for the NSS cert db on their embedding device, in order to work around the problem that restores from backup might contain bad CA certs.

	Robert Relyea
	Comment 18 • 17 years ago

That rename is a bad idea. You will be stuck trying to make that work with future nss database upgrades. Since we know there's one on the horizon, it's a particularly bad idea.

bob

	Nelson Bolyard (seldom reads bugmail)
	Comment 19 • 17 years ago

I agree with Bob.  Changing that name creates an ongoing maintenance nightmare
for whoever sustains that product.

	timeless
	Comment 20 • 17 years ago

i currently have an ongoing maintenance nightmare. suggestions welcome.

do i just decide to assume that this problem is academic and ignore everyone who has used the product for the past 6 months?

kaie mentioned perhaps setting a flag in each db, but as none of the people at nokia know anything about the database structure, writing such code would also be an ongoing maintenance nightmare (assuming we even manage to write something).

here are the cases for people to consider:
1. there aren't many ways to import user keys to the device, the best way i know of is to use firefox to import keys and then copy the files to the device
2. someone could have used a bad version of this product and received a CA cert which they shouldn't have.
3. the device has a backup/restore feature which basically zips certain files/directories (including the 2 db files affected here)
4. the device has two methods for updating files: they can be updated using a .deb or the device can be reflashed (after reflashing one is generally prompted to restore from the backup)

Now, the next version will no longer automatically import these certificates. but any certificate already imported will remain, or in the case of a backup will be restored.

Assuming there's anything I can do for 2, I suspect it'll break 1.

Adding a key value flag to the database would fix databases from 2 but make databases from 1 incompatible.

I'm not sure how much flexibility we have, but if we want a solution, we pretty much need it today.

otherwise we end up w/ a situation where there are three versions:
1. accepts all ca certs
2. doesn't accept ca certs, but has ca certs
3. a version that tries to clean out ca certs from 1 but not ca certs from 2.

	Nelson Bolyard (seldom reads bugmail)
	Comment 21 • 17 years ago

4. Make the cert manager really work, 
- showing imported certs, as well as built-in certs
- allowing user to delete and/or distrust them.

	Kai Engert (:KaiE:) Assignee
	Comment 22 • 17 years ago

When the browser starts up, it must answer the following question:

  "Have cert8.db, key3.db stored on disk been produced by
   a compromised version of the browser?"

This is necessary, because on a fixed software environment,
users can restore a backup from before the fix.

Timeless had suggested the fixed software could use files with different names.
This would make sure the fixed software would never use old poisoned files.
But as Bob and Nelson are recommending against this option, we should drop it.

The next suggest came from me. My thinking was, instead of using different filenames, maybe a flag could been stored inside of the NSS database? Maybe, when the fixed software starts, it could check for a special value stored inside of the NSS db. When missing, the software could erase the NSS DB, let NSS create a fresh one, then add the magic indicating name=value pair to the database.
But I have no idea how easy this would be.
In addition, it would require changes to the NSS code, too.
I think we should drop this idea, too, because it's too complicated.

Here is idea number 3, and I think this is my best idea:
Use a separate "indicator file" as a flag "this database was produced using good software".
Here's a proposed algorithm:
- when the browser software starts up, really at the beginning of init (before any execution of NSS), check for the presence of a new file. Let's say /home/user/.mozilla/microb/.fixed_bug_406724
- when the file is missing, conclude the database came from a backup from before the fix. In that case, remove cert8.db and key3.db. Then create file .fixed_bug_406724 to indicate the future database will be clean.
- when the file is present, conclude the database is safe, and keep the files, do nothing

(FYI, a firmware flash will always remove all files stored in the device)

	Nelson Bolyard (seldom reads bugmail)
	Comment 23 • 17 years ago

Kai, I think that's an alternative with much less ongoing maintenance 
difficulties.  

Another suggestion would be to use time stamps on the relevant files,
e.g. perhaps
  if (certDB is older than the code fix) remove it.

	Kai Engert (:KaiE:) Assignee
	Comment 24 • 17 years ago

(In reply to comment #24)
> 
> Another suggestion would be to use time stamps on the relevant files,
> e.g. perhaps
>   if (certDB is older than the code fix) remove it.

I believe that won't work, for multiple reasons.

- one month after the fix became available, the user might still
  run the older, broken firmware.
  As a result the timestamp on the files will appear to look new,
  although it still might be poisoned.

- the user might not care to set the real time on the device

	timeless
	Comment 25 • 17 years ago

excess files don't work. because a backup will not *delete* such files.

	Kai Engert (:KaiE:) Assignee
	Comment 26 • 17 years ago

(In reply to comment #26)
> excess files don't work. because a backup will not *delete* such files.

I think you're saying "restore will not delete the new file".

If the user starts to use the browser, and afterwards runs a restore, you're right. In this scenario the indicator file will be present at the same time as the poisoned files from the restore.

I had assumed, users would always restore first, and would never run restore after having used the new OS.

You're right, it's possible that users might run restore at any time.


A better fix would involve the device's backup/restore software.
The restore mechanism could be changed to never restore db files from the old OS.

While we're being paranoid, I can think of one more scenario:
- user updates firmware
- user runs restore
- user does not use browser
- user creates a new backup

That new backup would be "created by new firmware version",
but would still contain the potentially bad db files.

So, the backup software would require one more decision:
"if the indicator file is missing, don't backup db files"

	timeless
	Comment 27 • 17 years ago

there's no way I'm hacking the backup software. I have no idea how much flexibility it has, but I think it's fairly safe to bet it doesn't have enough or that I wouldn't get it right.

	Kai Engert (:KaiE:) Assignee
	Comment 28 • 17 years ago

Instead of using an external indicator file, you mentioned the proposal of using a pref that is stored in the browser prefs.js file. This sounds like a great idea, as both prefs.js and *.db will always backed up and restored together.

	timeless
	Comment 29 • 17 years ago

yes, that was dolske's suggestion. which should work for all default cases (it doesn't work in any case where the user has custom scripts, but that's an edge case i'm willing to ignore).

	Daniel Veditz [:dveditz]
	Comment 30 • 16 years ago

Since a fix has been checked in can we close this bug? If there are remaining issues we should open a new bug that clarifies what work remains. With the total backout (bug 408238) having landed maybe there are no more remaining issues.

Did we ever figure out if anyone other than Nokia was using the trunk version of this code that we need to alert? Now that Nokia has made updated firmware available (in December I think) is it OK to release a security advisory on this now?

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 31 • 16 years ago

(not marking wanted-next based on comment 31, people should feel free to renom for blocking if they feel that this bug does)

	Daniel Veditz [:dveditz]
	Comment 32 • 16 years ago

Reassigning to Kai for answers to comment 31 -- we're done here, right? Additional cleanup should get a new bug.

	Kai Engert (:KaiE:) Assignee
	Comment 33 • 16 years ago

I am not aware of open issues.

I am ok to resolve this bug as fixed.

I think we're still waiting for a reply from Nokia: Are we ready now to publish?