127702 - URLs in mail messages can issue IMAP commands

Status: VERIFIED FIXED

(MailNews Core :: Security, defect)

Description

[Mitchell Stoltz (not reading bugmail)]

Mozilla allows opening imap:// urls by javascript or by just
clicking on them.
Which is more, imap protocol commands may be embeded in the urls, which may
lead to modification of user's folders and probably more.

For example the following link in HTML message:
------------------------------
<a href="imap://USERNAME@SERVER:143/create%3E/guninski%3E%3E">CREATE</a>
[USERNAME and SERVER must be changed]
------------------------------

Creates the imap folder "guninski" if it is clicked (it is possible to get
an error message but the folder is still created).

It is also possible javascript to open imap messages. The same origin
policy prevents them from being read but it is still bad remote content to
open local email.

------------ supplement --------------------
No user interaction is required.
The following in HTML email:
---------------------
<iframe src="imap://USER@HOST:143/create%3E/guninski%3E%3E" ></iframe>
---------------------
Creates the folder GUNINSKI if the message is previewed.
More problems are possible.
For example the following:
----------------------------
<a href="imap://USER@HOST:143/onlineCopy%3EUID%3E/Trash%3E2%3E/INBOX">COPY</a>
----------------------------
Copies on message from Trash to INBOX.
Almost sure deleting messages and even folders is possible.
The good news is this does not work from the web.

I suggest disabling imap: URLs from email, the same way that they are not
accessible from web pages.

Comment 1

[Scott MacGregor]

taking from Mitch. I'll look into fixing this. 

Comment 2

[Mitchell Stoltz (not reading bugmail)]

Scott, any progress on this?

Comment 3

[selmer (gone)]

Please update this bug with an [adt1] - [adt3] impact rating (or take it off the
list if it doesn't even rate adt3.)  Thanks!

Comment 4

[Asa Dotzler [:asa]]

It's my understanding that this is one of two security issues that we need fixed
for 1.0 (and any other products based on the branch which are exposed to this
vulnerability). We're shipping RC1 today so we missed that train. Is this gonna
make the next one?

Comment 5

[Scott MacGregor]

Here's what I'm thinking about doing to solve this problem. 

1) Add a restricted capabilities bit to nsIImapUrl.idl

this restricted bit would be set to true every time we create an imap url.

2) Every time our imap service attempts to create a url to perform an action it
clears the restricted bit on the url it creates. Urls generated from the imap
service should not be restricted

Any url that comes from the user clicking on a url or from JS code trying to
load an imap url will end up with a nsIImapUri object which has the restricted
bit set.

3) When we go to run the url, we only run urls with the restricted bit set if
they represent the following actions:
fetching a part
displaying a message
what others? 

cc'ing bienvenu to get his input on my solution and to see if he has imap
actions he'd like to add to the restricted list. (Does this effect shared
folders at all? I don't think so)

Comment 6

[David :Bienvenu]

we need links to folders to work, i.e., clicking on an imap url to an imap
folder should load that folder.

I was surprised how many imap urls get run without going through nsImapService -
things like view | source, quoting a message, and others I can't remember, so
you need to be very careful. I already have a flag for urls not created by the
imap service; it's a boolean called m_externalLinkUrl and I think you should be
able to use it.

The other possibility is distinguish between imap uri's like links to messages
and folders, and *internal imap commands* constructed by nsImapService, things
like "create folder", "delete folder" etc, which really should be only allowed
from internal urls. So I guess what I'm suggesting is looking at the imap action
in conjunction with the m_externalLinkUrl and only allow external link urls to
fetch messages/parts and load folders.

Comment 7

[Scott MacGregor]

Wow thanks David. How convient that you've already added the flag:
m_externalLinkUrl. That will make this fix trivial in code (but not in risk of
course). Thanks for the tip.

Comment 8

[Scott MacGregor]

Attachment: initial fix

I think I'm going to change this patch, but here's the first pass. 

The following code lives in nsImapMockChannel::AsyncOpen (which is the only way
to open an imap url). If the url is an external link, then we only allow the
following actions:

1) nsIImapUrl::nsImapSelectFolder 
2) nsIImapUrl::nsImapMsgFetch
3) nsIImapUrl::nsImapOpenMimePart

However I think I'm going to change this patch and add a method to nsIURI to
the effect of validUrl() which does this check and returns a boolean if we are
allowed to open the url. In the future, other consumers may be able to leverage
this security check instead of just coding it directly into AsyncOpen. Then
again  AsyncOpen really is the only way to open the url so that extra step may
be unnecessary.

Comment 9

[Brendan Eich [:brendan]]

The initial fix patch was attached three weeks ago -- what's new?  We need this
fixed ASAP (today, ideally) for 1.0.

