Closed
Bug 302663
Opened 20 years ago
Closed 20 years ago
SECKEY_CopySubjectPublicKeyInfo uses bit count as byte count
Categories
(NSS :: Libraries, defect)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.11
People
(Reporter: mi+mozilla, Assigned: wtc)
Details
Attachments
(3 files)
20.20 KB,
image/png
|
Details | |
24.63 KB,
text/plain
|
Details | |
1.23 KB,
patch
|
julien.pierre
:
review+
nelson
:
superreview+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•20 years ago
|
||
Reporter | ||
Comment 2•20 years ago
|
||
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.
Assignee | ||
Comment 3•20 years ago
|
||
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 4•20 years ago
|
||
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.
Reporter | ||
Comment 5•20 years ago
|
||
=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•20 years ago
|
||
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.
Reporter | ||
Comment 7•20 years ago
|
||
=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•20 years ago
|
||
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 ...
Assignee | ||
Comment 9•20 years ago
|
||
Mikhail: could you test this patch? Thanks.
Attachment #191008 -
Flags: superreview?(nelson)
Attachment #191008 -
Flags: review?(julien.pierre.bugs)
Comment 10•20 years ago
|
||
Comment on attachment 191008 [details] [diff] [review]
Proposed patch
Looks right to me.
Attachment #191008 -
Flags: superreview?(nelson) → superreview+
Updated•20 years ago
|
Summary: Purify reports Array Bounds Read in secitem.c → SECKEY_CopySubjectPublicKeyInfo uses bit count as byte count
Reporter | ||
Comment 11•20 years ago
|
||
(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?
Assignee | ||
Comment 12•20 years ago
|
||
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.)
Reporter | ||
Comment 13•20 years ago
|
||
(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•20 years ago
|
||
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.
Updated•20 years ago
|
Attachment #191008 -
Flags: review?(julien.pierre.bugs) → review+
Assignee | ||
Comment 15•20 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.11
Comment 16•20 years ago
|
||
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.
Reporter | ||
Comment 17•20 years ago
|
||
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!
You need to log in
before you can comment on or make changes to this bug.
Description
•