16 years ago
taking from Mitch. I'll look into fixing this.
Scott, any progress on this?
Please update this bug with an [adt1] - [adt3] impact rating (or take it off the list if it doesn't even rate adt3.) Thanks!
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?
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)
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.
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.
Created attachment 81550 [details] [diff] [review] 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.
The initial fix patch was attached three weeks ago -- what's new? We need this fixed ASAP (today, ideally) for 1.0. /be
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.
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
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.
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 on attachment 81550 [details] [diff] [review] initial fix r=cavin.
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.
fixed on the trunk. Marking fixed. I'll add the fixed1.0.0 field when I get approval for the branch.
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
thanks for the approval Chris. adding fixed 1.0.0 keyword
Has this been checked into the branch? I assume so given the fixed1.0.0, but the comment didn't really say.
bonsai shows that this was checked in to branch on 05/20/2002 22:31 by mscott
cc gayatri, guessing QA > 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?
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://firstname.lastname@example.org:143/onlinemove%3EUID%3E/INBOX%3E190038%3E/Trash" imap://email@example.com:143/deletemsg%3EUID%3E/INBOX%3E190038%3E/
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"