TBE-01-007: "Reload Page" dialog runs Javascript with external attachment because we only disable JavaScript for nsIMsgMessageUrls
Categories
(MailNews Core :: Attachments, defect)
Tracking
(Not tracked)
People
(Reporter: BenB, Assigned: alta88)
Details
(Keywords: privacy, sec-low, testcase)
Attachments
(2 files, 8 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
alta88
:
review+
|
Details | Diff | Splinter Review |
Reporter | ||
Comment 1•7 years ago
|
||
Reporter | ||
Comment 2•7 years ago
|
||
Comment 3•7 years ago
|
||
Comment 4•7 years ago
|
||
Comment 5•7 years ago
|
||
Comment 6•7 years ago
|
||
Updated•7 years ago
|
Updated•7 years ago
|
Comment 7•7 years ago
|
||
Comment 8•7 years ago
|
||
Comment 9•7 years ago
|
||
Comment 10•7 years ago
|
||
Comment 11•7 years ago
|
||
Comment 12•7 years ago
|
||
Comment 13•7 years ago
|
||
Comment 14•6 years ago
|
||
Comment 15•6 years ago
|
||
Updated•6 years ago
|
Comment 16•6 years ago
|
||
I believe alta88 is on this.
AFAICT, bug 1345167 doesn't fix this per se since while that checks the remote file (using HEAD) there's nothing preventing the next call to that URL to lead somewhere else. Or that HEAD/GET would give different results.
But I think it could fairly easily be modified to do so. We need to do the fetch before accessing, then make sure to access the result we already have.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Updated•6 years ago
|
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 28•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #27)
(In reply to alta88 from comment #23)
You're missing that the response.url will be the redirect url, and will
differ from request.url. For fetch() it is one call only but 2 calls in
gecko, which you can even see in the console with xhr and request toggled
on. There is one response to fetch() which is for the redirect url. I don't
know what 'response can vary between calls' means, it doesn't work that way.It means that the attacker can have call #1 return A and call #2 return B even if the url is the same. It also means he can set HEAD to respond C and GET to respond D.
I see finally what you're describing, a timing attack where a successful HEAD response is turned into a malicious response on the open/GET to the same url. And for such an attack, doing a GET instead of HEAD doesn't necessarily prevent that either, so testing fetch() etc etc isn't the answer.
nsMessenger::OpenURL() loads the url in messagepane docShell so a content handler can be invoked on it. The url could be sent to a browser via nsIExternalProtocolService/nsIWebNavigation loadURI() but this would be a very degraded UX.
How about an override of netError.xhtml in aboutRedirector.js and simply removing the retry button or testing the errors in a netError.js stub and hiding it for certain conditions.
Edit: Btw, sorry for being slow to get this, I was somehow fixated on a specific flow..
Assignee | ||
Comment 29•6 years ago
|
||
Ok, so putting up a redirect server, pointing it to a file:// redirect url and using the test eml above, will make fetch() get a 200 response for HEAD (and continue to the corrupt page as a result of OpenURL()) but will catch with a Network Error with GET.
Comment 30•6 years ago
|
||
I guess overriding netError could work, maybe to make Retry open the URL open in the external browser for error conditions?
Another possible route would be doing the fetch, putting the verified ok'd data into a data: url and have sMessenger::OpenURL open that.
Assignee | ||
Comment 31•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #30)
I guess overriding netError could work, maybe to make Retry open the URL open in the external browser for error conditions?
I think it's not great UX to load the error page and leave the message; there's then no way to get back without deselect/select. If the error occurs in the first place, it won't work in the browser either. If we go this route, I'd say the button should say Return to Message or such.
Another possible route would be doing the fetch, putting the verified ok'd data into a data: url and have sMessenger::OpenURL open that.
Even if the link is to a 100M video (like the now obsolete testcase I attached before)?
If we replace HEAD with GET, a redirect to both an existing and non existing local file fails with Network Error, and there will be no open and no error page. Do we agree that fixes the case here?
In the case of a long server non response (fetch eventually times out) I made it so if the message has changed (likely) the response will just die. And if not, it will be a caught error.
What potential issues remain?
Comment 32•6 years ago
|
||
(In reply to alta88 from comment #31)
(In reply to Magnus Melin [:mkmelin] from comment #30)
I guess overriding netError could work, maybe to make Retry open the URL open in the external browser for error conditions?
I think it's not great UX to load the error page and leave the message;
there's then no way to get back without deselect/select. If the error occurs
in the first place, it won't work in the browser either. If we go this
route, I'd say the button should say Return to Message or such.
Sounds ok, or just disable the button.
Another possible route would be doing the fetch, putting the verified ok'd data into a data: url and have sMessenger::OpenURL open that.
Even if the link is to a 100M video (like the now obsolete testcase I
attached before)?
Might not be so nice, no.
If we replace HEAD with GET, a redirect to both an existing and non existing
local file fails with Network Error, and there will be no open and no error
page. Do we agree that fixes the case here?
I'm not sure I follow. If you're replacing HEAD with GET you do need to use the result of the GET. But, perhaps we won't take this route. I'm open to suggestions.
What potential issues remain?
Can't think of any.
Assignee | ||
Comment 33•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #32)
(In reply to alta88 from comment #31)
(In reply to Magnus Melin [:mkmelin] from comment #30)
I guess overriding netError could work, maybe to make Retry open the URL open in the external browser for error conditions?
I think it's not great UX to load the error page and leave the message;
there's then no way to get back without deselect/select. If the error occurs
in the first place, it won't work in the browser either. If we go this
route, I'd say the button should say Return to Message or such.Sounds ok, or just disable the button.
Another possible route would be doing the fetch, putting the verified ok'd data into a data: url and have sMessenger::OpenURL open that.
Even if the link is to a 100M video (like the now obsolete testcase I
attached before)?Might not be so nice, no.
If we replace HEAD with GET, a redirect to both an existing and non existing
local file fails with Network Error, and there will be no open and no error
page. Do we agree that fixes the case here?I'm not sure I follow. If you're replacing HEAD with GET you do need to use the result of the GET. But, perhaps we won't take this route. I'm open to suggestions.
In the case here, with a redirect to file, the GET fetch() will fail and the attack cannot proceed.
Hypothetically, the fetch() (first GET) has to succeed, and then the url response changed for the open (second GET) to show the error page. But how can the attacker know there's a second GET coming. This seems implausible.
Anyway, I suggest we change it to GET (http only) for the first case (this bug), and change netError to return to message for the second case.
What potential issues remain?
Can't think of any.
Comment 34•6 years ago
|
||
But how can the attacker know there's a second GET coming. This seems implausible.
If there is just a distinction between data from HEAD vs GET that would be easy. For GET+GET, I suppose one could match up on remote IP and take an educated guess... but sure, harder.
Anyway, I suggest we change it to GET (http only) for the first case (this bug), and change netError to > return to message for the second case.
Sounds good.
Assignee | ||
Comment 35•6 years ago
|
||
Yes, it's sure possible to trick even 2 GETs: read this bug, user agent, ip, second GET within a second..
The button should Return to Messge in general, as Retry usually fails, and the UX is nicer now for that page.
I don't think we want to clone netError.xhtml and netError.js, so did it this way.
Updated•6 years ago
|
Comment 36•6 years ago
|
||
Assignee | ||
Comment 37•6 years ago
|
||
no string change.
Comment 38•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Comment 39•6 years ago
|
||
Comment 40•6 years ago
|
||
Sorry, this doesn't pass linting:
$ ../mach eslint mail/base/content/
c:\mozilla-source\comm-central\comm\mail\base\content\mailWindow.js
219:54 error 'retryThis' is not defined. no-undef (eslint)
? 1 problem (1 error, 0 warnings)
... and I wouldn't know how to fix it. This seems to come from netError.js.
Comment 41•6 years ago
|
||
/* global retryThis */ on top of the file should do it. But perhaps we don't need the retryThis at all, since removing the eventListener doesn't look at the actual function.
Assignee | ||
Comment 42•6 years ago
|
||
declare global.
Comment 43•6 years ago
|
||
https://hg.mozilla.org/comm-central/rev/5e3c486189d48c8cfa6084be270236cd807c8205
On netError page change retry button to reload message. r=mkmelin
Updated•5 years ago
|
Updated•4 years ago
|
Description
•