81246 - pk12util uses hardcoded r-only /tmp/Pk12uTemp file

Status: RESOLVED FIXED

(NSS :: Tools, defect, P1)

Description

[Sonja Mirtitsch]

if one program terminates without removing the file a different user can never
use pk12util -i on this machine again. The errormessage is missleading:


$ pk12util -i Alice.p12 -d ../tools/copydir -k ../tests.pw.20169 -w
../tests.pw.20169
pk12util: PKCS12 decode not verified: security library: improperly formatted
DER-encoded message.

the QA scripts work around the problem by checking for the existance of this
file after pk12util return code indicates error and outputing the following message:
Error: pk12util temp file exists. Please remove this file and
       rerun the test (/tmp/Pk12uTemp)

Comment 1

[Wan-Teh Chang]

Seems like this may also prevent two people from running
pk12util at the same time on the same machine.

Assigned the bug to Ian.

Comment 2

[Sonja Mirtitsch]

It does prevent 2 people from running it at the exact same time, but it is so
fast that I think this doesn't matter. I had a hard time catching the name of
the file, I did an ls /tmp/Pk12* in a loop, and only the 2nd run of pk12util got
the ls loop to give me the message "/tmp/Pk12uTemp does not exist" without ever
having listed the  name of the file. 

Comment 3

[Wan-Teh Chang]

This bug should be fixed, or at least make pk12util issue
a meaningful error message when /tmp/Pk12uTemp exists.

Comment 4

[Sonja Mirtitsch]

A meaningfull errormessage will not help. The scripts work around the problem at 
the moment and already give the errormesage:

Error: pk12util temp file exists. Please remove this file and
       rerun the test (/tmp/Pk12uTemp)

As a 2nd workaround I put the removal of the Pk12uTemp file into the script 
qaclean and added the option -all which will clean up everything on the most 
commonly used QA machines

Comment 5

[Wan-Teh Chang]

Assigned the bug to Julien.

Comment 6

[Julien Pierre]

Attachment: proposed patch

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

please use the "-u" argument for ALL diffs used as patches.
That is, use "cvs diff -u filename".  Those patches are usually
much easier to read and verify.
This is a mozilla convention, not merely an NSS convention.

Comment 8

[Julien Pierre]

Attachment: pk12util patch with cvs diff -u

Comment 9

[Julien Pierre]

As discussed in the meeting yesterday, I have modified the patch to pre-allocate
a buffer if needed . I have set the initial size to 4k. If needed the buffer
will still be reallocated, in increments of 4k . I am attaching the patch.

Ian, could you please review the code ? Thanks.

Comment 10

[Julien Pierre]

Attachment: proposed patch with initial buffer allocated

Comment 11

[Wan-Teh Chang]

Comment on attachment 52156 [details] [diff] [review]
proposed patch with initial buffer allocated

Julien,

As we discussed at the meeting yesterday, it is
preferrable to use PORT_Alloc, PORT_Realloc, and
PORT_Free (declared in secport.h).

>@@ -139,7 +139,7 @@
>     return p12cxt;
> }
> 
>-#define TEMPFILE "Pk12uTemp"
>+#define DefaultTempSize 4096

Macros should be all caps.  So this should be:
#define DEFAULT_TEMP_SIZE 4096


>-    return PR_Write(p12cxt->file, buf, len);
>+    if (p12cxt->currentpos+len > p12cxt->filesize) {
>+        p12cxt->filesize = p12cxt->currentpos + len;
>+    }
>+    else
>+    {
>+        p12cxt->filesize += len;
>+    }
>+    if (p12cxt->filesize > p12cxt->allocated) {
>+        p12cxt->allocated = p12cxt->filesize + DefaultTempSize;
>+        p12cxt->buffer = realloc(p12cxt->buffer, p12cxt->allocated);
>+    }
>+    PR_ASSERT(p12cxt->buffer);

realloc may fail and return NULL.  Therefore, you should save
the return value of realloc in a local variable, otherwise
the old value of p12cxt->buffer will be lost if realloc fails,
resulting in a memory leak.

The last if block and the PR_ASSERT should be replaced by this:

      if (p12cxt->filesize > p12cxt->allocated) {
          void *newbuffer;
          size_t newsize = p12cxt->filesize + DEFAULT_TEMP_SIZE;
          newbuffer = PORT_Realloc(p12cxt->buffer, newsize);
          if (newbuffer == NULL) {
              return -1;  /* can't extend the buffer */
          }
          p12cxt->buffer = newbuffer;
          p12cxt->allocated = newsize;
      }

