Closed
Bug 75049
Opened 24 years ago
Closed 23 years ago
Changes needed for NSS to build under Carbon
Categories
(Core Graveyard :: Security: UI, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: sdagley, Assigned: beard)
References
Details
(Whiteboard: OSX++, PDT+, r=mcgreer, sr=nelsonb)
Attachments
(4 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details |
Changes needed to build NSS for Mac OS when targeting Carbon APIs.
Reporter | ||
Comment 1•24 years ago
|
||
Comment 2•24 years ago
|
||
nelsonb,
Could you review this patch?
We need this in the NSS 3.2 branch so that PSM builds on Mac OS X
Comment 3•24 years ago
|
||
I previously reviewed another patch quite similar to this one,
and sent out numerous review comments to someone. sdagley,
did I send them to you?
Comment 4•24 years ago
|
||
Javi, Steve,
Is this the same patch I reviewed for you back in February?
I'll email you another copy of my review comments from back then.
Reporter | ||
Comment 5•24 years ago
|
||
Nelson, yes, it's the same patch as originally submitted in February. I recall
some comments directed at beard regarding entropy but no detailed review of the
patch.
Comment 6•24 years ago
|
||
Right. My two comments were:
a) the patch ifdef's out lots of code whose purpose was to provide
badly-needed entropy to the PRNG. If you're going to remove sources
of entropy, you really should provide new sources of (at least)
equivalent amounts of entropy. Lack of adequate entropy is a good
way to fool yourself into thinking you've got security when you don't.
I cannot advise on where to find entropy on a MAC running OSX.
b) (minor) please use platform independent data types rather than
platform-dependent types where they're equivalent, e.g. PRUint32
rather than Uint32.
Comment 7•24 years ago
|
||
would it be possible to use unix_rand.c for Carbon builds? Or do we not have
access to the OS X unix level calls in Carbon builds?
Reporter | ||
Comment 8•24 years ago
|
||
The CFM based builds don't have access to the unix APIs (it is possible but it'd
be more work than just coming up with an acceptable version of a Carbon compliant
mac_rand.c)
Comment 9•24 years ago
|
||
Adding ddrinan to cc-list.
Nelson is going to write and publish a document that details the requirements
that we think must be satisifed by an entropy implementation for MAC OS X (or
any other platform). We should then solicit help for Apple and/or MAC OS X
experts in mozilla to assist us in implementing a mac_rand.c that satisfies
Nelsons requirements.
Comment 10•24 years ago
|
||
Any ideas when this might see some development work? Bug 73390 (Fizzilla can't
connect via https) is a big gap in functionality.
Comment 11•24 years ago
|
||
We have asked the community of Carbon developers to contribute the code
needed to add the entropy that is lost by the patch shown above.
So far, we have not received any such contribution. When we do receive
one, then ...
Whiteboard: Waiting for contribution from Carbon developers
Comment 12•24 years ago
|
||
If NSPR will be a Mach-O binary (bug 71718), could/should NSS be built based on
UNIX system calls as well?
Comment 13•24 years ago
|
||
Should this be Platform: Mac OS X, whiteboard: OSX+ ?
Comment 14•24 years ago
|
||
Our situation with OSX is similar to our situation with OS/2.
We depend on others to provide the platform-dependent parts.
AFAIK, there are no OSX/Carbon experts on the PSM and NSS teams.
We also don't have a system and related build software with which
we can try things out. That's why we're looking for that contribution.
There may be a number of simple solutions to this problem of which
we're simply unaware.
Comment 15•24 years ago
|
||
just spoke with nelson. since iplanet doesn't have the resources, i'll pull this
back into CPD.
changing product so i can set the milestone appropriate to rtm
Updated•24 years ago
|
Status: NEW → ASSIGNED
Comment 16•24 years ago
|
||
--> beard, now that i have a beta stopper on my list.
Assignee: pinkerton → beard
Status: ASSIGNED → NEW
Comment 18•24 years ago
|
||
BTW, Ian McGreer has developed some techniques for measuring the amount of
entropy actually being fed into the PRNG. I suggest that these techniques
be used to measure the amount being collected on the MAC, and on OSX with
the previously proposed patch, and then with any other proposed patches.
The solution for this problem should not result in less entopy being fed
into the PRNG than is now fed for MACs. I suggest people try Ian's
techniques before proposing new patches.
Comment 19•23 years ago
|
||
*** Bug 84680 has been marked as a duplicate of this bug. ***
Comment 20•23 years ago
|
||
Is there any update on this bug?
Assignee | ||
Comment 21•23 years ago
|
||
I'm working on it, looking for additional sources of entryopy.
Assignee | ||
Comment 22•23 years ago
|
||
I have an updated patch to mac_rand.c, using Microseconds instead of
TickCount() to get a faster running clock. With this patch, I'm getting the
following entropy measurements when running Mozilla 10 times w/o
browsing:
0.00 < entropy < 0.10 ] : 19788
0.10 < entropy < 0.20 ] : 0
0.20 < entropy < 0.30 ] : 2409
0.30 < entropy < 0.40 ] : 3874
0.40 < entropy < 0.50 ] : 267
0.50 < entropy < 0.60 ] : 0
0.60 < entropy < 0.70 ] : 5497
0.70 < entropy < 0.80 ] : 6227
0.80 < entropy < 0.90 ] : 1803
0.90 < entropy < 1.00 ] : 0
1.00 < entropy < 1.10 ] : 4775
Ian McGreer tells me that this is sufficiently random for our purposes.
Assignee | ||
Comment 23•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 24•23 years ago
|
||
Ian and Nelson, can I get r/sr from you and then would you check this
patch in?
Keywords: review
Assignee | ||
Updated•23 years ago
|
Whiteboard: OSX++ → OSX++, PDT+
Comment 25•23 years ago
|
||
I don't really know Mac API's, but for the RNG part r=mcgreer.
Note that the entropy statistics are a bit inflated in this case. I looked at
the actual data collected, and much of it is repeated within the sample, but
different across samples. this is the case where the analyzer is limited. I
believe that there is as much entropy being fed in now as there ever was with
the Mac, so we should go ahead with the patch, but I need to work on a better
estimate of what the entropy really is.
Comment 26•23 years ago
|
||
I agree with Ian that the new numbers are inflated. IMO, we shouldn't
rely so heavily on a single source, especially when it is monotonic.
What are the numbers you get without the microsecond counter?
If they're good enough, then I'm not worried about the possible inflation.
Updated•23 years ago
|
Whiteboard: OSX++, PDT+ → OSX++, PDT+ patch available
Comment 27•23 years ago
|
||
Nelson, can you give this an sr= then?
Whiteboard: OSX++, PDT+ patch available → OSX++, PDT+, r=mcgreer, sr=nelsonb??
Comment 28•23 years ago
|
||
Good:
The change from TickCount() to Microseconds() is an improvement.
Bad:
For TARGET_CARBON build, the patch simply comments out the code
that uses the user name, current directory, heap, and event queue
to feed data into RNG_RandomUpdate(). Only the code that uses
scrap size and scrap count has been provided with an altenate
implementation using Carbon API.
Comment 29•23 years ago
|
||
Wan-Teh,
Is this good enough for a beta shipment? For Mac OS X the release of NS6.1 will
be a beta, with final to come in the fall. So if this partial fix gets us close
enough for now, it will buy time for the full fix by fall.
Comment 30•23 years ago
|
||
Clayton, it's possible that this patch is good enough for a beta, but I'm afraid
that this distinction may be lost on people outside our team. Is there perhaps a
little more time Patrick could devote to make sure we don't get into trouble?
Assignee | ||
Comment 31•23 years ago
|
||
The entryopy measurements should be enough to decide this. I'm not qualified to
choose other sources of randomness, short of trying other things and measuring
the resulting entropy using Ian McGreer's tools. The guidelines he gave me said
that roughly 500 bits of randomness are needed to properly seed the RNG when the
browser starts up. The data I collected apparently generate a fair bit more than
this.
A comment on the code that is not running when TARGET_CARBON is 1. The PPCToolbox
API GetDefaultUser() returns the same value every time it is run.
LMGetCurDirStore() returns the last directory that was accessed in the standard
file dialog, which typically won't change from run to run of an application.
LMGetTheZone() is a pointer to the base address where an application loads. This
won't be terribly random when an application is run multiple times before the
user reboots. The data values returned from GetMainDevice() are effectively
constant on a given machine configuration, they only change when the user changes
the device resolution.
Lastly, the OS event queue is no longer exposed. The interesting thing, is that
it is probably almost always empty while the user is waiting for the application
to launch. Only if the user has typed ahead, or has clicked the mouse a number of
times will the OS event queue contain anything.
Comment 32•23 years ago
|
||
There are two types of entropy we are looking at. One is pure entropy (things
that will change from run to run), the other is machine dependent entropy
(things that may not change very often from run to run, but are different from
machine to machine). We need to get as much of the first as possible, but we
also can use as much of the second as we can get our hands on. For example, on
UNIX we mix in /etc/passwd. It doesn't change much from run to run, but is
likely to be different on different machines.
bob
Comment 33•23 years ago
|
||
wtc, so? we've demonstrated there is more than enough entropy with those
removed. why do we have to replace them?
Comment 34•23 years ago
|
||
The key is that the bits must be truely random. There are many measures
of randomness.
One is the average value of the bit. Obviously a bit that is usually 1
and rarely 0, or or usually 0 and rarely 1 offers little entropy. The
test that has been previously given here measures the probability that
the bit is equally likely to be zero or 1. That's a first approximation
of entropy. But it's only an approximation. An input stream that is
0x5a5a in all odd numbered runs and 0xa5a5 in all even numbered runs
will appear to this first test to be pure entropy, but there is very
little entopy in it.
Another measure is the predictability of the bit given that all past bits
input are known. From a security perspective, a predictable bit stream
offers no entropy, even if its bits are 1 and 0 in equal numbers.
When Ian wrote that much of the data collected is repeated within the
sample, that seriously lessens the unpredictability of the stream.
That concerns me.
Above, I asked for a test result that should show how much of the
newly measured "entropy" comes (or does not come) from the microsecond
function. I'd still like to see that result.
Assignee | ||
Comment 35•23 years ago
|
||
I will publish those results as soon as possible.
Comment 36•23 years ago
|
||
*** Bug 86915 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 37•23 years ago
|
||
Using TickCount():
0.00 < entropy < 0.10 ] : 25099
0.10 < entropy < 0.20 ] : 0
0.20 < entropy < 0.30 ] : 20147
0.30 < entropy < 0.40 ] : 36415
0.40 < entropy < 0.50 ] : 18129
0.50 < entropy < 0.60 ] : 0
0.60 < entropy < 0.70 ] : 65144
0.70 < entropy < 0.80 ] : 31027
0.80 < entropy < 0.90 ] : 17237
0.90 < entropy < 1.00 ] : 0
1.00 < entropy < 1.10 ] : 20530
Using Microseconds():
0.00 < entropy < 0.10 ] : 19788
0.10 < entropy < 0.20 ] : 0
0.20 < entropy < 0.30 ] : 2409
0.30 < entropy < 0.40 ] : 3874
0.40 < entropy < 0.50 ] : 267
0.50 < entropy < 0.60 ] : 0
0.60 < entropy < 0.70 ] : 5497
0.70 < entropy < 0.80 ] : 6227
0.80 < entropy < 0.90 ] : 1803
0.90 < entropy < 1.00 ] : 0
1.00 < entropy < 1.10 ] : 4775
So, TickCount() is better by a large factor. So, the first patch should be checked in rather than the second patch. Nelson, with these results, please sr.
.
Comment 38•23 years ago
|
||
Patrick-
Did you collect the data from TickCount() and Microseconds() as close to
simultaneously as possible? If not, the other entropy collection functions
could have greatly affected the data. For example, if your clipboard was empty
before running the build using Microseconds(), that would cause the large
difference in the results.
I ask because the difference is an order of magnitude, though the contribution
from either TickCount() or Microseconds() is at most a few bytes. So that alone
cannot be the difference.
After reading the documentation at www.apple.com, I strongly believe that
Microseconds() is the call we should be making. The resolution of TickCount()
is only 1/60 of a second, while Microseconds() is about 20ms. Using
Microseconds() would be a significant improvement.
Thus, the second patch is the one we should consider. I'm still willing to give
an r= to that patch.
Comment 39•23 years ago
|
||
Look at the huge differences in sample counts.
This is not comparing apples to apples.
Why does the example with tick count numbers have an order of
magnitude more samples?
Assignee | ||
Comment 40•23 years ago
|
||
OK, it looks like I'll need to build it both ways, and clone the builds so I can
run them in similar initial conditions.
Assignee | ||
Comment 41•23 years ago
|
||
OK, these were done back to back, same clipboard, hopefully nearly the same
screen. These measurements concur with your opinion, that Microseconds() is
better than TickCount(). Here're the results:
Using Microseconds:
0.00 < entropy < 0.10 ] : 20689
0.10 < entropy < 0.20 ] : 0
0.20 < entropy < 0.30 ] : 5949
0.30 < entropy < 0.40 ] : 24490
0.40 < entropy < 0.50 ] : 31
0.50 < entropy < 0.60 ] : 0
0.60 < entropy < 0.70 ] : 21879
0.70 < entropy < 0.80 ] : 10334
0.80 < entropy < 0.90 ] : 304
0.90 < entropy < 1.00 ] : 0
1.00 < entropy < 1.10 ] : 2596
Using TickCount:
0.00 < entropy < 0.10 ] : 20449
0.10 < entropy < 0.20 ] : 0
0.20 < entropy < 0.30 ] : 10711
0.30 < entropy < 0.40 ] : 4105
0.40 < entropy < 0.50 ] : 302
0.50 < entropy < 0.60 ] : 0
0.60 < entropy < 0.70 ] : 7688
0.70 < entropy < 0.80 ] : 9935
0.80 < entropy < 0.90 ] : 5816
0.90 < entropy < 1.00 ] : 0
1.00 < entropy < 1.10 ] : 9954
Microseconds is clearly the winner then.
Assignee | ||
Comment 42•23 years ago
|
||
I emailed the raw data for these measurements to Ian and Nelson for them to
confirm my conclusions.
Comment 43•23 years ago
|
||
The results shown above do not seem to support the conclusion that
microseconds is "clearly the winner".
I tossed those numbers into a spreadsheet and got these results:
sample bits entropy bits percentage (entropy/sample)
----------- ------------ ----------
TickCount 86272 31749.6 36.8
Microseconds 68960 29668.6 43.0
The change from TickCount to MicroSeconds does not account for the
difference in raw bits of input, does it?
I conclude that there is some other factor, perhaps an uncontrolled
variable, that is causing the amount of input to the collector to
vary widely. I believe the "noise" represented by that factor drowns
out the difference between tickCount and Microseconds.
However, that uncontrolled variable which affects the amount of data
collected, may itself be a large source of entropy. The question in
my mind is whether another program running concurrently would see
essentially the same results (same number of samples). If the cause
of this variance of amount collected is not observable or reproducible by
another conncurrent process, then I'd say this entropy is sufficient, and
the choice of TickCount or Microseconds is not significant (or better yet,
do both).
Comment 44•23 years ago
|
||
All,
we need to make a decision on this. If you don't object, let's pick the
microsecond patch. The rationale is that it's not clearly worse (is it?), the
www.apple.doc has some information which tips the balance ever so slightly.
Let's check this in for RTM, and we can open a new bugs if we conclusively finds
a flaw in the solution.
Assignee | ||
Comment 45•23 years ago
|
||
Thanks to nelson for doing more than a cursory eyeball of the numbers (which is
all I had time for). Yes, let's get this in for RTM, which also means let's
nominate this for nsbranch (a manager from your group must add the nsbranch
keyword) so it can be checked into the MOZILLA_0_9_2_BRANCH.
The appearance of various windows on the screen during my data collection could
account for these changes. Since we're reading screen memory, as the pixels
scroll in various windows this is probably affecting the data itself.
Comment 46•23 years ago
|
||
Isolating only the data supplied from the TickCount/Microseconds call:
Using TickCount():
0.00 < entropy < 0.10 ] : 16
0.10 < entropy < 0.20 ] : 0
0.20 < entropy < 0.30 ] : 1
0.30 < entropy < 0.40 ] : 0
0.40 < entropy < 0.50 ] : 2
0.50 < entropy < 0.60 ] : 0
0.60 < entropy < 0.70 ] : 3
0.70 < entropy < 0.80 ] : 1
0.80 < entropy < 0.90 ] : 3
0.90 < entropy < 1.00 ] : 0
1.00 < entropy < 1.10 ] : 6
32 total bits (TickCount() returns a uint32)
So, (0.0 * 16) + (.2 * 1) + (.4 * 2) + (.6 * 3) + (.7 * 1) + (.8 * 3) + (1.0 *
6) = 11.9
Using Microseconds():
0.00 < entropy < 0.10 ] : 34
0.10 < entropy < 0.20 ] : 0
0.20 < entropy < 0.30 ] : 0
0.30 < entropy < 0.40 ] : 0
0.40 < entropy < 0.50 ] : 0
0.50 < entropy < 0.60 ] : 0
0.60 < entropy < 0.70 ] : 9
0.70 < entropy < 0.80 ] : 6
0.80 < entropy < 0.90 ] : 6
0.90 < entropy < 1.00 ] : 0
1.00 < entropy < 1.10 ] : 9
64 total bits (Microseconds() returns a struct of two 32-bit integers)
(0.0 * 34) + (.6 * 9) + (.7 * 6) + (.8 * 6) + (1.0 * 9) = 23.4
It appears Microseconds() is better.
Comment 47•23 years ago
|
||
Finally, a more conservative estimate of how much entropy is actually being
supplied. What I noticed in looking at the data is that the repeated calls to
retrieve the clipboard vary from run to run, but not within each run. That is,
things like "FF" are repeated over and over in one sample, then in the next it
will be "00" repeated. That throws off my analyzer. So I decided to remove
*all* of that data, even though it may make some contribution. With it removed,
and using the Microseconds() data, I got:
0.00 < entropy < 0.10 ] : 51322
0.10 < entropy < 0.20 ] : 0
0.20 < entropy < 0.30 ] : 343
0.30 < entropy < 0.40 ] : 171
0.40 < entropy < 0.50 ] : 146
0.50 < entropy < 0.60 ] : 0
0.60 < entropy < 0.70 ] : 413
0.70 < entropy < 0.80 ] : 329
0.80 < entropy < 0.90 ] : 302
0.90 < entropy < 1.00 ] : 0
1.00 < entropy < 1.10 ] : 542
It is clear that much of the entropy measured here comes from collecting screen
pixels, which seems like a good thing to do to me. There are 4096 bytes from
that call, which is much greater than anything else in the sample (after the
clipboard stuff was removed).
Comment 48•23 years ago
|
||
Several comments:
First, thank you Ian for doing more in-depth investigation of the results.
Second, 11.9 and 23.4 are merely proverbial drips in the bucket. So,
the important decision here is NOT tickcount vs microseconds.
Third, If another process can collect the very same data that's being used
for input (or a large portion of the very same data), then the value of
that data as being unpredictable and unrepeatable is essentially zero for
security purposes. I'm guessing that the screen buffer is readable by all
processes, yes?
What numbers do you get if you remove the screen data as well?
Comment 49•23 years ago
|
||
Here is the Microseconds data with both clipboard and screen capture removed.
0.00 < entropy < 0.10 ] : 20366
0.10 < entropy < 0.20 ] : 0
0.20 < entropy < 0.30 ] : 19
0.30 < entropy < 0.40 ] : 34
0.40 < entropy < 0.50 ] : 31
0.50 < entropy < 0.60 ] : 0
0.60 < entropy < 0.70 ] : 71
0.70 < entropy < 0.80 ] : 81
0.80 < entropy < 0.90 ] : 47
0.90 < entropy < 1.00 ] : 0
1.00 < entropy < 1.10 ] : 151
Assignee | ||
Comment 50•23 years ago
|
||
Assignee | ||
Comment 51•23 years ago
|
||
Comment 52•23 years ago
|
||
The numbers in the table just above add up to 314.3 bits.
That number is more than 160, which is the absolute bare minimum.
And I believe it's sufficiently conservative to be believed.
So, I approve this entropy change based on these numbers.
Comment 53•23 years ago
|
||
sr=nelsonb for the patch
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=39418
After obtaining the relevant drivers and PDT approvals,
please check it into the appropriate branch(es).
Whiteboard: OSX++, PDT+, r=mcgreer, sr=nelsonb?? → OSX++, PDT+, r=mcgreer, sr=nelsonb
Comment 54•23 years ago
|
||
fizilla only, looks good. a= asa@mozilla.org for checkin to 0.9.2.
(on behalf of drivers)
Comment 55•23 years ago
|
||
Checked in on NSS tip, NSS_3_2_BRANCH, and MOZILLA_0_9_2_BRANCH, and updated
NSS_CLIENT_TAG to point to the new revision.
Comment 56•23 years ago
|
||
meant to mark this fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 57•23 years ago
|
||
Checked in on NSS_3_3_BRANCH.
Comment 58•23 years ago
|
||
NSS is part of Security: Crypto IIRC. Changing module to reflect...
Component: Security: General → Security: Crypto
Comment 59•23 years ago
|
||
sonmi - can you pls verify.
Would I be able to verify this from any UI or going to any particular https:
page in the OS X build I have?
Reporter | ||
Comment 60•23 years ago
|
||
Lisa, if you can load <https://www.verisign.com/> and you get the lock icon on
the bottom right of the window you've got a working NSS
Comment 61•23 years ago
|
||
Wonderful! I've verified this on:
OS X: 2001-07-17 branch build.
Status: RESOLVED → VERIFIED
Comment 62•23 years ago
|
||
Mass changing Security:Crypto to PSM
Component: Security: Crypto → Client Library
Product: Browser → PSM
Target Milestone: mozilla0.9.2 → ---
Version: other → 2.1
Comment 63•23 years ago
|
||
Mass changing Security:Crypto to PSM
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•