/be

Comment 10

[Scott MacGregor]

I doubt you'll want to take this for moz 1.0. I need to get this into the trunk
and it needs baking time. There are too many ways in which it could break imap
(replying to messages, quoting replied messages, saving / opening attachments,
etc). 

My gut would say pass on this for 1.0. 

Comment 11

[Brendan Eich [:brendan]]

mscott, 1.0 shipping with a security hole like this means that we have to
identify the hole and summarize it to anyone thinking of using 1.0.  Maybe you
don't care, so long as this is fixed for Netscape rtm (and presumably 1.0.1). 
Either we fix, or we disclose enough information for 1.0 users to make informed
decisions, though.  Does this alter your thinking?

/be

Comment 12

[Scott MacGregor]

I told Dawn a fib just now. I didn't think you could really delete a message
through this kind of an exploit, just cause the user to select one of their
folders or select a message. But I was just able to force a url which would
delete a message and sure enough we ran it when I clicked on it(even though the
messsage key didn't exist in my case). With this patch we don't even try to run
the url. 

That test does change my thinking on this bug. I'll get r and sr today for it.
But we still have to recognize some risk for 1.0 by taking it today without some
trunk baking time. 

Comment 13

[Scott MacGregor]

cc'ing Cavin since I'm going to have him review this patch and you have to be
cc'ed in order to see it. 

Comment 14

[Cavin Song]

Comment on attachment 81550 [details] [diff] [review]
initial fix

r=cavin.

Comment 15

[David :Bienvenu]

Comment on attachment 81550 [details] [diff] [review]
initial fix

sr=bienvenu - let's try this out asap. This might require some tweaking, but
it's probably better to start out conservatively in the list of things we
allow.

Comment 16

[Scott MacGregor]

fixed on the trunk. Marking fixed. I'll add the fixed1.0.0 field when I get
approval for the branch. 

Comment 17

[chris hofmann]

Comment on attachment 81550 [details] [diff] [review]
initial fix

lets go ahead and get this on the branch as soon as possible so it can get
extra testing.	a=chofmann,blizzard

Comment 18

[Scott MacGregor]

thanks for the approval Chris. adding fixed 1.0.0 keyword

Comment 19

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

Has this been checked into the branch?  I assume so given the fixed1.0.0, but
the comment didn't really say.

Comment 20

[Dawn Endico]

bonsai shows that this was checked in to branch on 05/20/2002 22:31 by mscott

Comment 21

[John Unruh]

cc gayatri, guessing QA > huang

Comment 22

[Karen Huang]

Used 06-03-08-trunk build
If above reporter's original URL is correct,      
"imap://USERNAME@SERVER:143/create%3E/foldername%3E%3E"
Then I am wondering to know why am I getting the following dialogs for this fix? 
"Would you like to subscribe to Shared Folders/User/3qatest01/karenfolder?"
After I select OK, I got "The current command did not succeed. The 
mail server responded: Mailboxes does not exist."
It seems this fix is trying to call bug 112105 fix....
Is this expected result?

Comment 23

[Scott MacGregor]

Example URL for QA to use for testing...replace username with your username on
the specified mail server. Try inserting this url into a mail message, send it
to yourself then click on the url. For bonus points use a real message key from
your inbox by looking at an imap log and replacing the numbers in this URL with
a real message key in your account. 

imap://[e-mail]:143/onlinemove%3EUID%3E/INBOX%3E190038%3E/Trash"


imap://[e-mail]:143/deletemsg%3EUID%3E/INBOX%3E190038%3E/

Comment 24

[Karen Huang]

Verified fix on all the platforms on current branch builds:
Windows 07-11-08-1.0
Linux 07-11-07-1.0
Mac 07-11-05-1.0

*Before fix:
-For UID 4100 (for move): 
1364[4add468]: nsmail-1:S-INBOX:SendData: 19 uid copy 4100 "Trash"
-For UID 4104 (for delete):
1364[4a9add8]: nsmail-1:S-INBOX:SendData: 11 uid store 4104 +FLAGS (\Deleted)
-For UID 4102 (for copy)
1364[4a9add8]: nsmail-1:S-INBOX:SendData: 14 uid copy 4102 "Trash"
1364[4a9add8]: nsmail-1:S-INBOX:CreateNewLineFromSocket: 14 OK [COPYUID 
964732484 4102 3106] Completed

After this bug fix, I don't see move, delete and copy implementation from Inbox 
to Trash folder on the UI and the IMAP log.

The only rest issue for create command(see above Comment #22)has been logged on 
Bug 157037.
 
Marking as verified and changing keywords from "fixed1.0.0" to "verified1.0.1"