302663 - SECKEY_CopySubjectPublicKeyInfo uses bit count as byte count

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Mikhail T.]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.4; FreeBSD) KHTML/3.4.1 (like Gecko)
Build Identifier: 

This error is benign -- garbage source is copied into garbage target. But the 
warning may mask other problems. For example, if something were to use the 
garbage target, Purify will not be able to warn us about the use of unitialized 
memory. 
 
Apparently, when the certificate is created, the "len" field of its subject 
remains set to the maximum length, instead of the actual legnth of the 
initialized data. 
 
The problem is here shows up here: ABR in nss/lib/util/secitem.c:240 : 
 
            memcpy         
[nss-3.10/mozilla/security/nss/cmd/certutil/SunOS5.9_purify_64_DBG.OBJ/rtlib.o] 
            SECITEM_CopyItem 
[nss-3.10/mozilla/security/nss/lib/util/secitem.c:240] 
                       if (!to->data) { 
                           return SECFailure; 
                       } 
            =>         PORT_Memcpy(to->data, from->data, from->len); 
                       to->len = from->len; 
                   } else { 
                       to->data = 0; 
            SECKEY_CopySubjectPublicKeyInfo 
[nss-3.10/mozilla/security/nss/lib/cryptohi/seckey.c:305] 
                
                   rv = SECOID_CopyAlgorithmID(arena, &to->algorithm, 
&from->algorithm); 
                   if (rv == SECSuccess) 
            =>         rv = SECITEM_CopyItem(arena, &to->subjectPublicKey, 
&from->subjectPublicKey); 
                
                   return rv; 
               } 
            CERT_CreateCertificate 
[nss-3.10/mozilla/security/nss/lib/certhigh/certreq.c:118] 
            MakeV1Cert     
[nss-3.10/mozilla/security/nss/cmd/certutil/certutil.c:1398] 
            CreateCert     
[nss-3.10/mozilla/security/nss/cmd/certutil/certutil.c:2169] 
            certutil_main  
[nss-3.10/mozilla/security/nss/cmd/certutil/certutil.c:2945] 
            main           
[nss-3.10/mozilla/security/nss/cmd/certutil/certutil.c:3099] 
            _start         [/opt/SUNWspro/WS6U2/lib/v9/crt1.o] 
 
 
I'll attach Purify's window dump for your convenience. 

Reproducible: Always

Comment 1

[Mikhail T.]

Attachment: Window dump of Purify diagnostics

Comment 2

[Mikhail T.]

Attachment: Full log of Purify diagnostics

This is a complete Purify log of the command:

certutil -C -c chain-2-serverCA -m 200 -v 60 -d \
	  serverCA -i req -o beaker.us.murex.com.cert -f ../tests.pw.9218

Showing this and earlier reported errors/warnings.

Comment 3

[Wan-Teh Chang]

Good catch.

The problem is that the SECItem subjectPublicKey
in CERTSubjectPublicKeyInfo is a bit string, so the "len"
field is the length in bits.  SECKEY_CopySubjectPublicKeyInfo
need to convert that to length in bytes when copying the
"data" buffer.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

As part of the NSS release process, we do purify testing that is intended 
to cover libSMIME, libSSL, and the portions of libNSS and libFreebl used
by libSMIME and libSSL.  We use various NSS test programs to achieve that
purify testing, but certutil is not among those that are presently purified.

So, I gather that  SECKEY_CopySubjectPublicKeyInfo is not among the functions
covered by the existing purify testing.  Perhaps the bug reporter would care
to suggest a "script" of utility program commands to purify.

Comment 5

[Mikhail T.]

=Perhaps the bug reporter would care to suggest a 
="script" of utility program commands to purify. 
 
How about the most obvious choice: 
 
     nss-3.10/mozilla/security/nss/tests/all.sh 
 
? 
 
I compiled the whole tree with "purify cc" instead of "cc" and ran the above 
script. 

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