I am wondering if we should use a linked list of 4K buffers,
as opposed to realloc'ing whenever we run out of space in
the buffer.

>Index: pk12util.h
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/cmd/pk12util/pk12util.h,v
>retrieving revision 1.1
>diff -u -r1.1 pk12util.h
>--- pk12util.h	2000/04/03 18:56:53	1.1
>+++ pk12util.h	2001/10/05 00:45:36
>@@ -62,9 +62,13 @@
> 
> /* additions for importing and exporting PKCS 12 files */
> typedef struct p12uContextStr {
>-    char        *filename;    /* name of file */
>+	PRBool      inMemory;     /* whether this file is in memory or real */
>+	char        *filename;    /* name of file */
> 	PRFileDesc  *file;        /* pointer to file */
>-	PRBool       error;       /* error occurred? */
>-	int          errorValue;  /* which error occurred? */
>-	SECItem     *data;
>+	void        *buffer;      /* storage area */
>+	PRInt32     filesize;     /* file size */
>+        PRInt32     allocated;    /* total buffer size allocated */
>+	PRInt32     currentpos;   /* position counter */
>+	PRBool      error;        /* error occurred? */
>+	int         errorValue;   /* which error occurred? */
> } p12uContext;

If we decide not to use a temporary file, the inMemory flag
is not necessary.  The filename, file, and filesize fields
can probably be deleted too.

Comment 12

[Julien Pierre]

Attachment: patch incorporating wtc's changes

Comment 13

[Ian McGreer]

