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.

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

Trail of bits 018 #9674

Merged
merged 11 commits into from
Aug 17, 2020
Next Next commit
TOB-018 remediation
sgmiller committed Aug 6, 2020
commit 305fc6d5a9315f27bca5aa0ba775becf9a44245d
2 changes: 1 addition & 1 deletion command/agent/cert_with_name_end_to_end_test.go
Original file line number Diff line number Diff line change
@@ -186,7 +186,7 @@ func testCertWithNameEndToEnd(t *testing.T, ahWrapping bool) {
continue
}

aesKey, err := dhutil.GenerateSharedKey(pri, resp.Curve25519PublicKey)
aesKey, err := dhutil.GenerateSharedKey(pri, pub, resp.Curve25519PublicKey)
if err != nil {
t.Fatal(err)
}
2 changes: 1 addition & 1 deletion command/agent/cert_with_no_name_end_to_end_test.go
Original file line number Diff line number Diff line change
@@ -183,7 +183,7 @@ func testCertWithNoNAmeEndToEnd(t *testing.T, ahWrapping bool) {
continue
}

aesKey, err := dhutil.GenerateSharedKey(pri, resp.Curve25519PublicKey)
aesKey, err := dhutil.GenerateSharedKey(pri, pub, resp.Curve25519PublicKey)
if err != nil {
t.Fatal(err)
}
2 changes: 1 addition & 1 deletion command/agent/jwt_end_to_end_test.go
Original file line number Diff line number Diff line change
@@ -231,7 +231,7 @@ func testJWTEndToEnd(t *testing.T, ahWrapping bool) {
continue
}

aesKey, err := dhutil.GenerateSharedKey(pri, resp.Curve25519PublicKey)
aesKey, err := dhutil.GenerateSharedKey(pri, pub, resp.Curve25519PublicKey)
if err != nil {
t.Fatal(err)
}
2 changes: 1 addition & 1 deletion command/agent/sink/sink.go
Original file line number Diff line number Diff line change
@@ -205,7 +205,7 @@ func (s *SinkConfig) encryptToken(token string) (string, error) {
resp.Curve25519PublicKey = s.cachedPubKey
}

aesKey, err = dhutil.GenerateSharedKey(s.cachedPriKey, s.cachedRemotePubKey)
aesKey, err = dhutil.GenerateSharedKey(s.cachedPriKey, s.cachedPubKey, s.cachedRemotePubKey)
if err != nil {
return "", errwrap.Wrapf("error deriving shared key: {{err}}", err)
}
41 changes: 35 additions & 6 deletions helper/dhutil/dhutil.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package dhutil

import (
"bytes"
"crypto/aes"
"crypto/cipher"
"crypto/rand"
"crypto/sha256"
"errors"
"fmt"
"golang.org/x/crypto/hkdf"
"io"

"golang.org/x/crypto/curve25519"
@@ -36,21 +39,47 @@ func GeneratePublicPrivateKey() ([]byte, []byte, error) {

// generateSharedKey uses the private key and the other party's public key to
ncabatoff marked this conversation as resolved.
Show resolved Hide resolved
// generate the shared secret.
func GenerateSharedKey(ourPrivate, theirPublic []byte) ([]byte, error) {
func GenerateSharedKey(ourPrivate, ourPublic, theirPublic []byte) ([]byte, error) {
if len(ourPrivate) != 32 {
return nil, fmt.Errorf("invalid private key length: %d", len(ourPrivate))
}
if len(theirPublic) != 32 {
return nil, fmt.Errorf("invalid public key length: %d", len(theirPublic))
}

var scalar, pub, secret [32]byte
copy(scalar[:], ourPrivate)
copy(pub[:], theirPublic)
secret, err := curve25519.X25519(ourPrivate, theirPublic)
if err != nil {
return nil, err
}

// Derive the final key from the HKDF of the secret and public keys.

//These must be consistently ordered we're on "our" side of key negotiation or the other.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain a bit more (or link to some reading material) why the result of the comparison determines the order of arguments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Internally, HKDF hashes the secret and two public keys. If Alice and Bob are doing DH key exchange, Alice calculates:

HKDF(secret, A, B) since ourPublic is A.

Bob calculates HKDF(secret, B, A), since Bob's ours is B. That produces a different value. Now we only care that both public keys participate in the derivation, so simply sorting them so they are in a consistent numerical order (either one would do) arrives at an agreed value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Mind adding that as a comment?

var pub1 []byte
var pub2 []byte
switch bytes.Compare(ourPublic, theirPublic) {
ncabatoff marked this conversation as resolved.
Show resolved Hide resolved
case 0:
return nil, errors.New("same public key supplied for both participants")
case -1:
pub1 = ourPublic
pub2 = theirPublic
case 1:
pub1 = theirPublic
pub2 = ourPublic
}

curve25519.ScalarMult(&secret, &scalar, &pub)
kio := hkdf.New(sha256.New, secret, pub1, pub2)

return secret[:], nil
var key [32]byte
n, err := io.ReadFull(kio, key[:])
if n != 32 {
return nil, errors.New("short read from hkdf")
}
if err != nil {
ncabatoff marked this conversation as resolved.
Show resolved Hide resolved
// Don't return the key along with the error to prevent misuse
return nil, err
}
return key[:], nil
}

// Use AES256-GCM to encrypt some plaintext with a provided key. The returned values are