There have been MANY problems reported to us associated with trying to 
run purify builds of NSS that use the v8+ or v9 code.  I'm talking about
hangs or crashes (not purify warngings) in code that runs fine without
purify.  We've received reports that the only way to succesfully run 
NSS with purify is to use the pure v8 code (not v8+ or v9).  

It sounds like you may have a version of purify that works better than
ours on v8+/v9 CPUs.  We'd appreciate any info you can supply about your 
version of purify that might enable us to use purify on v8+ and v9 code.

Comment 7

[Mikhail T.]

=We'd appreciate any info you can supply about your version  
=of purify that might enable us to use purify on v8+ and v9 code.  
  
This is what we use today on SunOS beaker 5.9 Generic_118558-09 sun4u sparc 
SUNW,Sun-Fire-V440: 
      Version 2003a.06.15 Solaris 2 
 
And on Linux 2.4.20-31.9bigmem #1 SMP:  
      Version 2003a.06.13 FixPack 0172 041231 Linux (32-bit) 
  
To my knowledge, it is either the latest or very recent version 
currently available. For registered users the upgrades are free.  
  
Many of the problems I reported yesterday and today are not even v8+ or v9  
specific. If you could run tests/all.sh of the most vanilla Linux or Solaris  
(v8) builds through Purify, you would've caught these before release... 

Comment 8

[Julien Pierre]

Mikhail,

This is the same Purify for Solaris release that we have had problems with here .
shlibsign will not succeed with it during the NSS build - it hangs at 100% CPU
and stays there for several days ...

Comment 9

[Wan-Teh Chang]

Attachment: Proposed patch

Mikhail: could you test this patch?  Thanks.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 191008 [details] [diff] [review]
Proposed patch

Looks right to me.

Comment 11

[Mikhail T.]

(In reply to comment #8)      
> Mikhail,      
>       
> This is the same Purify for Solaris release that we have had problems with      
here .      
> shlibsign will not succeed with it during the NSS build - it hangs at 100%      
CPU      
> and stays there for several days ...      
      
Interesting. What could be different about my setup is compiling for debugging      
and 64-bit. The nspr-4.6 was also compiled without any optimization. I used      
Solaris' own -lz (but your dbm, for which you seem to have forgotten to include      
the build glue, BTW -- there is mozilla/dbm alright, but no      
mozilla/security/dbm).      
      
Also the machine is Solaris-9 patched up to the latest 9_Recommended.zip as of      
June 27th.     
     