I hate to throw this in this late in the game, but it seems to me this
functionality should actually be part of the PKCS#12 library.  We're talking
about this being a "reference implementation" for PSM; why can't it be the
default implementation?  I think the buffered open/close/read/write functions
should be put in lib/pkcs12, and used as defaults if the caller of
SEC_PKCS12DecoderStart does not supply callbacks.  The state variables needed to
manage the memory buffers would need to be moved into SEC_PKCS12DecoderContext
(in a way that doesn't affect someone who actually does override the defaults).

Does everyone else agree?  Otherwise, every user of our PKCS#12 library will
need to copy out code from pk12util to make it work (which is where we are at
now - the same undesirable code is in both pk12util and PSM).

-Ian

Comment 14

[Julien Pierre]

Wan-Teh,

I implemented your suggestions. As far as removing fields, the p12uContext
structure is used for several purposes - one of them writing the P12 file during
export - so the file field cannot really be removed, unless two distinct
structures are created.

As far as using a linked list of buffers, I'm not sure if this is necessary. It
could be if we have a platform where realloc is not working reliably.

Ian,

Your late comments make sense. It should indeed be possible to preserve the API
the way it is and create the memory buffer when SEC_PKCS12DecoderStart is called
within the SEC_PKCS12DecoderContext structure with no callbacks and no object
pointer, freeing it only when SEC_PKCS12DecoderFinish is called.
The fix for PSM and other apps at that point becomes much easier. If everybody
else agres I will move the code there.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Pk12util contains a function to convert ascii to unicode, which is 
necessary for certain operations.  IINM, this code duplicates another
function in libNSS that does the same thing.  That duplication should
be eliminated, and the functions in libnss should be used.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

It may be appropriate for a util program like pk12util to read the
entire pfx file into memory, and to duplicate it into memory again
as part of the decoding process, but the implementation in libnss
probably ought not burden every program that uses it with that large
memory requirement.  IMO.

Comment 17

[Ian McGreer]

I removed the unicode conversion functions in pk12util a long time ago, except
for  UCS2 to ASCII conversion.  This function actually uses NSS's
sec_port_ucs2_utf8_conversion_function, but with some additional swapBytes
logic.  That seemed to work, so I didn't touch it.  NSS does not have a default
UCS2 to ASCII conversion function.

I don't think having a default buffering implementation will burden users of
PKCS#12.  Most users will probably want to use a memory-buffered approach, this
removes the burden of them having to code it.  The function parameters are not
going to change, so a user still has the prerogative to supply his own
callbacks.  No current user of NSS will be affected by doing this, unless he
changes all of his callback arguments to NULL.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Thanks Ian.  Maybe NSS should have an UCS2->ASCII conversion function.
the PKCS#12 stuff will need it, I believe.

Comment 19

[Julien Pierre]

Nelson,

How is the UCS /ASCII issue related to this bug ? I believe the buffering to
memory vs disk is independent of that, so it should probably be discussed in a
separate bug. Just making sure.

Comment 20

[Julien Pierre]

Attachment: proposed patch for lib/pkcs12

Comment 21

[Julien Pierre]

Attachment: proposed patch for pk12util to use the default in-memory decoder implementation

Comment 22

[Julien Pierre]

I have just attached two new patches. Basically the in-memory implementation has
been moved to lib/pkcs12 .
The patch for pk12util mainly deletes a bunch of code of the old implementation
to create and manage the temp file, and simply calls the decoder with NULL args.
I have tested it and it works fine. Let me know if it is all good to check in.

Comment 23

[Julien Pierre]

Ian,

I'd appreciate if you could review this latest code. It incorporates your
suggestions and I believe the default handlers will be good enough for most
apps. Thanks.

Comment 24

[Stephane Saux]

Cross referrence to PSM bug 101996

Comment 25

[Ian McGreer]

Comment on attachment 52299 [details] [diff] [review]
proposed patch for lib/pkcs12

>Index: p12d.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/pkcs12/p12d.c,v
>retrieving revision 1.7
>diff -u -r1.7 p12d.c
>--- p12d.c	2001/09/20 21:38:24	1.7
>+++ p12d.c	2001/10/05 23:46:37
>@@ -135,6 +135,12 @@
> 
>     /* import information */
>     PRBool bagsVerified;
>+
>+    /* buffer management for the default callbacks implementation */
>+    void        *buffer;      /* storage area */
>+    PRInt32     filesize;     /* actual data size */
>+    PRInt32     allocated;    /* total buffer size allocated */
>+    PRInt32     currentpos;   /* position counter */
> };
> 
> 
>@@ -1019,6 +1025,112 @@
>     p12dcx->error = PR_TRUE;
> }
> 
>+/*  default implementations of the open/close/read/write functions for
>+    SEC_PKCS12DecoderStart 
>+*/
>+
>+#define DEFAULT_TEMP_SIZE 4096
>+
>+static SECStatus
>+p12u_DigestOpen(void *arg, PRBool readData)
>+{
>+    SEC_PKCS12DecoderContext* p12cxt = arg;
>+
>+    p12cxt->currentpos = 0;

Probably should also set p12cxt->filesize = 0.

>+
>+    if (PR_FALSE == readData) {
>+        /* allocate an initial buffer */
>+        p12cxt->allocated = DEFAULT_TEMP_SIZE;
>+        p12cxt->buffer = PORT_Alloc(DEFAULT_TEMP_SIZE);
>+    }
>+    else
>+    {
>+        PR_ASSERT(p12cxt->buffer);
>+        if (!p12cxt->buffer) {
>+            return SECFailure; /* no data to read */
>+        }
>+    }
>+
>+    return SECSuccess;
>+}
>+
>+static SECStatus
>+p12u_DigestClose(void *arg, PRBool removeFile)
>+{
>+    SEC_PKCS12DecoderContext* p12cxt = arg;
>+
>+    PR_ASSERT(p12cxt);
>+    if (!p12cxt) {
>+        return SECFailure;
>+    }
>+    p12cxt->currentpos = 0;
>+
>+    if (PR_TRUE == removeFile) {
>+        PR_ASSERT(p12cxt->buffer);
>+        if (!p12cxt->buffer) {
>+            return SECFailure;
>+        }
>+        if (p12cxt->buffer) {
>+            PORT_Free(p12cxt->buffer);
>+            p12cxt->buffer = NULL;
>+            p12cxt->allocated = 0;
>+            p12cxt->filesize = 0;
>+        }
>+    }
>+
>+    return SECSuccess;
>+}
>+
>+static int
>+p12u_DigestRead(void *arg, unsigned char *buf, unsigned long len)
>+{
>+    int toread = len;
>+    SEC_PKCS12DecoderContext* p12cxt = arg;
>+
>+    if(!buf || len == 0) {
>+	return -1;
>+    }
>+
>+    if (!p12cxt->buffer || ((p12cxt->filesize-p12cxt->currentpos)<len) ) {
>+        /* trying to read past the end of the buffer */
>+        toread = p12cxt->filesize-p12cxt->currentpos;
>+    }
>+    memcpy(buf, (void*)((char*)p12cxt->buffer+p12cxt->currentpos), toread);
>+    p12cxt->currentpos += toread;
>+    return toread;
>+}
>+
>+static int
>+p12u_DigestWrite(void *arg, unsigned char *buf, unsigned long len)
>+{
>+    SEC_PKCS12DecoderContext* p12cxt = arg;
>+
>+    if(!buf || len == 0) {
>+        return -1;
>+    }
>+
>+    if (p12cxt->currentpos+len > p12cxt->filesize) {
>+        p12cxt->filesize = p12cxt->currentpos + len;
>+    }
>+    else {
>+        p12cxt->filesize += len;
>+    }
>+    if (p12cxt->filesize > p12cxt->allocated) {
>+        void* newbuffer;
>+        size_t newsize = p12cxt->filesize + DEFAULT_TEMP_SIZE;
>+        newbuffer  = PORT_Realloc(p12cxt->buffer, p12cxt->allocated);

Don't you mean newsize instead of p12cxt->allocated?

>+        if (NULL == newbuffer) {
>+            return -1; /* can't extend the buffer */
>+        }
>+        p12cxt->buffer = newbuffer;
>+        p12cxt->allocated = newsize;
>+    }
>+    PR_ASSERT(p12cxt->buffer);
>+    memcpy((void*)((char*)p12cxt->buffer+p12cxt->currentpos), buf, len);
>+    p12cxt->currentpos += len;
>+    return len;
>+}
>+
> /* SEC_PKCS12DecoderStart
>  *	Creates a decoder context for decoding a PKCS 12 PDU objct.
>  *	This function sets up the initial decoding context for the
>@@ -1039,6 +1151,9 @@
>  *		 routines.
>  *	dArg - the argument for dOpen, etc.
>  *
>+ *      if NULL == dOpen == dClose == dRead == dWrite == dArg, then default
>+ *      implementations using a memory buffer are used
>+ *
>  *	This function returns the decoder context, if it was successful.
>  *	Otherwise, null is returned.
>  */
>@@ -1062,6 +1177,16 @@
> 	PORT_SetError(SEC_ERROR_NO_MEMORY);
> 	goto loser;
>     }
>+
>+    if (!dOpen && !dClose && !dRead && !dWrite && !dArg) {
>+        /* use default implementations */
>+        dOpen = p12u_DigestOpen;
>+        dClose = p12u_DigestClose;
>+        dRead = p12u_DigestRead;
>+        dWrite = p12u_DigestWrite;
>+        dArg = (void*)p12dcx;
>+    }
>+
>     p12dcx->arena = arena;
>     p12dcx->pwitem = pwitem;
>     p12dcx->slot = (slot ? slot : PK11_GetInternalKeySlot());

