211804 - mail should use rdf delegates instead of resource factories

Status: RESOLVED WORKSFORME

(MailNews Core :: Backend, defect)

Description

[Axel Hecht]

Mail makes use of resource factories which cause trouble with remote rdf content 
(you can easily create any resource with a tree and a rdf file).
I have at least found the mailfolders to work via resource factories.
It should use delegates to bind data to resources instead.

Comment 1

[(not reading, please use [e-mail] instead)]

axel, can you elaborate on how this is a securty-sensitive bug?

Comment 2

[(not reading, please use [e-mail] instead)]

passing the buck to mscott.

Comment 3

[Axel Hecht]

Any webpage can create any resource. So any resource implementation that is
around needs to be security checked.
I didn't find any obvious security or privacy issues in the impls in mailnews,
though I'm not 100% sure about it, esp about the things that involve outlook.
I wouldn't claim to have done a real security review on these resource impls
and their use, though.

One example exploit would be if one could spoof the existence of addressbooks or
entries by trying to get to that resource in a remote foo.rdf file. If the 
assertion isn't there, you don't have it. Or something like that.

This is more about potential security problems than about a known attack. If 
this is enough to have this have the box checked or not, is up to others. I was 
asked to check the box, so I did. Better safe than sorry.

Comment 4

[Benjamin Smedberg]

Have patch.

Comment 5

[Benjamin Smedberg]

Attachment: Use delegates instead of resources

This is a freakishly large patch that makes very few real changes. The only
real changes are 

1) I decontaminated the process of constructing mailnews folder
implementations. Instead of calling createInstance in the component manager, we
just #include the proper header and create one. There's lots more of the
decontamination work that could be done.

2) Since the reference model used by existing RDF delegates was guaranteed to
cause circular references in some cases, I had to define the model more
carefully. This means that the weak-reference model is now used by search
delegates.

Comment 6

[Benjamin Smedberg]

Comment on attachment 148373 [details] [diff] [review]
Use delegates instead of resources

Scott, I don't know whether you or David would be better reviewing this, so
I'll let you two vie for such pleasant duty.

Comment 7

[David :Bienvenu]

Seth might have some interest in this...

a -uw diff would make the diff a little smaller, remove the whitespace-only changes.

Comment 8

[Benjamin Smedberg]

*** Bug 20222 has been marked as a duplicate of this bug. ***

Comment 9

[Benjamin Smedberg]

Attachment: diff -w

Same thing, diff -w

Just a note, this patch depends on bug 211801 (and needs to land with that
patch). Also, I need to do a CVS-copy:
from mailnews/addrbook/src/nsAbDirectoryRDFResource.h/cpp
to   mailnews/addrbook/src/nsAbDirectoryRDFDelegate.h/cpp

If you want to test this patch, you can make symlinks in your local tree from
the old location to the new one.

Comment 10

[[e-mail]]

Apart from leaving some debugging stuff in the patch, I noticed that bsmedberg
started trying to integrate some cleanup into the patch, but gave up, because it
was making the patch too big. I'd like to be able to believe that a followup
patch to finish off the cleanup would get reviewed...

Comment 11

[Benjamin Smedberg]

Comment on attachment 148373 [details] [diff] [review]
Use delegates instead of resources

This has thoroughly rotted: I'm going to need a different approach that doesn't
rot quite so quickly.

Comment 12

[Benjamin Smedberg]

I give up.

Comment 13

[Joshua Cranmer [:jcranmer]]

Adding jminta to the CC list, as he is working on kill-RDF.

Comment 14

[David :Bienvenu]

We think we're more likely to make progress on this issue if the bug is open, and the threat seems fairly vague for mail folders.

Comment 15

[Joshua Cranmer [:jcranmer]]

Given how large and invasive the attempted patch was and the fact that we want to remove RDF anyways, it's probably a better use of time to work on de-RDF instead.

Comment 16

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 17

[Wayne Mery (:wsmwk)]

(In reply to Joshua Cranmer [:jcranmer] from comment #15)
> Given how large and invasive the attempted patch was and the fact that we
> want to remove RDF anyways, it's probably a better use of time to work on
> de-RDF instead.

indeed

Comment 18

[Magnus Melin [:mkmelin]]

de-RDF took care of this.