Closed
Bug 58774
Opened 24 years ago
Closed 24 years ago
We should "salt" the temp file name we store external content to
Categories
(Core Graveyard :: File Handling, defect, P3)
Core Graveyard
File Handling
Tracking
(Not tracked)
VERIFIED
FIXED
M18
People
(Reporter: mscott, Assigned: mscott)
References
Details
(Whiteboard: [rtm++] r=sspitzer, sr=alecf)
Attachments
(1 file)
3.35 KB,
patch
|
Details | Diff | Splinter Review |
When you click on a link that requies an external handler (i.e. requires us to
open a helper app or save to disk), we immediately start downloading the content
into a temp file while the user chooses their final destination for the content.
This temp file name is currently generated based on the file name in the url we
are trying to run. This could pose a security hazard as we are placing content
in a predictable place with a predictable file name.
The fix is to do what the profile folks are doing with salting profile names.
Use a salted name for the name of the temporary file.
I'll attach the diffs.
Assignee | ||
Comment 1•24 years ago
|
||
adding rtm keywords since the fix is low risk with a high gain (from a security
stand point).
Keywords: rtm
Whiteboard: [rtm need info]
Assignee | ||
Comment 2•24 years ago
|
||
Assignee | ||
Comment 3•24 years ago
|
||
Here's the proposed fix. Instead of using the file name in the url for the temp
file name, generate a random name for the temp file. This code was copied
directly from the code the profile team is using to salt profile names. Seth and
I are trying to figure out where to find a good home for this code on the tip so
it wouldn't need to be copied.
seeking an r=sspitzer, sr=alecf??
Status: NEW → ASSIGNED
Target Milestone: --- → M18
Comment 5•24 years ago
|
||
Good call.
Comment 6•24 years ago
|
||
r=sspitzer. looks good. I'll go log a bug to make sure we combine the
duplicated code.
Whiteboard: [rtm need info] → [rtm need info] r=sspitzer, sr=?
Assignee | ||
Comment 7•24 years ago
|
||
As an added bonus, this fix also fixes another rtm bug I have: 56296 -->
nsLocalFileMac chokes when given files > 31 characters, this causes us to fail
to download content on the Mac where the file name is > 31 characters.
Comment 8•24 years ago
|
||
my only concern here is that certain external applications might be expecting a
file to have a specific extension... i.e. RealAudio MIGHT be expecting the file
to be called foo.ram
if we salt it, do we loose the extension (looks like it from the patch)?
I believe 4.x retained the extension, or at the very least tacked on the default
extension from the MIME types for that type.
If I'm imagining 4.x behavior, then never mind :)
the patch itself looks fine, sr=alecf - leaving in the need info state so that
my question about filename extensions can be addressed.
Whiteboard: [rtm need info] r=sspitzer, sr=? → [rtm need info] r=sspitzer, sr=alecf
Assignee | ||
Comment 9•24 years ago
|
||
actually the patch applies the file extension from the url onto the salted name.
So we do indeed preserve the file extension and every thing is kosher. Moving to
rtm+ now that everything is lined up.
Whiteboard: [rtm need info] r=sspitzer, sr=alecf → [rtm+] r=sspitzer, sr=alecf
Component: Browser-General → Networking
Keywords: patch
OS: Windows NT → All
QA Contact: doronr → sairuh
Hardware: PC → All
Comment 10•24 years ago
|
||
This bug is in candidate limbo
Assignee | ||
Comment 11•24 years ago
|
||
This has been checked into the tip.
Comment 12•24 years ago
|
||
PDT marking [rtm-]. Medium benefit, medium risk, not enough at this point.
Whiteboard: [rtm+] r=sspitzer, sr=alecf → [rtm-] r=sspitzer, sr=alecf
Updated•24 years ago
|
QA Contact: sairuh → tever
Comment 13•24 years ago
|
||
rtm++, taking on respin.
Whiteboard: [rtm-] r=sspitzer, sr=alecf → [rtm++] r=sspitzer, sr=alecf
Assignee | ||
Comment 14•24 years ago
|
||
checked into the branch and the tip.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 15•24 years ago
|
||
Scott - can you give suggestions on verification? This extends beyond mail
attachments, doesn't it?
Assignee | ||
Comment 16•24 years ago
|
||
My apologies I made a big post right after Lisa asked for verification
instructions but apparently I wasn't logged into bugzilla so my changes didn't
get submitted and I never noticed the login screen.
Let's try this again a day later:
Browse to www.mozilla.org. Click on one of the nightly build links. When the
helper app dialog comes up, don't do anything. =)
Go to your temp directory. Search for *.exe, *.bin, *.gz (depending on the
nightly you downloaded). You should see an obscured name in the temp directory
like x584lkz.exe. This is the temp file we are downloading content in the
background into.
Now go ahead and go back to the helper app dialog and click save to disk. Pick a
file location (for kicks use the same directory the salted file is in). When the
download is complete, go back to the temp directory and make sure the salted
name is no longer there. Verify that the file name you chose is there.
An added bonus --> try launching with a helper app. i.e. click on a mp3 link or
something.
Comment 17•24 years ago
|
||
vrfied using opt comm branch bits [using mscott's steps]:
linux rh 6.2 2000.11.08.07-n6
winNT 2000.11.08.01-n6
mac os 9.0 2000.11.08.00-n6
Keywords: vtrunk
Comment 18•24 years ago
|
||
Is this really a security issue? It causes lots of confusion with downloaded
filenames. See bug 60203.
Comment 19•24 years ago
|
||
Simon,
As Scott describes, this is a security issue as it could allow the placing of
foreign content at a known location. I see from 60203 that people are confused,
so we need to educate people that this is now the expected behavior.
Comment 20•24 years ago
|
||
this is similar to 63225
Comment 21•24 years ago
|
||
qa to me.
VERIFIED, per Sarah.
Updated•24 years ago
|
Blocks: advocacybugs
Updated•24 years ago
|
No longer blocks: advocacybugs
Comment 22•21 years ago
|
||
This security fix was reversed in bug 60203.
Comment 23•21 years ago
|
||
not quite - during the download, the salted file name is still used. only once
the download is finished is the file renamed (in the case of a helper application).
Comment 24•21 years ago
|
||
So? In the case of a helper application that might be switched to automatic by
the user, like pdf files perhaps, the files are still renamed to a predictable
name without a user interaction. It should not be hard to use a delay and then
invoking that file from JS to execute something with local permissions.
The root of the problem is that the salting is very similar to security by
obscurity. In my opinion it would be safer to declare all download locations
like the caches and temp directories equivalent to internet zone because that's
where the content in these directories comes from.
This would amount to something similar as tainting these locations, as perl does
with internal dataflow.
Updated•9 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•