Comment 26

[Ian McGreer]

Comment on attachment 52300 [details] [diff] [review]
proposed patch for pk12util to use the default in-memory decoder implementation

could you remove the p12u_CreateTemporaryDigestFile function from pk12util.c?  It isn't being called anymore, and the patch reduces it to an allocation of an object that does nothing.

Comment 27

[Julien Pierre]

Ian,

for lib/pkcs12 :

p21cxt->filesize needs to be reset only in the code that creates the file the
first time and that's presumed to be the case because the decoder context is
zallocated.  When opening the file for read we lose track of how much data
actually is in the buffer if filesize is reset. Nevertheless I'm forcing
filesize to be reset to zero now, in case somebody wanted to reuse the same
decoder context for a different file.

As far as the allocation, yes, I meant newsize, this is fixed now. Thanks.

I have also removed the unnecessary code from pk12util.c . 

New patches are in to reflect these changes.

Comment 28

[Julien Pierre]

Attachment: pk12util.c update

Comment 29

[Julien Pierre]

Attachment: lib/pkcs12 fix update

Comment 30

[Ian McGreer]

Comment on attachment 52545 [details] [diff] [review]
pk12util.c update

looks good.

Comment 31

[Ian McGreer]

Comment on attachment 52547 [details] [diff] [review]
lib/pkcs12 fix update

rock on.

Comment 32

[Julien Pierre]

All checked in :

Checking in pk12util.c;
/cvsroot/mozilla/security/nss/cmd/pk12util/pk12util.c,v  <--  pk12util.c
new revision: 1.18; previous revision: 1.17
done
Checking in pk12util.h;
/cvsroot/mozilla/security/nss/cmd/pk12util/pk12util.h,v  <--  pk12util.h
new revision: 1.2; previous revision: 1.1
done

Checking in p12d.c;
/cvsroot/mozilla/security/nss/lib/pkcs12/p12d.c,v  <--  p12d.c
new revision: 1.8; previous revision: 1.7
done