(In reply to comment #9)     
> Mikhail: could you test this patch?     
     
The patch looks fine as a work-around, but a proper fundamental fix, probably,     
should change SECITEM_CopyItem() to interpret the SECItem.data based on the     
SECItem.type. Something along the lines of:   
    
     size_t size;   
   
     switch (from->type) {    
     case siDERNameBuffer:  
     case ?????: 
          size = from->len/8;   
     default:   
          size = from->len;   
   
     PORT_Memcpy(to->data, from->data, size);   
     to->len = from->len;  
  
This seems like a better way to skin this poor animal... No? 

Comment 12

[Wan-Teh Chang]

Comment on attachment 191008 [details] [diff] [review]
Proposed patch

Mikhail: you are right.  That would be the right way to
fix this bug.  But we can't fix this bug that way because
it will break similar workarounds that NSS customers have
implemented.

NSS guarantees backward compatibility.	This bug is an
imperfection we have to live with.

We can consider adding a new variant of SECITEM_CopyItem
(for example, SECITEM_CopyItemWithType) that handles a bit
string correctly.  (A prerequisite is to add a new
SECItemType enum constant for bit strings.)

Comment 13

[Mikhail T.]

(In reply to comment #12) 
> (From update of attachment 191008 [details] [diff] [review] [edit]) 
> But we can't fix this bug that way because it will break similar 
> workarounds that NSS customers have implemented. 
 
Oh, so you don't want to divide the length by 8 again?.. 
 
> NSS guarantees backward compatibility.  
> This bug is an imperfection we have to live with. 
 
Well, at some point you have to "stop perpetuating" a bug and bite the bullet. 
Being "bugs-compatible" is wrong. SECItem_Copy() copies an item, period :-) 
 
Personally, I'd say, that a customer, who implemented a workaround _without 
filing a bug report_ have to blame themselves... If they don't care to give 
even so little (as a bug report) back to a free software vendor, they can 
unimplement their workaround on their own. 
 
I doubt, there are many such people, however. Copying too much is not usually 
destructive, so few people (if any) have noticed this... Just mention it in 
RELEASE NOTES and move on :-) 

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

SECItem's type field was added to SECItem as an afterthought.  
It was intended as an aid to leak analysis, before more modern tools 
were available for that.  It was intended to indicate what sort of 
encoded data structure is contained in the buffer, e.g. an SSL record,
an SMIME component, a particular component of a certificate, etc.  
so that one might be able to track down and find the code path that 
allocated it.  It wa salso considered as a possible source of information
from which to choose a free list for the buffer, although that was not 
implemented.  It was never intended to be used as an indicator of the
units of the len field (bits, bytes, ints).  

Because it was an afterthought, and because it was intended as a debugging
aid, no effort was ever made to go through all of NSS and ensure that every
SECItem created had an appropriate "type" value, or even that the SECItemType
enum had symbols defined for every type of data structure.  

When we started using purify years ago, the largest single source of UMRs 
was the type field in SECItems being uninitialized.  So, an effort to silence 
those UMRs was undertaken, but it merely set the type member to siBuffer in 
all places where no value had been assigned.  So, today it is rather 
worthless as a real buffer type indicator.  

Today, the type field has many various ad-hoc uses, inconsistent with that 
original type meaning.  It is often used in templates for the various ASN.1 
encoders and decoders as a place to store discriminant values for CHOICEs, 
or to inidcate that the INTEGER value to be encoded is signed or unsigned.  

Now there is a suggestion for another use of the SECItem's type member, 
namely as an indicator of the units of the len member (bits, bytes, other).
That is another choice that the designers of SECItem did not choose.  
We may argue, after the fact, that they should have chosen so, but we cannot
say it is a bug that they made another choice.  
If were to consider having a member of SECItem dedicated to len unit indication,
the choices would be:
a) add another member to the struct, resulting in a large binary incompatibility
for existing binaries that use NSS,
b) evict all other existing uses of the type member, forcing them to find 
another home.  The difficulty of that very problem is the reason why those
ad-hoc uses exist in the first place.

Several things is clear.  
1) To change NSS to use type as a len unit indicator would break many of its
existing uses.  
2) Today, most functions that handle SECItems disregard the type value.  
Much code in NSS and in NSS-based applications depends on the present behavior.
To change those functions to begin to operate differently based on type value
would introduce binary incompatiblity.

Wan-Teh is right that the preservation of binary compatibility demands that 
we create NEW functions that use type in this fashion, and change code to 
call those new functions where desired.  

For now, code must continue to "know" when it is dealing with a bit string 
and must perform the appropriate length conversions, using the supplied
macros and functions, as necessary.  

Comment 15

[Wan-Teh Chang]

I checked in the patch on the NSS trunk for NSS 3.11.

Checking in seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.35; previous revision: 1.34
done

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

I think we should also fix this on the 3.10 branch if/when we get the 
issues with that worked out.  I'm somewhat optimistic that will be soon.

Comment 17

[Mikhail T.]

Maybe, a type-aware version of  SECITEM_CopyItem (CopyTypedItem or 
CopyItemWithType) should be introduced _now_, so that the conversion to the 
better way of doing things can begin and the "dumb" SECITEM_CopyItem can be 
marked obsoleted :-) 
 
This will allow you to get rid of the SECITEM_CopyItem altogether by release 
4.0 or some such. 
 
And, once again, I urge you to make any changes deemed worthy of being 
committed into the already released sources available right next to the 
released tarballs. Thanks!