Closed Bug 58774 Opened 22 years ago Closed 22 years ago

We should "salt" the temp file name we store external content to

Categories

(Core Graveyard :: File Handling, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mscott, Assigned: mscott)

References

Details

(Whiteboard: [rtm++] r=sspitzer, sr=alecf)

Attachments

(1 file)

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.
adding rtm keywords since the fix is low risk with a high gain (from a security
stand point).
Keywords: rtm
Whiteboard: [rtm need info]
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
nominating for 0.9 should this not make rtm 
Keywords: mozilla0.9
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=?
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.

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
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
This bug is in candidate limbo
This has been checked into the tip. 
PDT marking [rtm-]. Medium benefit, medium risk, not enough at this point.
Whiteboard: [rtm+] r=sspitzer, sr=alecf → [rtm-] r=sspitzer, sr=alecf
Blocks: 59114
QA Contact: sairuh → tever
rtm++, taking on respin.
Whiteboard: [rtm-] r=sspitzer, sr=alecf → [rtm++] r=sspitzer, sr=alecf
checked into the branch and the tip.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Scott - can you give suggestions on verification?  This extends beyond mail
attachments, doesn't it?
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.

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
Is this really a security issue? It causes lots of confusion with downloaded 
filenames. See bug 60203.
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. 
this is similar to 63225
Status: RESOLVED → VERIFIED
QA Contact: tever → benc
qa to me.

VERIFIED, per Sarah.
Blocks: advocacybugs
No longer blocks: advocacybugs
This security fix was reversed in bug 60203.
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).
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.
Depends on: 369428
Component: Networking → File Handling
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.