132438 - SSL_GetCipherSuiteInfo should report more characteristics

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

SSL_GetCipherSuiteInfo should report these additional characteristics:

1. isExportable:  This ciphersuite was one that was allowed to be exported
prior to Jan 10, 2000.  

2. nonStandard:  This ciphersuite is proprietary or otherwise "special".

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Will attach patch shortly.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: proposed patch implements two new bits.

Kai, please test this patch, if you will.  Thanks.

Comment 3

[Wan-Teh Chang]

Comment on attachment 75291 [details] [diff] [review]
proposed patch implements two new bits.  

The SSLCipherSuiteInfo structure makes the non-portable
assumption that PRUintn is 32-bit.  My reading of the
C standard is that bitfields can only have the type int,
signed int, or unsigned int.  So we probably can't declare
the bitfield as PRUint32, which on some platforms is
unsigned long.

This patch reminds me of my objection to using a bitfield.
At least two sources (one of them is Kernighan and Pike,
The Practice of Programming, p. 195) say that bitfields
are non-portable and recommend against using bitfields.
Unfortunately, I don't know what portability issues
bitfields have, so I don't have anything to support my
objection other than "I heard bitfields are evil".

I am most worried about binary compatibility issues if
we are to add a new bit in a future NSS release.  I did
a simple experiment with gcc and Sun Forte 6 compilers
and there is no binary compatibility problem when I add
a new bit.  So maybe it's fine.

To avoid using the bitfield, these bits
     PRUintn		  isFIPS       : 1;
     PRUintn		  isExportable : 1;
     PRUintn		  nonStandard  : 1;
     PRUintn		  reservedBits :29;
would be replaced by
     PRUintn		  flags;
and these macros
     #define SSL_IS_FIPS	  0x1
     #define SSL_IS_EXPORTABLE	  0x2
     #define SSL_IS_NONSTANDARD   0x4
and the static initializers would need to be changed
accordingly.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

I've been using bit fields for years without any problem.  The only 
"portability" problem with bit fields is that the order of packing of 
the bits into the underlying int is not specified by the language, 
and is generally an endian-dependent thing.  So, bit fields are not 
suitable for network protocols, but are OK for structures that are only
used within a machine.  

Some of the compilers we use are very pedantic, giving errors or warnings 
if any other integral type (including unsigned int) is used with bit fields.  

I used PRIntn knowing that PRIntn is identical to int, by definition.  
I thought that if I used int, someone might assume that I had not thought
about other issues and had just used int out of force of habit, and might 
say that I should have used some other type, but if I used PRIntn, it 
would be obvious that I had not used int out of force of habit, but that
I meant precisely int.  

At this time, given that we no longer support Win16, 100% of the platforms
we support have 32-bit ints.  Finally, even if int becomes 64 bits on
some platform, this code will remain correct.

Comment 5

[Wan-Teh Chang]

Nelson,

Actually you are using PRUintn, not PRIntn.  The unsigned
type is better.  I found that if it is declared PRIntn,
under gcc, the bits will all become -1 if set.  See this
experiment on Red Hat Linux 7.2:

=-=-=-=-=-=-=-
cmslinux:/u/wtc/tmp 54% uname -a
Linux cmslinux 2.4.7-10 #1 Thu Sep 6 17:27:27 EDT 2001 i686 unknown
cmslinux:/u/wtc/tmp 55% gcc -v
Reading specs from /usr/lib/gcc-lib/i386-redhat-linux/2.96/specs
gcc version 2.96 20000731 (Red Hat Linux 7.2 2.96-103)
cmslinux:/u/wtc/tmp 56% cat baz.c
#include <stdio.h>

struct {
    int bit:1;
} s;

int main()
{
    s.bit = 1;
    printf("%d %u\n", (int)s.bit, (unsigned)s.bit);
    return 0;
}
cmslinux:/u/wtc/tmp 57% gcc -Wall baz.c
cmslinux:/u/wtc/tmp 58% ./a.out
-1 4294967295
=-=-=-=-=-=-=-

So using PRUintn is good.

I guess declaring the 'reserved' as "PRUintn reserved:13;" would
be portable, at the cost of being limited to 16 bits.  I don't
have a strong preference.  It's your call.

Thanks.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

I originally tried PRUintn and one of our platforms was very unhappy with
it.  I don't have time now to determine which one it was.

Comment 7

[Wan-Teh Chang]

Comment on attachment 75291 [details] [diff] [review]
proposed patch implements two new bits.  

>Index: sslt.h
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/ssl/sslt.h,v
>retrieving revision 1.2
>diff -u -r1.2 sslt.h
>--- sslt.h	2 Nov 2001 04:24:21 -0000	1.2
>+++ sslt.h	20 Mar 2002 23:50:51 -0000
>@@ -156,7 +156,9 @@
>     PRUint16             macBits;
> 
>     PRUintn              isFIPS       : 1;
>-    PRUintn              reservedBits :31;
>+    PRUintn              isExportable : 1;
>+    PRUintn              nonStandard  : 1;
>+    PRUintn              reservedBits :29;
> 
> } SSLCipherSuiteInfo;
> 

It's PRUintn.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Checking in sslinfo.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslinfo.c,v  <--  sslinfo.c
new revision: 1.4; previous revision: 1.3
done
Checking in sslt.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslt.h,v  <--  sslt.h
new revision: 1.3; previous revision: 1.2
done

Comment 9

[Kai Engert [:KaiE:]]

Can we check this in to NSS_CLIENT_TAG ?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

a=dbaron for checkin to NSS_CLIENT_TAG (not that we'd be able to detect who
rolled the tag, or when...)

Comment 11

[Wan-Teh Chang]

I moved the NSS_CLIENT_TAG on sslt.h and sslinfo.c.
This fix is in the NSS_CLIENT_TAG now.