Closed Bug 101996 Opened 19 years ago Closed 15 years ago

temp file with guessable name used during p12 import.

Categories

(Core Graveyard :: Security: UI, defect, P1)

1.0 Branch
x86
Windows 2000
defect

Tracking

(Not tracked)

RESOLVED FIXED
psm2.2

People

(Reporter: ssaux, Assigned: KaiE)

Details

(Keywords: fixed1.8.1, Whiteboard: [kerh-coz])

Attachments

(1 file, 1 obsolete file)

See: 
http://www.securityfocus.com/bid/1201

Here's the place that Ian McGreer found where the temp file is created.
http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsPKCS12Blob.cpp#509
adding PDT to whiteboard, and nsbranch+ to keywords.
this bug is a result of a security firedrill.
Keywords: nsbranch+
Priority: -- → P1
Whiteboard: PDT
Target Milestone: --- → 2.1
Note to PDT: this may not be a stop-ship, but while we're doing the due
dilligence on this firedrill, I want the bug to be noticed.
What are the chances of getting this one for the 0.9.4 branch.
Whiteboard: PDT → [PDT] [ETA ?]
We're waiting for more information from CERT.

One thing that is certain is that an attacker needs access to the machine.

Known files related to the cert db in the /tmp directory contain indexes of
stuctures in memory, not data.

Changing this data would most probably result in a crash.  Reading this data is
only useful if the attacker is root and can manipulate the memory, and it the
attacker is root, the victim has other problems to worry about.

We still wish to wait for more information.
thanks for the update ...
Not a stop-ship for 0.9.4 branch.
Removing PDT, nsbranch+
->future.
Keywords: nsbranch+
Whiteboard: [PDT] [ETA ?]
Target Milestone: 2.1 → Future
The benchmark for what PSM needs to do to resolve this issue is being developped
as a part of the fix to NSS bug 81246.
Target Milestone: Future → 2.2
target to 2.2
nsbeta1
Keywords: nsbeta1
nsbeta1+
Keywords: nsbeta1nsbeta1+
is 2.2 dropiing into MachV?
to kai
Assignee: ddrinan → kaie
Bob, Ian, during PKCS12 file encoding / decoding, a temp file is used.

Can you please give me a hint what kind of data is stored in this "digest" file?

It seems, I can define my own file I/O routines. I should implement a small
automatic buffer, that can be accessed like a file, but lives in memory solely.
Status: NEW → ASSIGNED
Kai,

If you read bug 81246, you'll see that NSS now employs a memory-buffered
approach by default, instead of a temp file.  You should use pk12util.c as a
reference.  You should be able to pass NULL to all the file I/O parameters.

AFAIK, what is stored is just digests.  That doesn't seem to revealing to me (it
should be Very Difficult to reconstruct the message from the digest).  However,
maybe the digests reveal some info, Bob or Julien are probably more familiar
with PKCS#12 than I.
Based on Ian's comment I nsbeta- this bug.
Keywords: nsbeta1+nsbeta1-
Keywords: nsbeta1
Keywords: nsbeta1
Keywords: nsbeta1-
Keywords: nsbeta1-
QA Contact: junruh → bmartin
Product: PSM → Core
Do you think this is still an issue?
Should I find out where this temporary file is being created, or whether this bug has become invalid?
Too bad the source code reference in comment 0 is a rotted lxr link, people better used function names... 
This is from the revision of the file on the day this
bug report was opened:

   502  // digest_open
   503  // open a temporary file for reading/writing digests
   504  SECStatus PR_CALLBACK
   505  nsPKCS12Blob::digest_open(void *arg, PRBool reading)
   506  {
   507    nsPKCS12Blob *cx = (nsPKCS12Blob *)arg;
   508    nsresult rv;
   509    // use DirectoryService to find the system temp directory
   510    nsCOMPtr<nsILocalFile> tmpFile;
   511    nsCOMPtr<nsIProperties> directoryService =
   512             do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID, &rv);
   513    if (NS_FAILED(rv)) return SECFailure;
   514    directoryService->Get(NS_OS_TEMP_DIR,
   515                          NS_GET_IID(nsIFile),
   516                          getter_AddRefs(tmpFile));
   517    if (tmpFile) {
   518      tmpFile->Append(PIP_PKCS12_TMPFILENAME);
   519      nsXPIDLCString pathBuf;
   520      tmpFile->GetPath(getter_Copies(pathBuf));
   521      cx->mTmpFilePath = PL_strdup(pathBuf.get());

So I believe the method in question is nsPKCS12Blob::digest_open,
and the current LXR link is
http://lxr.mozilla.org/seamonkey/source/security/manager/ssl/src/nsPKCS12Blob.cpp#592.

I don't know how to construct an LXR link for that method.
Thanks a lot for your research Wan-Teh!
Attached patch Patch v1 (obsolete) — Splinter Review
I looked at the temporary file that got created.
It contains the data from the input file, and some additional bytes.

While the file contents are encrypted, I still think we should not place the user's private data in this temporary, public location.

This patch changes our code to use a buffer in memory.
Attachment #202493 - Flags: review?(wtchang)
Whiteboard: [kerh-coz]
Comment on attachment 202493 [details] [diff] [review]
Patch v1

Patch looks very good with the following caveats:

1)digest_open should ensure mDigest is not NULL when reading.

2) these are NSS callback functions, so they should set the appropriate PORT_SetError when the fail.
Attachment #202493 - Flags: review?(wtchang) → review?(rrelyea)
Comment on attachment 202493 [details] [diff] [review]
Patch v1

r+ provided you do the ensure on mDigest on read.

I would also like to see the PORT_SetError() as well, but it's not a requirement for landing.
Attachment #202493 - Flags: review?(rrelyea) → review+
Attached patch Patch v2Splinter Review
Bob, in this new patch I'm checking that mDigest is true when we open for reading, and within the read callback.

Regarding PORT_SetError, I've added to calls that set SEC_ERROR_NO_MEMORY when we fail to allocate.

However, all other null checks would be the result of inappropriate function calls, like reading or reading without succesful open. So I have not added error codes here. Let me know if you want to suggest an error code.
Attachment #202493 - Attachment is obsolete: true
Attachment #203019 - Flags: review?(rrelyea)
Comment on attachment 203019 [details] [diff] [review]
Patch v2

The correct error code for the other cases is SEC_ERROR_LIBRARY_FAILURE since your code is called only from NSS, and such an error would only be the result of a coding error at the previous level.

Patch is good enough to go in.

bob
Attachment #203019 - Flags: review?(rrelyea) → review+
Attachment #203019 - Flags: superreview?(dveditz)
Attachment #203019 - Flags: superreview?(dveditz)
Patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #203019 - Flags: branch-1.8.1+
Keywords: fixed1.8.1
Version: psm2.1 → 1.0 Branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.