Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: return generic errors for aes256cbc encrypt and decrypt #509

Merged
merged 10 commits into from
Feb 2, 2020
Merged

chore: return generic errors for aes256cbc encrypt and decrypt #509

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7a6ddb3
feat: create generic errors for cipher encrypt and decrypt #498
developerfred Jan 5, 2020
20febd0
chore: return generic errors for aes256cbc encrypt and decrypt #499
developerfred Jan 5, 2020
15b0155
chore: return generic errors for aes256cbc encrypt and decrypt #499
developerfred Jan 5, 2020
9d49515
Merge branch 'master' into issue/499
developerfred Jan 5, 2020
aa8a3fd
Merge branch 'issue/499' of github.com:developerfred/mailchain into i…
developerfred Jan 5, 2020
f823427
Merge branch 'master' into issue/499
developerfred Jan 5, 2020
ff0c66d
chore: return generic errors #499
developerfred Jan 5, 2020
4a6dba4
Merge branch 'master' into issue/499
robdefeo Jan 6, 2020
0801ef9
Merge branch 'master' into issue/499
robdefeo Feb 2, 2020
56bfad0
missing errors import
robdefeo Feb 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 deletions crypto/cipher/aes256cbc/decrypter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"github.com/mailchain/mailchain/crypto"
mc "github.com/mailchain/mailchain/crypto/cipher"
"github.com/mailchain/mailchain/crypto/secp256k1"
"github.com/pkg/errors"
)

