Add config field to enable custom client fingerprints #224

bvandersloot · 2017-02-03T17:42:57Z

These changes enable a zTLS consumer to specify exactly the format of the ClientHello message.
This is useful for censorship-resistance, so zTLS clients can camouflage themselves as other traffic.

The mechanism this uses is an additional field in the Config struct, ClientFingerprint, that when non-null specifies the contents of the ClientHello message. When this field is nil, everything proceeds as normal.

I repeat: WHEN THIS CONFIG FIELD IS nil, THESE CODE PATHS ARE NOT HIT

The extensions supported out-of-the-box are:

SNI
ExtendedMasterSecret
Renegotiation Info
Elliptic Curves
EC point formats
SessionTickets
NPN
StatusRequests
Signature and Hash Algorithms
ALPN (pending Pull in ALPN tls extension from golang tls #220 and some minor changes)

But a consumer can also specify their own extensions and roll them in as well.
Custom Session Caches are also allowed.

I should have hello-specific things in handshake_client

zakird · 2017-02-04T16:28:40Z

This seems very similar to #223. Do we need both approaches?

bvandersloot · 2017-02-04T16:39:38Z

We don't need both. Mine is only a modification to the zTLS library, and will not be accessible from the zGrab command line without further changes. My version also does not support sending arbitrary (potentially unimplemented, or even Google black-box) extensions over the wire out-of-the-box.

dadrian · 2017-02-04T19:51:06Z

I'm currently more inclined to this approach than #223. That being said, let's move any broad discussion of goals, etc to #223, rather than having to split threads. If we end up with your approach, I'll come back and review the code here.

dadrian · 2017-02-08T18:16:03Z

@bvandersloot Can you resolve merge conflicts with #223? Then I'll review.

bvandersloot · 2017-02-08T18:16:28Z

Sad!

Conflicts: ztools/ztls/common.go ztools/ztls/handshake_client.go

bvandersloot · 2017-02-08T21:16:00Z

@dadrian: This should be ready for eyes.

dadrian

Here's some comments to start.

dadrian · 2017-02-08T20:58:48Z

ztools/ztls/handshake_client.go

+type NullExtension struct {
+}
+
+func (e NullExtension) Marshal() []byte {


These should probably be pointer receivers.

dadrian · 2017-02-08T21:01:09Z

ztools/ztls/common.go

@@ -226,6 +226,10 @@ type ClientSessionCache interface {
 	Put(sessionKey string, cs *ClientSessionState)
 }

+type ClientExtension interface {
+	Marshal() (data []byte)


You don't need a named result here.

Sure. Changing.

dadrian · 2017-02-08T21:01:50Z

ztools/ztls/common.go

+
+	// If non-null specifies the contents of the client-hello
+	// WARNING: Setting this may invalidate other fields in the Config object
+	ClientFingerprint *ClientHelloConfiguration


Why are these named differently?

Refactoring both to ClientFingerprintConfiguration

dadrian · 2017-02-08T21:04:34Z

ztools/ztls/handshake_client.go

+}
+
+type ClientHelloConfiguration struct {
+	//Version in the handshake header


There should be a space between // and the rest of the comment.

Fixing throughout.

dadrian · 2017-02-08T21:11:15Z

ztools/ztls/handshake_client.go

+func (c *ClientHelloConfiguration) ValidateExtensions() error {
+	for _, ext := range c.Extensions {
+		switch ext.(type) {
+		case PointFormatExtension:


Why are there only two extensions here? What happens in the default case?

Most extensions don't need to make sure their details are implemented, so the default is no action. Validate may not be the best wording. Also, maybe this is best rolled into another function in the Extension interface, and calling it on all extensions here?

I agree, should probably be part of the extension interface.

dadrian · 2017-02-08T21:34:58Z

ztools/ztls/handshake_client.go

+	config.ExtendedMasterSecret = false
+	config.SignedCertificateTimestampExt = false
+	for i, ext := range c.Extensions {
+		switch ext.(type) {


This does not have a default case

adding explicit default: continue

dadrian · 2017-02-08T21:37:22Z

ztools/ztls/handshake_client.go

+	config.SignedCertificateTimestampExt = false
+	for i, ext := range c.Extensions {
+		switch ext.(type) {
+		case SniExtension:


You should be able to do assignment in the switch statement, rather than having to recast inside of each case body.

I forgot you can do that!

dadrian · 2017-02-08T21:46:54Z

ztools/ztls/handshake_client.go

+	for i, ext := range c.Extensions {
+		switch ext.(type) {
+		case SniExtension:
+			if ext.(SniExtension).Autopopulate {


What does autopopulate mean?

Fill in the SNI extension per-request. That way a new Config is not needed per-domain.

dadrian · 2017-02-08T21:47:17Z

ztools/ztls/handshake_client.go

+		case ExtendedMasterSecretExtension:
+			config.ExtendedMasterSecret = true
+		case SignatureAlgorithmExtension:
+			supportedSKXSignatureAlgorithms = ext.(SignatureAlgorithmExtension).getStruct()


getStruct is an incredibly vague function name

:%s/getStruct/getStructuredAlgorithms/g

dadrian · 2017-02-08T21:47:52Z

ztools/ztls/handshake_client.go

+	return config
+}
+
+func (c *ClientHelloConfiguration) marshal(config *Config) ([]byte, error) {


Why is this method on ClientHelloConfiguration? Why does it need a Config?

This is making the on-the-wire version of the ClientHelloConfiguration. I have this distinct from the clientHelloMsg serializer to make clearer that they are very different cases. It needs the Config to specify the random number generator, and to see if the config is specifying ForceSuites.

So a ClientHelloConfiguration still marshals into a TLS ClientHello? It doesn't go through the "normal" ClientHello?

bvandersloot · 2017-02-09T17:09:48Z

ztools/ztls/common.go

@@ -226,6 +226,10 @@ type ClientSessionCache interface {
 	Put(sessionKey string, cs *ClientSessionState)
 }

+type ClientExtension interface {


This also should not be in this file- moving it to be next to the definition of ClientFingerprintConfiguration

dadrian · 2017-02-09T17:12:18Z

Sorry about the somewhat vague comments, I'm still trying to work out a mental model of this in my head.

bvandersloot · 2017-02-09T17:18:30Z

No problem, there is some complexity here.

bvandersloot · 2017-02-09T21:24:55Z

This is ready for another pass @dadrian

dadrian

I've gone through up to marshal.

dadrian · 2017-02-09T22:14:29Z

ztools/ztls/handshake_client.go

+	CacheKey     CacheKeyGenerator
+}
+
+type ClientExtension interface {


Can you document how this interface is supposed to behave?

Adding some comments for each member function

dadrian · 2017-02-09T22:16:29Z

ztools/ztls/handshake_client.go

+		switch casted := ext.(type) {
+		case SniExtension:
+			if casted.Autopopulate {
+				c.Extensions[i] = SniExtension{[]string{config.ServerName}, true}


You should use named fields here, instead of relying on ordering. This also prevents compilation from breaking if you add a field.

c.Extensions[i] = SniExtension{ Domains: []string{config.ServerName}, Autopopulate: true, }

dadrian · 2017-02-09T22:18:13Z

ztools/ztls/handshake_client.go

+			if casted.Autopopulate {
+				c.Extensions[i] = SniExtension{[]string{config.ServerName}, true}
+			}
+			if config.ServerName == "" && len(casted.Domains) > 0 {


I'm a little confused by this logic. We only use casted.Domains if config.ServerName is empty? Can you add some comments to explain what's going on?

Adding a comment to clarify. This is to allow certificate verification to proceed using a SNI name.

dadrian · 2017-02-09T22:19:44Z

ztools/ztls/handshake_client.go

+	config.ExtendedMasterSecret = false
+	config.SignedCertificateTimestampExt = false
+	for i, ext := range c.Extensions {
+		switch casted := ext.(type) {


This entire switch should be made a method in the ClientExtension interface.

WriteToConfig(*Config) error

Adding. Calling for each ext.

dadrian · 2017-02-09T22:21:54Z

ztools/ztls/handshake_client.go

+	} else {
+		_, err := io.ReadFull(config.rand(), head[6:38])
+		if err != nil {
+			return nil, errors.New("tls: short read from Rand: " + err.Error())


I'd probably use fmt.Errorf for this, but /shrug

dadrian · 2017-02-09T22:27:28Z

ztools/ztls/handshake_client.go

+		ciphers[3+i*2] = uint8(suite)
+	}
+
+	compressions := make([]byte, len(c.CompressionMethods)+1)


Same comment about length verification

Shouldn't be a problem with the checks below, but adding because safety is best.

dadrian · 2017-02-09T22:29:25Z

ztools/ztls/handshake_client.go

+		copy(compressions[1:], c.CompressionMethods)
+		if c.CompressionMethods[0] != 0 {
+			return nil, errors.New(fmt.Sprintf("tls: unimplemented compression method %d", c.CompressionMethods[0]))
+		} else if len(c.CompressionMethods) > 1 {


This shouldn't be an else. It should be a separate if

Does the RFC specify to not send a compression method more than once?

So? The network is evil and wants to kill you.

dadrian · 2017-02-09T22:30:04Z

ztools/ztls/handshake_client.go

+
+	var extensions []byte
+	for _, ext := range c.Extensions {
+		extensions = append(extensions, ext.Marshal()...)


Can ext.Marshal() ever fail?

dadrian · 2017-02-09T22:30:39Z

ztools/ztls/handshake_client.go

+		length[1] = uint8(len(extensions))
+		extensions = append(length, extensions...)
+	}
+	hello := append(head, append(sessionID, append(ciphers, append(compressions, extensions...)...)...)...)


This is kind of comical.

Agreed. Do you know a better way to append a bunch of slices together in golang?

Not really. Although, preallocating a slice and then appending in a loop of a [][]byte might read a little easier and be faster? Alternatively, you could try to preallocate everything before you serialize, and then just slice off the relevant chunks as you go.

Fixed in a28b378

dadrian · 2017-02-09T22:31:24Z

ztools/ztls/handshake_client.go

+		extensions = append(length, extensions...)
+	}
+	hello := append(head, append(sessionID, append(ciphers, append(compressions, extensions...)...)...)...)
+	hello[1] = uint8((len(hello) - 4) >> 16)


Save len(hello) - 4 to a variable first

Adding. Also adding a check that len(hello) - 4 <= 1 << 24

bvandersloot · 2017-02-10T02:13:04Z

Comments by @dadrian up to this point should be addressed by 37bbabb

bvandersloot · 2017-02-11T16:25:02Z

ztools/ztls/handshake_client.go

-	ClientRandom []byte
+	// except the top 4 bytes if InsertTimestamp is true
+	ClientRandom    []byte
+	InsertTimestamp bool


This seems like a good thing to do, as far as camouflaging clients go. Android does this, apparently.

bvandersloot · 2017-02-11T16:26:45Z

@dadrian Seems like we are converging. I fixed a few things and added one small feature.

dadrian

Sorry I'm so slow on this. Almost there!

dadrian · 2017-02-10T21:34:32Z

ztools/ztls/handshake_client.go

+	Marshal() []byte
+
+	// Function will return an error if zTLS does not implement the necessary features for this extension
+	CheckImplemented() error


Can you give an example of when this will return non-nil?

In the Hash and Signature Algorithm Extension, if a algorithm combination listed to be offered by the client is not actually in zTLS. Or even a non-defined value.

Oh, I understand now.

dadrian · 2017-02-12T00:40:22Z

ztools/ztls/handshake_client.go

+	}
+
+	// first, let's check if a ClientFingerprintConfiguration template was provided by the config
+	if c.config.ClientFingerprintConfiguration != nil {


This can go in the above if

Merging the above if into this.

dadrian · 2017-02-12T03:06:28Z

ztools/ztls/handshake_client.go

+					candidateSession.vers <= c.config.ClientFingerprintConfiguration.HandshakeVersion
+				if versOk && cipherSuiteOk {
+					session = candidateSession
+				}


Is this not an error case otherwise? Do you have to match version to the previous session if resuming?

dadrian · 2017-02-12T03:07:48Z

ztools/ztls/handshake_client.go

+			}
+		}
+		for i, ext := range c.config.ClientFingerprintConfiguration.Extensions {
+			switch ext.(type) {


This should probably also be part of the extension interface

This is a special case only for session management.

dadrian · 2017-02-12T03:08:09Z

ztools/ztls/handshake_client.go

+		}
+		hello = &clientHelloMsg{}
+		if ok := hello.unmarshal(helloBytes); !ok {
+			return errors.New("Incompatible Custom Client Fingerprint")


Lowercase incompatible

bvandersloot · 2017-02-12T16:47:55Z

No worries, I've had about the same latency between interactions.

dadrian

Almost done!

dadrian · 2017-02-15T01:18:30Z

ztools/ztls/handshake_client.go

+	Marshal() []byte
+
+	// Function will return an error if zTLS does not implement the necessary features for this extension
+	CheckImplemented() error


Oh, I understand now.

dadrian · 2017-02-15T01:21:03Z

ztools/ztls/handshake_extensions.go

+	return []byte{}
+}
+
+type SniExtension struct {


Should be SNIExtension

dadrian · 2017-02-15T01:22:43Z

ztools/ztls/handshake_extensions.go

+}
+
+func (e SupportedCurvesExtension) CheckImplemented() error {
+	return nil


We support all possible curves?

dadrian · 2017-02-15T01:23:20Z

ztools/ztls/handshake_extensions.go

+func (e PointFormatExtension) CheckImplemented() error {
+	for _, format := range e.Formats {
+		if format != pointFormatUncompressed {
+			return errors.New(fmt.Sprintf("Unsupported EC Point Format %d", format))


replace with fmt.Errorf

dadrian · 2017-02-15T01:24:32Z

ztools/ztls/handshake_extensions.go

+	return nil
+}
+
+func (e HeartbeatExtension) CheckImplemented() error {


File an issue that we don't actually support Heartbeat, only kind of.

We can't receive arbitrary Heartbeat messages at arbitrary times.

dadrian · 2017-02-15T01:25:34Z

ztools/ztls/handshake_extensions.go

+	"fmt"
+)
+
+type NullExtension struct {


This file should all be pointer receivers, unless you have a good reason not to.

dadrian · 2017-02-15T20:32:02Z

@zakird Any further comments?

zakird · 2017-02-19T23:19:09Z

This mostly looks fine to me. However, this is a lot of code that touches a lot of things in a lot of places, and I have little confidence from reading it over that it works.

I'd like to see at least a few unit tests that confirm that what goes into a ClientFingerprintConfiguration leads to the expected Client Hello. I suspect you must have done this already somehow to see if any of this worked. Let's just get them into tests.

zakird

I'm going to request changes on this this until there are tests in place. This is a lot of code, and I have little confidence that this works without tests in place. In the longterm, I think this will also benefit you.

bvandersloot · 2017-02-20T20:30:08Z

@zakird, I went with one large functional test that imitates a Firefox hello, rather than trying to unit test. This seems to match the granularity of existing tests. It passes and I manually inspected the hexdump test file and all of the configuration changes were reflected on the wire.

Benjamin VanderSloot added 14 commits January 31, 2017 16:34

Foundation for client-hello customization config

b8bdf1d

Fixing test

b2265d7

Rearranging to be a bit more consistent

18752ab

I should have hello-specific things in handshake_client

First draft of hello marshal

40bf5ba

Bug Fix

4016936

Sanity checking cipher suite and compression method

2773ca9

SNI extension implemented

df0b99f

Dump of many protocol extension marshallers

0c5a933

Squashing bugs

02f5d00

Signature and Hash made global

bb44245

+heartbeat

7517260

Add option to automatically populate SNI extension

c91b52f

Session Cache behavior is added

83a4581

Clarification in the config

94e4a4f

zakird mentioned this pull request Feb 4, 2017

Add external ClientHello support #223

Merged

zakird assigned dadrian Feb 4, 2017

Benjamin VanderSloot added 5 commits February 8, 2017 15:52

Merge branch 'master' into client-fingerprint

cea8180

Conflicts: ztools/ztls/common.go ztools/ztls/handshake_client.go

Restore dropped line

66ee594

Roll extensions into separate file

1a747cd

Adding other disappeared if clause

28c0072

ALPN-pending changes

38ff433

dadrian suggested changes Feb 8, 2017

View reviewed changes

bvandersloot commented Feb 9, 2017

View reviewed changes

Benjamin VanderSloot added 2 commits February 9, 2017 12:33

Code review changes for #224

eb53b07

Build errors resolved

0d2c2cb

dadrian suggested changes Feb 9, 2017

View reviewed changes

More code review changes for #224

37bbabb

Benjamin VanderSloot added 3 commits February 11, 2017 10:59

Explanation of some agl magic

37a4e7f

Adding timestamp in random field compliance

afc5f4e

Append chain removed. Adding loop.

a28b378

bvandersloot commented Feb 11, 2017

View reviewed changes

dadrian suggested changes Feb 12, 2017

View reviewed changes

PR #224 revisions

5081f4d

dadrian suggested changes Feb 15, 2017

View reviewed changes

Next round of changes for PR #224

bf5ed53

dadrian approved these changes Feb 15, 2017

View reviewed changes

zakird suggested changes Feb 19, 2017

View reviewed changes

Benjamin VanderSloot added 3 commits February 20, 2017 15:22

Adding Firefox imitation test

c3f2d23

Fixes to pass new test :)

e74248b

Merge branch 'master' into client-fingerprint

e3a8dcc

zakird approved these changes Feb 26, 2017

View reviewed changes

zakird merged commit bd9d4b8 into master Feb 26, 2017

dadrian deleted the client-fingerprint branch May 3, 2017 14:49

Add config field to enable custom client fingerprints #224

Add config field to enable custom client fingerprints #224

Conversation

bvandersloot commented Feb 3, 2017

zakird commented Feb 4, 2017

bvandersloot commented Feb 4, 2017

dadrian commented Feb 4, 2017

dadrian commented Feb 8, 2017

bvandersloot commented Feb 8, 2017

bvandersloot commented Feb 8, 2017

dadrian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvandersloot Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadrian Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadrian commented Feb 9, 2017

bvandersloot commented Feb 9, 2017

bvandersloot commented Feb 9, 2017

dadrian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvandersloot commented Feb 10, 2017

Choose a reason for hiding this comment

bvandersloot commented Feb 11, 2017

dadrian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvandersloot Feb 9, 2017 •

edited

Loading

dadrian Feb 9, 2017 •

edited

Loading