code review 217116: Implementation of the symmetric block cipher Blowfish.

src/pkg/crypto/blowfish/cipher.go

 // Copyright 2010 The Go Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-// This package implements Blowfish: a symmetric 64-bit block cipher designed

[rsc, 2010/02/25 09:01:02]

the package comment should say what users need to know:
the rest can go after the package statement.

// This package implements Bruce Schneier's Blowfish encryption algorithm.
package blowfish

// The code is a port of Bruce Schneier's C implementation.
// See http://www.schneier.com/blowfish.html.

[rsn, 2010/02/25 12:23:34]

Done.

[rsn, 2010/02/25 12:23:34]

Done.

-// by Bruce Schneier.
-// The code is a port of Bruce Schneier's C implementation. For up-to-date
-// information on the algorithm see: http://www.schneier.com/blowfish.html
+// This package implements Bruce Schneier's Blowfish encryption algorithm.
 package blowfish
+
+// The code is a port of Bruce Schneier's C implementation.
+// See http://www.schneier.com/blowfish.html.
 
 import (
         "os"
         "strconv"
 )
 
-// The Blowfish block size (64 bits) in bytes.

[rsc, 2010/02/25 09:01:02]

can drop (64 bits) which is redundant given the definition below

[rsn, 2010/02/25 12:23:34]

Done.

+// The Blowfish block size in bytes.
 const BlockSize = 8
 
-// Cipher is an instance of the internal Blowfish key constructed from user

[rsc, 2010/02/25 09:01:02]

please copy the comments from the AES implementation
(crypto/aes), both here and for all the functions below.
they are more concise.

[rsn, 2010/02/25 12:23:34]

Done.

-// supplied key material.
+// A Cipher is an instance of Blowfish encryption using a particular key.
 type Cipher struct {
-        p, s0, s1, s2, s3 []uint32

[rsc, 2010/02/25 09:01:02]

Since these are fixed size, the slice is unnecessary.
You can use a single array and do the allocation in
one chunk.

More importantly, having the fixed array will let the
compiler eliminate bounds checks for constant indexes
smaller than the array size:

type Cipher struct {
    p [18]uint32
    s0, s1, s2, s3 [256]uint32
}

[rsn, 2010/02/25 12:23:34]

Done.

+        p              [18]uint32
+        s0, s1, s2, s3 [256]uint32
 }
 
 type KeySizeError int
 
 func (k KeySizeError) String() string {
-        return "crypto/blowfish: Invalid key size " + strconv.Itoa(int(k))

[rsc, 2010/02/25 09:01:02]

lowercase letter after : please.  (invalid not Invalid).
(see crypto/aes)

[rsn, 2010/02/25 12:23:34]

Done.

+        return "crypto/blowfish: invalid key size " + strconv.Itoa(int(k))
 }
 
-// NewCipher creates and returns a new Blowfish Cipher.  The 'key' argument

[rsc, 2010/02/25 09:01:02]

you're in package blowfish already.
no need for quotes.  break line.
(see crypto/aes)

// NewCipher creates and returns a Cipher.
// The key argument should be the Blowfish key, 4 to 56 bytes.

[rsn, 2010/02/25 12:23:34]

Done.

-// MUST be in the range [32..448] bits in increments of 8 bits.
+// NewCipher creates and returns a Cipher.
+// The key argument should be the Blowfish key, 4 to 56 bytes.
 func NewCipher(key []byte) (*Cipher, os.Error) {
         k := len(key)
         if k < 4 || k > 56 {
                 return nil, KeySizeError(k)
         }
-        // allocate space for the cipher's context

[rsc, 2010/02/25 09:01:02]

with the changed Cipher above this shortens to

var c Cipher
expandKey(key, &c)
return &c, nil

[rsn, 2010/02/25 12:23:34]

Done.

-        var p = make([]uint32, 18)
-        var s0 = make([]uint32, 256)
-        var s1 = make([]uint32, 256)
-        var s2 = make([]uint32, 256)
-        var s3 = make([]uint32, 256)
-        result := &Cipher{p, s0, s1, s2, s3}
-        expandKey(key, result)
-        return result, nil
+        var result Cipher
+        expandKey(key, &result)
+        return &result, nil
 }
 
-// BlockSize returns the Blowfish block size in bytes.  It is needed to
-// satisfy the Key interface in the package "crypto/modes".

[rsc, 2010/02/25 09:01:02]