// NewDecrypter create a new decrypter attaching the private key to it
Expand All @@ -40,7 +39,7 @@ type Decrypter struct {
func (d Decrypter) Decrypt(data mc.EncryptedContent) (mc.PlainContent, error) {
encryptedData, err := bytesDecode(data)
if err != nil {
return nil, errors.WithMessage(err, "could not convert encryptedData")
return nil, mc.ErrDecrypt
}

return decryptEncryptedData(d.privateKey, encryptedData)
developerfred marked this conversation as resolved.
Show resolved Hide resolved
developerfred marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -49,42 +48,41 @@ func (d Decrypter) Decrypt(data mc.EncryptedContent) (mc.PlainContent, error) {
func decryptEncryptedData(privKey crypto.PrivateKey, data *encryptedData) ([]byte, error) {
tmpEphemeralPublicKey, err := secp256k1.PublicKeyFromBytes(data.EphemeralPublicKey)
if err != nil {
return nil, errors.WithMessage(err, "could not convert ephemeralPublicKey")
return nil, mc.ErrDecrypt
}

ephemeralPublicKey, err := tmpEphemeralPublicKey.(*secp256k1.PublicKey).ECIES()
if err != nil {
return nil, errors.WithMessage(err, "could not convert to ecies")
return nil, mc.ErrDecrypt
}

recipientPrivKey, err := asPrivateECIES(privKey)
if err != nil {
return nil, err
return nil, mc.ErrDecrypt
}

sharedSecret, err := deriveSharedSecret(ephemeralPublicKey, recipientPrivKey)
if err != nil {
return nil, errors.WithMessage(err, "could not derive shared secret")
return nil, mc.ErrDecrypt
}

macKey, encryptionKey := generateMacKeyAndEncryptionKey(sharedSecret)
mac, err := generateMac(macKey, data.InitializationVector, *ephemeralPublicKey, data.Ciphertext)

if err != nil {
return nil, errors.WithMessage(err, "generateMac failed")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good idea to mask all errors with the same error. It's okay to return the same error type, but it should not be the same error message for different types of errors, the actual error is getting masked here. It'll be harder to debug the reason just by seeing "error in decryption".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that in the future we will create a variable for each type of error, making the code more maintainable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hasitpbhatt thank you for taking the time to leave a review on this PR. Sorry for the delay getting back to you I was off for a while. I would love to hear about how you found out about Mailchain and if there is anything I can do to help you. Please feel free to comment here or join our slack channel

You raise an excellent point about the outcome from a developer perspective. The reason for masking the error is to reduce the information an attacker might get from an error. This does need more thought overall so I have opened an issues #532 to discuss this.

In the meantime @developerfred I will merge this change as it resolves the issue as originally defined. Once #532 is resolved we will come back to this

return nil, mc.ErrDecrypt
}

if subtle.ConstantTimeCompare(data.MessageAuthenticationCode, mac) != 1 {
return nil, errors.Errorf("invalid mac")
return nil, mc.ErrDecrypt
}

return decryptCBC(encryptionKey, data.InitializationVector, data.Ciphertext)
}

func decryptCBC(key, iv, ciphertext []byte) ([]byte, error) {
block, err := aes.NewCipher(key)
if err != nil {
return nil, err
return nil, mc.ErrDecrypt
}

plaintext := make([]byte, len(ciphertext))
Expand All @@ -93,7 +91,7 @@ func decryptCBC(key, iv, ciphertext []byte) ([]byte, error) {

plaintext, err = padding.NewPkcs7Padding(block.BlockSize()).Unpad(plaintext)
if err != nil {
return nil, errors.WithMessage(err, "could not pad")
return nil, mc.ErrDecrypt
}

ret := make([]byte, len(plaintext))
Expand Down
17 changes: 8 additions & 9 deletions crypto/cipher/aes256cbc/encrypter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
"github.com/ethereum/go-ethereum/crypto/ecies"
"github.com/mailchain/mailchain/crypto"
mc "github.com/mailchain/mailchain/crypto/cipher"
"github.com/pkg/errors"
)

// NewEncrypter create a new encrypter with crypto rand for reader
Expand All @@ -44,22 +43,22 @@ type Encrypter struct {
func (e Encrypter) Encrypt(message mc.PlainContent) (mc.EncryptedContent, error) {
developerfred marked this conversation as resolved.
Show resolved Hide resolved
epk, err := asPublicECIES(e.publicKey)
if err != nil {
return nil, errors.WithMessage(err, "could not convert")
return nil, mc.ErrEncrypt
}

ephemeral, err := ecies.GenerateKey(e.rand, ecies.DefaultCurve, nil)
if err != nil {
return nil, errors.WithMessage(err, "could not generate ephemeral key")
return nil, mc.ErrEncrypt
}

iv, err := e.generateIV()
if err != nil {
return nil, errors.WithMessage(err, "could not generate iv")
return nil, mc.ErrEncrypt
}

encryptedData, err := encrypt(ephemeral, epk, message, iv)
if err != nil {
return nil, errors.WithMessage(err, "could not encrypt data")
return nil, mc.ErrEncrypt
}

return bytesEncode(encryptedData)
Expand All @@ -74,12 +73,12 @@ func encrypt(ephemeralPrivateKey *ecies.PrivateKey, pub *ecies.PublicKey, input,
macKey, encryptionKey := generateMacKeyAndEncryptionKey(sharedSecret)
ciphertext, err := encryptCBC(input, iv, encryptionKey)
if err != nil {
return nil, errors.WithMessage(err, "encryptCBC failed")
return nil, mc.ErrEncrypt
}

mac, err := generateMac(macKey, iv, ephemeralPublicKey, ciphertext)
if err != nil {
return nil, errors.WithMessage(err, "generateMac failed")
return nil, mc.ErrEncrypt
}

return &encryptedData{
Expand All @@ -97,11 +96,11 @@ func encryptCBC(data, iv, key []byte) ([]byte, error) {
}
data, err = padding.NewPkcs7Padding(block.BlockSize()).Pad(data)
if err != nil {
return nil, errors.WithMessage(err, "could not pad")
return nil, mc.ErrEncrypt
}

if len(iv) != block.BlockSize() {
return nil, errors.Errorf("cipher.NewCBCEncrypter: IV length must equal block size")
return nil, mc.ErrEncrypt
}

ciphertext := make([]byte, len(data))
Expand Down