406724 - (CVE-2007-5967) Problematic embedding code

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GTK Widget, defect, P2)

Description

[Kai Engert [:KaiE:]]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 4

[Doug Turner (:dougt)]

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

Comment 5

[Kai Engert [:KaiE:]]

Attachment: Patch v1

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.)

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

See bug 388550 for some history about this.

Comment 7

[Kai Engert [:KaiE:]]

Attachment: Patch v2

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

Comment 8

[Daniel Veditz [:dveditz]]

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

r/sr=dveditz

Comment 9

[Kai Engert [:KaiE:]]

Attachment: Patch v2 + comments [checked in]

added comments, no code change from v2.

carrying forward r/sr

Comment 10

[Daniel Veditz [:dveditz]]

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

Comment 11

[Window Snyder]

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.

Comment 12

[Doug Turner (:dougt)]

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?

Comment 13

[Kai Engert [:KaiE:]]

Attachment: patch v3 for discussion

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.

Comment 14

[Kai Engert [:KaiE:]]

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.

Comment 15

[Doug Turner (:dougt)]

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

Comment 16

[Doug Turner (:dougt)]

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?

Comment 18

[Kai Engert [:KaiE:]]

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.

Comment 19

[Robert Relyea]

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

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 21

[timeless]

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.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 23

[Kai Engert [:KaiE:]]

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)

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 25

[Kai Engert [:KaiE:]]

(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

Comment 26

[timeless]

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

Comment 27

[Kai Engert [:KaiE:]]

(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"

Comment 28

[timeless]

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.

Comment 29

[Kai Engert [:KaiE:]]

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.

Comment 30

[timeless]

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).

Comment 31

[Daniel Veditz [:dveditz]]

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?

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 33

[Daniel Veditz [:dveditz]]

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

Comment 34

[Kai Engert [:KaiE:]]

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?