re-copy aes comment now that it says crypto/block.  ;-)

// BlockSize returns the Blowfish block size, 8 bytes.
// It is necessary to satisfy the Cipher interface in the
// package "crypto/block".

[rsn, 2010/02/25 12:23:34]

Done.

+// BlockSize returns the Blowfish block size, 8 bytes.
+// It is necessary to satisfy the Cipher interface in the
+// package "crypto/block".
 func (c *Cipher) BlockSize() int { return BlockSize }
 
-// Encrypt enciphers the first 8 bytes (64 bits) in 'src' using the Cipher's

[rsc, 2010/02/25 09:01:02]

copy aes comment

[rsn, 2010/02/25 12:23:34]

done but note that this and the next one (for the Decrypt) refer to k which does
not exist in the signature of the method.

-// internal key and stores the result in the first 8 bytes of 'dst'.
+// Encrypt encrypts the 8-byte buffer src using the key k
+// and stores the result in dst.
+// Note that for amounts of data larger than a block,
+// it is not safe to just call Encrypt on successive blocks;
+// instead, use an encryption mode like CBC (see crypto/block/cbc.go).
 func (c *Cipher) Encrypt(src, dst []byte) {
         l := uint32(src[0])<<24 | uint32(src[1])<<16 | uint32(src[2])<<8 | uint32(src[3])
         r := uint32(src[4])<<24 | uint32(src[5])<<16 | uint32(src[6])<<8 | uint32(src[7])
         l, r = encryptBlock(l, r, c)
         dst[0], dst[1], dst[2], dst[3] = byte(l>>24), byte(l>>16), byte(l>>8), byte(l)
         dst[4], dst[5], dst[6], dst[7] = byte(r>>24), byte(r>>16), byte(r>>8), byte(r)
-        l, r = 0, 0

[rsc, 2010/02/25 09:01:02]

delete
(compiler will just optimize away)

[rsn, 2010/02/25 12:23:34]

Done.

 }
 
-// Decrypt deciphers the first 8 bytes (64 bits) in 'src' using the Cipher's

[rsc, 2010/02/25 09:01:02]

copy aes comment

[rsn, 2010/02/25 12:23:34]

Done.

-// internal key and stores the result in the first 8 bytes of 'dst'.
+// Decrypt decrypts the 8-byte buffer src using the key k
+// and stores the result in dst.
 func (c *Cipher) Decrypt(src, dst []byte) {
         l := uint32(src[0])<<24 | uint32(src[1])<<16 | uint32(src[2])<<8 | uint32(src[3])
         r := uint32(src[4])<<24 | uint32(src[5])<<16 | uint32(src[6])<<8 | uint32(src[7])
         l, r = decryptBlock(l, r, c)
         dst[0], dst[1], dst[2], dst[3] = byte(l>>24), byte(l>>16), byte(l>>8), byte(l)
         dst[4], dst[5], dst[6], dst[7] = byte(r>>24), byte(r>>16), byte(r>>8), byte(r)
-        l, r = 0, 0

[rsc, 2010/02/25 09:01:02]

delete

[rsn, 2010/02/25 12:23:34]

Done.

 }
 
-// Reset zeroes the cipher's internal key.

[rsc, 2010/02/25 09:01:02]

copy aes comment

[rsn, 2010/02/25 12:23:34]

Done.

+// Reset zeros the key data, so that it will no longer
+// appear in the process's memory.
 func (c *Cipher) Reset() {
-        var i int = 0

[rsc, 2010/02/25 09:01:02]

i suggest writing a helper

func zero(x []uint32) {
    for i := range x {
        x[i] = 0
    }
}

and then call it here on each slice.
the compiler is much more likely to 
get the bounds checks eliminated
in a single loop, and it's clearer what's going on.
the loop constants here are magic otherwise.

when you get rid of the slices as mentioned above,
you'll have to say zero(&c.p) or zero(c.p[0:]).

[rsn, 2010/02/25 12:23:34]

Done.

-        for ; i < 18; i++ {
-                c.p[i], c.s0[i], c.s1[i], c.s2[i], c.s3[i] = 0, 0, 0, 0, 0
-        }
-        for ; i < 256; i++ {
-                c.s0[i], c.s1[i], c.s2[i], c.s3[i] = 0, 0, 0, 0
-        }
+        zero(&c.p)
+        zero(&c.s0)
+        zero(&c.s1)
+        zero(&c.s2)
+        zero(&c.s3)
 }