101996 - temp file with guessable name used during p12 import.

Status: RESOLVED FIXED

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

Description

[Stephane Saux]

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

Comment 1

[Stephane Saux]

adding PDT to whiteboard, and nsbranch+ to keywords.
this bug is a result of a security firedrill.

Comment 2

[Stephane Saux]

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.

Comment 3

[Jaime Rodriguez, Jr.]

What are the chances of getting this one for the 0.9.4 branch.

Comment 4

[Stephane Saux]

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.

Comment 5

[Jaime Rodriguez, Jr.]

thanks for the update ...

Comment 6

[Stephane Saux]

Not a stop-ship for 0.9.4 branch.
Removing PDT, nsbranch+
->future.

Comment 7

[Stephane Saux]

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.

Comment 8

[Stephane Saux]

target to 2.2

Comment 9

[John Unruh]

nsbeta1

Comment 10

[Stephane Saux]

nsbeta1+

Comment 11

[Jaime Rodriguez, Jr.]

is 2.2 dropiing into MachV?

Comment 12

[Stephane Saux]

to kai

Comment 13

[Kai Engert [: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.

Comment 14

[Ian McGreer]

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.

Comment 15

[Stephane Saux]

Based on Ian's comment I nsbeta- this bug.

Comment 16

[Kai Engert [:KaiE:]]

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... 

Comment 17

[Wan-Teh Chang]

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.

Comment 18

[Kai Engert [:KaiE:]]

Thanks a lot for your research Wan-Teh!

Comment 19

[Kai Engert [:KaiE:]]

Attachment: Patch v1

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.

Comment 20

[Robert Relyea]

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.

Comment 21

[Robert Relyea]

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.

Comment 22

[Kai Engert [:KaiE:]]

Attachment: Patch v2

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.

Comment 23

[Robert Relyea]

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

Comment 24

[Kai Engert [:KaiE:]]

Patch checked in.