[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

golang-go.crypto / CVE-2019-11841 / CVE-2017-3204



See attached patch:

# CVE-2017-3204

For CVE-2017-3204, I notice we have the note in data/CVE/list:

[jessie] - golang-go.crypto <ignored> (In jessie no rdeps using SSH,
that version doesn't even support host key validation)

This isn't strictly speaking correct. There is host key validation, it
is disabled by default, but the names are different, so I can understand
the confusion.

--- jessie ---
// HostKeyChecker represents a database of known server host keys.               
type HostKeyChecker interface {                                                  
        // Check is called during the handshake to check server's                
        // public key for unexpected changes. The hostKey argument is            
        // in SSH wire format. It can be parsed using                            
        // ssh.ParsePublicKey. The address before DNS resolution is              
        // passed in the addr argument, so the key can also be checked           
        // against the hostname.                                                 
        Check(addr string, remote net.Addr, algorithm string, hostKey []byte) error
}
--- jessie ---

--- latest ---
type HostKeyCallback func(hostname string, remote net.Addr, key PublicKey) error
--- latest ---

The ClientConfig structure field is also different:

- HostKeyChecker HostKeyChecker
+ HostKeyFallback HostKeyCallback

One change the upstream patch provides in the fix for CVE-2017-3204 is
the addition of two checkers: InsecureIgnoreHostKey() and
FixedHostKey(key)

However the major difference the upstream patch provides is to reject
all host keys if the checker is not provided (as opposed to allowing
them all):

+       if fullConf.HostKeyCallback == nil {
+               c.Close()
+               return nil, nil, nil, errors.New("ssh: must specify HostKeyCallback")
+       }
+

This in turn requires updating all the tests which would now fail.

This change would break all existing applications; hence unfortunately I
don't think it has a place in a Jessie security update.


# CVE-2019-11841

After applying the tests patch by hand I noticed that the tests were
hanging. This was fixed upstream by:

https://go.googlesource.com/crypto/+/1bae088edb428672a48c02abd9ef6d889afe0af6%5E!/

I applied this patch by hand too. Now the tests run.

-- 
Brian May <brian@linuxpenguins.xyz>
https://linuxpenguins.xyz/brian/
diff -Nru golang-go.crypto-0.0~hg190/debian/changelog golang-go.crypto-0.0~hg190/debian/changelog
--- golang-go.crypto-0.0~hg190/debian/changelog	2019-06-30 20:28:30.000000000 +1000
+++ golang-go.crypto-0.0~hg190/debian/changelog	2019-09-12 07:16:50.000000000 +1000
@@ -1,3 +1,10 @@
+golang-go.crypto (0.0~hg190-1+deb8u2) jessie-security; urgency=high
+
+  * Non-maintainer upload by the LTS Team.
+  * CVE-2019-11841 Add protection for spoofed Hash header.
+
+ -- Brian May <bam@debian.org>  Thu, 12 Sep 2019 07:16:50 +1000
+
 golang-go.crypto (0.0~hg190-1+deb8u1) jessie-security; urgency=medium
 
   * Non-maintainer upload.
diff -Nru golang-go.crypto-0.0~hg190/debian/patches/CVE-2019-11841.patch golang-go.crypto-0.0~hg190/debian/patches/CVE-2019-11841.patch
--- golang-go.crypto-0.0~hg190/debian/patches/CVE-2019-11841.patch	1970-01-01 10:00:00.000000000 +1000
+++ golang-go.crypto-0.0~hg190/debian/patches/CVE-2019-11841.patch	2019-09-12 07:08:47.000000000 +1000
@@ -0,0 +1,238 @@
+commit c05e17bb3b2dca130fc919668a96b4bec9eb9442
+Author: Filippo Valsorda <filippo@golang.org>
+Date:   Tue Apr 23 15:32:34 2019 -0400
+
+    openpgp/clearsign: reject potentially misleading headers and messages
+    
+    Aida Mynzhasova of SEC Consult Vulnerability Lab reported that the
+    clearsign package accepts some malformed messages, which can make it
+    possible for an attacker to trick a human user (but not a Go program)
+    into thinking unverified text is part of the message.
+    
+    For example, if in the following message the vertical tab character is
+    printed as a new line, a human observer could believe that the
+    unverified header text is part of the signed message.
+    
+    -----BEGIN PGP SIGNED MESSAGE-----
+    Hash: SHA1\x0b\x0bThis text is part of the header.
+    
+    Hello world!
+    -----BEGIN PGP SIGNATURE-----
+    Version: GnuPG v1.4.10 (GNU/Linux)
+    
+    iJwEAQECAAYFAk8kMuEACgkQO9o98PRieSpMsAQAhmY/vwmNpflrPgmfWsYhk5O8
+    [...]
+    MyTpno24AjIAGb+mH1U=
+    =hIJ6
+    -----END PGP SIGNATURE-----
+    
+    The OpenPGP specs are delightfully vague about purpose and validation of
+    these headers. RFC 4880, Section 7 says
+    
+        The cleartext signed message consists of:
+    
+            - The cleartext header '-----BEGIN PGP SIGNED MESSAGE-----' on a
+            single line,
+    
+            - One or more "Hash" Armor Headers,
+    
+            - Exactly one empty line not included into the message digest,
+            [...]
+    
+    but also
+    
+        If MD5 is the only hash used, then an
+        implementation MAY omit this header for improved V2.x compatibility.
+    
+    and
+    
+        If more than one message digest is used in the signature, the "Hash"
+        armor header contains a comma-delimited list of used message digests.
+    
+    which seems to suggest that there can be zero or more Hash headers, each
+    with one or more algorithms, and no other header types.
+    
+    Anyway, it's entirely unclear what security purpose, if any, the Hash
+    header accomplishes. If the hash is too weak to be secure or
+    unsupported, the verification will fail. Otherwise, the user shouldn't
+    care. Given its dubious function, avoid breaking abstractions to check
+    that it matches the signature, and just document it as unverified.
+    
+    As for valid characters, RFC 4880 is silent, except reluctantly
+    mentioning that the Comment header can be UTF-8, but I am going to
+    assume that all hash algorithms will have ASCII names, because come on.
+    
+    Even more importantly, reject non-Hash SIGNED MESSAGE headers (as opposed
+    to the SIGNATURE headers), to prevent a "Thank you!" message turning into
+    
+    -----BEGIN PGP SIGNED MESSAGE-----
+    Reminder: I need you to wire $100k to 12345566 as soon as possible.
+    
+    Thank you!
+    -----BEGIN PGP SIGNATURE-----
+    [...]
+    
+    While at it, also check for trailing characters after the signed message
+    delimiter, as they are invalid and can be similarly used to confuse humans.
+    
+    The Decode API is also unfortunate in that it doesn't return an error,
+    so we can't tell the user what's wrong with the message, but that's what
+    we've got.
+    
+    Change-Id: I8a72c4851075337443d7a27e0b49a6b6e39f5a41
+    Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/453011
+    Reviewed-by: Adam Langley <agl@google.com>
+    Reviewed-on: https://go-review.googlesource.com/c/crypto/+/173778
+    Run-TryBot: Filippo Valsorda <filippo@golang.org>
+    Reviewed-by: Adam Langley <agl@golang.org>
+    TryBot-Result: Gobot Gobot <gobot@golang.org>
+
+--- a/openpgp/clearsign/clearsign.go
++++ b/openpgp/clearsign/clearsign.go
+@@ -17,6 +17,7 @@
+ 	"io"
+ 	"net/textproto"
+ 	"strconv"
++	"strings"
+ 
+ 	"code.google.com/p/go.crypto/openpgp/armor"
+ 	"code.google.com/p/go.crypto/openpgp/errors"
+@@ -26,7 +27,7 @@
+ // A Block represents a clearsigned message. A signature on a Block can
+ // be checked by passing Bytes into openpgp.CheckDetachedSignature.
+ type Block struct {
+-	Headers          textproto.MIMEHeader // Optional message headers
++	Headers          textproto.MIMEHeader // Optional unverified Hash headers
+ 	Plaintext        []byte               // The original message text
+ 	Bytes            []byte               // The signed message
+ 	ArmoredSignature *armor.Block         // The signature block
+@@ -68,8 +69,13 @@
+ 	return data[0:i], data[j:]
+ }
+ 
+-// Decode finds the first clearsigned message in data and returns it, as well
+-// as the suffix of data which remains after the message.
++// Decode finds the first clearsigned message in data and returns it, as well as
++// the suffix of data which remains after the message. Any prefix data is
++// discarded.
++//
++// If no message is found, or if the message is invalid, Decode returns nil and
++// the whole data slice. The only allowed header type is Hash, and it is not
++// verified against the signature hash.
+ func Decode(data []byte) (b *Block, rest []byte) {
+ 	// start begins with a newline. However, at the very beginning of
+ 	// the byte array, we'll accept the start string without it.
+@@ -82,8 +88,11 @@
+ 		return nil, data
+ 	}
+ 
+-	// Consume the start line.
+-	_, rest = getLine(rest)
++	// Consume the start line and check it does not have a suffix.
++	suffix, rest := getLine(rest)
++	if len(suffix) != 0 {
++		return nil, data
++	}
+ 
+ 	var line []byte
+ 	b = &Block{
+@@ -102,21 +111,35 @@
+ 			break
+ 		}
+ 
++		// Reject headers with control or Unicode characters.
++		if i := bytes.IndexFunc(line, func(r rune) bool {
++			return r < 0x20 || r > 0x7e
++		}); i != -1 {
++			return nil, data
++		}
++
+ 		i := bytes.Index(line, []byte{':'})
+ 		if i == -1 {
+ 			return nil, data
+ 		}
+ 
+-		key, val := line[0:i], line[i+1:]
+-		key = bytes.TrimSpace(key)
+-		val = bytes.TrimSpace(val)
+-		b.Headers.Add(string(key), string(val))
++		key, val := string(line[0:i]), string(line[i+1:])
++		key = strings.TrimSpace(key)
++		if key != "Hash" {
++			return nil, data
++		}
++		val = strings.TrimSpace(val)
++		b.Headers.Add(key, val)
+ 	}
+ 
+ 	for {
+ 		start := rest
+ 
+ 		line, rest = getLine(rest)
++		if len(line) == 0 && len(rest) == 0 {
++			// No armored data was found, so this isn't a complete message.
++			return nil, data
++		}
+ 		if bytes.Equal(line, endText) {
+ 			// Back up to the start of the line because armor expects to see the
+ 			// header line.
+--- a/openpgp/clearsign/clearsign_test.go
++++ b/openpgp/clearsign/clearsign_test.go
+@@ -159,3 +159,57 @@
+ =zNCn
+ -----END PGP PRIVATE KEY BLOCK-----
+ `
++
++const signatureBlock = `
++-----BEGIN PGP SIGNATURE-----
++Version: OpenPrivacy 0.99
++
++yDgBO22WxBHv7O8X7O/jygAEzol56iUKiXmV+XmpCtmpqQUKiQrFqclFqUDBovzS
++vBSFjNSiVHsuAA==
++=njUN
++-----END PGP SIGNATURE-----
++`
++
++var invalidInputs = []string{
++	`
++-----BEGIN PGP SIGNED MESSAGE-----
++Hash: SHA256
++
++(This message was truncated.)
++`,
++	`
++-----BEGIN PGP SIGNED MESSAGE-----garbage
++Hash: SHA256
++
++_o/
++` + signatureBlock,
++	`
++garbage-----BEGIN PGP SIGNED MESSAGE-----
++Hash: SHA256
++
++_o/
++` + signatureBlock,
++	`
++-----BEGIN PGP SIGNED MESSAGE-----
++Hash: SHA` + "\x0b\x0b" + `256
++
++_o/
++` + signatureBlock,
++	`
++-----BEGIN PGP SIGNED MESSAGE-----
++NotHash: SHA256
++
++_o/
++` + signatureBlock,
++}
++
++func TestParseInvalid(t *testing.T) {
++	for i, input := range invalidInputs {
++		if b, rest := Decode([]byte(input)); b != nil {
++			t.Errorf("#%d: decoded a bad clearsigned message without any error", i)
++		} else if string(rest) != input {
++			t.Errorf("#%d: did not return all data with a bad message", i)
++		}
++	}
++}
++
diff -Nru golang-go.crypto-0.0~hg190/debian/patches/series golang-go.crypto-0.0~hg190/debian/patches/series
--- golang-go.crypto-0.0~hg190/debian/patches/series	2019-06-30 20:23:13.000000000 +1000
+++ golang-go.crypto-0.0~hg190/debian/patches/series	2019-09-10 17:43:30.000000000 +1000
@@ -1 +1,2 @@
 0001-salsa20-salsa-fix-keystream-loop-in-amd64-assembly-w.patch
+CVE-2019-11841.patch

Reply to: