Hi Mark, Thanks for the comments, most I have accepted, see inline.
Mark (the other one) > ikeadm.c > -------- > mcp-0 line 1799,1800 Why not factor out all the duplicate > calls to dump_key()? I can't see a reason why I shouldn't do this, need to talk this over with the rest of the team. > > ipsecalgs.c > ----------- > mcp-1 line 65 Enumerated values start at 0, so max_param > will have a value of 3. The array will be > one short. No, this is OK, max_param is never used, only to determine the array size. > > mcp-2 line 119 Use strncmp() - especially if flag_str is > user supplied. In this case strcmp() is safe becaue both strings are NULL terminated, the first comes from an automatic array, the second from getopt() which NULL terminates its strings. > > mcp-3 line 135 Shouldn't it be "CCM,GCM"? I think "[-F COMBINED,COUNTER,CCM|GCM ]" is correct, its GCM or CCM, while you could specify both, its invalid! > > ipseckey.c > ---------- > mcp-4 line 457 Looks like one tab too many. > Opps! > mcp-5 line 635 "is insecure": shouldn't this be > "may be insecure" since the user/admin > might actually get the nonce right? > "may be" sounds better, so I changed it. The problem is not the admin getting it wrong, its that if these static values are configured into /etc/inet/ipseckeys and the system is rebooted, the same nonce/spi values would be reused, this is a potential problem! > mcp-6 line 2280 The reserved_bits argument is an int so it > should be 0 rather than B_FALSE. > Yes, changed. > ipsec_util.c > ------------ > mcp-7 line 225 s/bits, if/bits. If/ OK > > mcp-8 lines 242-249 You could use a macro like this > one in /usr/include/sys/crypto/common.h: > > #define CRYPTO_BITS2BYTES(n) (((n) + 7) >> 3) > Same thing. > algs.c > ------ > mcp-9 line 424 You really want to check mech_params. I do! > > ipsecesp.c > ---------- > mcp-10 line 1806 s/authentication/Authentication/ > > mcp-11 line 2169,2468 s/setip/setup/ > > mcp-12 line 2842 s/recieving/receiving/ Changed. > > sadb.c > ------ > mcp-13 line 3533 s/know/known/ > Changed. > mcp-14 lines 3543-3551 see mcp-8 > These are used extensively in IPsec, I don't want to change these. > spd.c > ----- > mcp-15 lines 5220,5221 This could be more compact if written: > > if ((alg->alg_flags & (ALG_FLAG_CCM|ALG_FLAG_GCM)) == > ALG_FLAG_CCM|ALG_FLAG_GCM) OK. > > mcp-16 lines 5220,5221 This could be more compact if written: > > if (alg->alg_flags & (ALG_FLAG_CCM|ALG_FLAG_GCM)) OK. > > mcp-17 line 5237,5238,5251 Wow! I don't think I've ever seen that > construction before. > :-) > > Mark Fenwick wrote: > > [ BCC'd security-discuss at opensolaris.org ] > > > > > > > > This is a request for code review for modifications to Solaris IPsec to > > support AES CCM and GCM Modes. The webrev for the usr/src portions are here: > > > > http://cr.opensolaris.org/~markfen/ccm_gcm/ > > > > Please provide any feedback before the end of Friday 9th October (sorry its > > such a short period). > > > > The following changes add support for AES_CCM and AES_GCM modes to Solaris > > IPsec. The following description is taken from PSARC/2009/513 which also > > describes the interface changes. > > > > Introduction: > > ============= > > > > IPsec ESP provides network packet encryption and authentication using > > encryption and authentication algorithms described by ipsecalgs(1m) and > > configured using the ipsecconf(1m) tokens: > > > > encr_algs > > ancr_auth_algs > > > > Each operation, that is encryption and authentication, is performed by a > > separate cryptographic mechanism in the encryption framework. > > > > The introduction of AES combined mode ciphers into the encryption framework > > began with AES CCM Mode (PSARC/2007/266), then AES GCM Mode. These combined > > mode > > ciphers provide a mechanism to encrypt and authenticate in a single > > operation. > > There are potential performance improvements when using combined mode > > ciphers. > > > > There are currently two combined mode AES cipher modes supported by the > > encryption framework: > > > > AES CCM - (Counter with CBC-MAC) > > AES GCM - (Galois/Counter Mode) > > > > AES GCM Mode is a requirement for NSA Suite B Cryptography, a US government > > standard for cryptographic algorithms. The support of AES GCM for IPsec ESP > > is > > a significant step towards Suite B compliance and puts Solaris on par with > > Linux and Microsoft Windows server 2008 R2 in this area. > > > > Implementation guidelines are provided by the following RFCs: > > > > 4309 - Using Advanced Encryption Standard (AES) CCM Mode > > with IPsec Encapsulating Security Payload (ESP) > > > > 4106 - The Use of Galois/Counter Mode (GCM) > > in IPsec Encapsulating Security Payload (ESP) > > > > Overview of changes: > > ==================== > > > > usr/closed > > > > There are some minor changes to the usr/closed gate which unfortunately I > > can't post here, but I can describe: > > > > The CCM/GCM Modes need a salt value, this will form part of the nonce used > > by the crypto mechanism. The salt is a secret know to both peers, its > > derived from > > the same PRF that derived the encryption keys. When in.iked registers with > > the kernel, the kernel sends a message which tells in.iked how long the > > keys are (in bits) and the length of the salt. You can see this in the > > usr/src webrev, The key length forms part of the IKE Phase 2 transform, but > > the salt is not. The changes in usr/closed are to add the salt value into > > the length used by the PRF to get the key. A single byte stream (the length > > of key+salt) is sent back to the kernel as the encryption key, the kernel > > then takes the last "saltbits" of this key to use in the nonce. > > > > usr/src > > > > Summary of changes: > > > > The ipsecalgs(1m) command has been modified to allow it do tell the kernel > > some of the extra parameters required for combined mode ciphers like CCM > > and GCM. Specifically: > > > > Salt length > > IV Length > > ICV Length > > Flags > > > > There are also changes to the /etc/inet/ipsecalgs file used at install and > > the class action script used by upgrade (SVR4 only). > > > > The coresponding kernel changes are in spdsock.c > > > > The ipsecconf(1m) command uses the flags defined by ipsecalgs(1m) to > > determine if a particular algorithm provides encryption and authentication > > in same operation, if it does then it allows encr_algs WITHOUT > > encr_auth_algs. > > > > Some minor changes to getipsecalgbyname(3NSL) - adding parameters and flags. > > > > Some minor libipsecutil changes to dump salt values when using 'ipseckey > > dump' > > > > Changes to sadb.c - Changes to sadb_common_add() deal with adding anew SA. > > The changes deal with adding in a nonce update function to the SA, this is > > used to configure the nonce (PARAMS) before passing the IP packet to the > > Crypto Framework. There are comments in the code to describe the function > > operation. > > > > The changes in ipsecesp.c relate to the way IPsec ESP packets are > > processed. Before these changes were made, ESP packets could be encrypted > > and/or authenticated, these operations were performed by different code > > paths and different encryption framework function calls. Combined mode (CCM > > or GCM) is now treated as a special case of encryption only, although it in > > fact does both. > > > > > > > > ---------------------------------------------------------------------------- > > Mark Fenwick, Solaris Security Technologies. > > __o > > Sun Microsystems Inc, Menlo Park, California. `\<,_ > > (*)/ (*) > > ---------------------------------------------------------------------------- > > > > > > > > > > _______________________________________________ > > security-discuss mailing list > > security-discuss at opensolaris.org > > >
