Golang crypto library: security

I've recently put together a library for working with Crypto in Go.

I did not roll my own, I'm relying on stdlib for the core parts of this, but I would still like to get this reviewed by some experts in case I've done something horrific.

If you're interested and want to view this in a more palatable format, you can find it here: https://github.com/alistanis/goenc

All of my tests currently pass and everything is working as expected.

I really appreciate the time that anyone is willing to spend on this, so thanks in advance.

The Code

enc.go

```
// Package goenc contains functions for working with encryption
package goenc

// work is derived from many sources:
//
// http://stackoverflow.com/questions/21151714/go-generate-an-ssh-public-key
// https://golang.org/pkg/crypto/cipher/
// https://leanpub.com/gocrypto/read#leanpub-auto-aes-cbc
// https://github.com/hashicorp/memberlist/blob/master/security.go

import (
"crypto/rand"
"encoding/binary"
"io"

"io/ioutil"

"os"

"github.com/alistanis/goenc/aes/cbc"
"github.com/alistanis/goenc/aes/cfb"
"github.com/alistanis/goenc/aes/ctr"
"github.com/alistanis/goenc/aes/gcm"
"github.com/alistanis/goenc/encerrors"
"github.com/alistanis/goenc/generate"
"github.com/alistanis/goenc/nacl"
"golang.org/x/crypto/nacl/box"
"golang.org/x/crypto/nacl/secretbox"
"golang.org/x/crypto/scrypt"
)

/*
TODO(cmc): Verify this isn't horrifically insecure and have this reviewed by a(n) expert(s) before publishing
*/

// BlockCipher represents a cipher that encodes and decodes chunks of data at a time
type BlockCipher interface {
Encrypt(key, plaintext []byte) ([]byte, error)
Decrypt(key, ciphertext []byte) ([]byte, error)
KeySize() int
}

//---------------------------------------------------------------------------
// BlockCipherInterface Functions - these should not be used with large files
//------------------------------------------------------------------------------

The code in the question shows a CBC wrapper class. However, that wrapper class uses HMAC for authentication, without this being apparent in the name of the class. Having authenticated ciphertext is great, but if it is present then that should be made clear. Another design mistake is making the mode of operation part of the BlockCipher interface. That is the wrong way around: a mode of operation uses a block cipher as configuration parameter. A block cipher does not include a HMAC.

HMAC authentication is also implemented for CTR mode. CTR mode has many uses for specialized encryption constructs. For instance, CTR mode is used in GCM, CCM and EAX modes of operation. However, those do of course not use HMAC. Inexplicably, the HMAC authentication is not implemented for the old CFB mode that, which, although secure, is generally not used in new protocols. It also means that the API is not symmetric; CBC and CTR do have HMAC authentication, why is CFB left behind?

Fortunately the code uses encrypt-then-MAC, which is the correct order.

The code contains functionality perform transport security and - what I presume is - in place encryption using passwords. That means it tries t do multiple things at the same time. Cryptography related code should be written with a specific use case in mind. Generic crypto code should be a very well written API so it can be used for those kind of use cases, and this is not it.

The transport mode security seems to contain key agreement but not authentication. There doesn't seem to be any warning about this in the comments.

The idea keys supplied by the user needs padding is very dangerous. It lets users believe that their key is secure, while it should be either 16, 24 or 32 fully random bytes for AES. Generally passwords should not be treated as keys. It is possible to turn passwords into keys using a PBKDF2, although for encryption password should be avoided where possible.

In general I'd warn against using "wrapper" classes, that are created to make AES "easy". It takes a very significant amount of time to remove a wrapper class from a code base. I've spend many hours trying to remove a self-spun wrapper class, mimicking a C++ wrapper class in Java.

You have been warned; don't make the same mistake that I did.

StackExchange Code Review Q#156458, answer score: 6