[BUG] (Fake|Utls)PreSharedKeyExtension about utls

[3andne]

There appears to be a bug/typo, which I'll clarify. In short, the bug is in newSessionController, where we should set these to nil:

utls/u_session_controller.go

Lines 57 to 58 in df6e4c8

	sessionTicketExt: &SessionTicketExtension{},
	pskExtension: &UtlsPreSharedKeyExtension{},

---

The SessionController was designed to manage all session-related operations. Initially, it had ownership over extensions.

But this posed a challenge. The ClientHelloSpec might have a different set of PSK/Session extensions. For clarity, the original design had the SessionController take charge of the extensions, overwriting those from the spec. This leads to the design that newSessionController produces a non-nil session and a psk extension. A downside of this approach was that it altered the signature of a public method, and we didn't have a clear vision for v2.0 yet.

To circumvent this, I pivoted to a strategy where the Spec owns the extensions, ensuring the method signature remains unchanged. However, users should be able to override psk/session extensions before invoking ApplyPreset. These overriding extensions should exclusively reside in the SessionController. In such cases, the SessionController have ownership over the extensions.

This necessitated the creation of the syncSessionExts method to discern which extension takes precedence. The guiding principle is straightforward:

1. If both the user and the spec offer an extension, prioritize the user's.
2. If the spec lacks an extension and the user attempts to provide one, it's deemed an error.
3. In the absence of a user-provided extension, revert to the spec's version, if available.

Yet, during this overhaul, I overlooked the newSessionController method. As illustrated in the comments, deep in my mind I believe that the extension would default to nil unless overridden by the user:

utls/u_session_controller.go

Lines 271 to 273 in df6e4c8

	if s.pskExtension == nil {
	// If there isn't a user-provided psk extension, use the one from the spec
	s.pskExtension = ext

[gaukas]

Interesting observation. I don't believe we are ready to parse raw byte ClientHello into PSK yet. On the other hand it is theoretically not possible to resume the connection with just the PSK ticket and binder from raw bytes.

That said, it might worth debugging and fixing to make at least FakePreSharedKeyExtension work with raw bytes. But note that in the end, unexpected things would happen, such as server rejecting you for submitting a valid ticket with an incorrect binder, etc.

Tagging @3andne for visibility.

[gaukas]

To clarify on the intended behaviors:

• If RealPSKResumption is set to true: parsing from raw bytes will discard the ticket and binders from the raw bytes, but use the available ones from ClientSessionCache instead.
• If RealPSKResumption is false: parsing from raw bytes with legitimate PSK tickets and binders IS GUARANTEED TO FAIL, given that:
  • the server will check the ticket and find it is legitimate, and proceed with verifying the binder. If we are sending binders from the original raw bytes (that's the intended behavior), the binder will not match the expected value which is calculated from all previous bytes in ClientHello.
  • If we were to re-calculated binders, it will be a different challenge for us to find the shared secret for that specific ticket.

[VeNoMouS]

Ok, i'll dive deeper into FakePreSharedKeyExtension and see if i can fix what ever is broken with it (if a PSK is present) .. 🤞

[3andne]

@VeNoMouS No problem! Thanks for your efforts!

[VeNoMouS]

Sorry about the delay in getting a PR sorted... life and all that 😄

[VeNoMouS]

Addtionally ...

Noticed something odd happening when using FakePreSharedKeyExtension its calling the wrong extension to Read() , due to this

[gaukas]

Given all the complexities, I would encourage avoiding reusing raw bytes with PSK, either way it will not work as you expect, either we need to use a different PSK ticket/binder, or the connection will fail outright.

[VeNoMouS]

FWIW, it use to work, prior to the change (when genericExtension 41 was used etc)

[gaukas]

prior to the change (when genericExtension 41 was used etc)

Yep, and in that case, you would actually expect the server to return an error. Just based on my test result, when I use a legitimate PSK Identity with its original binder, the server gives me an error (due to failing the binder check).

Have you actually observed any other behaviors? That would be really intriguing.

And a quick patch here could be just roll back to use GenericExtension by removing the Writer implementation on PSK extensions. Currently, I do not really have a good idea for how to we handle this appropriately ... or, any suggestions?

[VeNoMouS]

Yea i started doing that last night, tho ran into an issue with unable to decrypt error , trying to work out what is going on

[gaukas]

unable to decrypt error

I don't remember exactly what error did I receive when I was testing. But there are essentially two possible outcomes:

• Server rejects your ClientHello (you get a remote error) due to incorrect Binder (Random/GREASE/KeyShare has changed) and binder is not re-calculated
• Server didn't catch that issue and accepted your resumption. Then sent you an encrypted message. You don't REALLY have the selected PSK Identity (more specifically, the corresponding key) so could not decrypt it.

[VeNoMouS]

• Server rejects your ClientHello (you get a remote error) due to incorrect Binder (Random/GREASE/KeyShare has changed) and binder is not re-calculated

This just gave me hint , when i was testing my custom spec i had grease place holder... so i'll test with static grease id's and let you know on that

Switching back to rawBytes and using a genericExtenstion 41 here worked, the PSK loaded from raw bytes and was present on the hello

Not sure why that's in a switch either as its only checking one case ..

[VeNoMouS]

@gaukas what do you think about doing something with this

and replace it with

		if ext != nil && ok { // known extension and implements TLSExtensionWriter properly
			switch extension {
			case extensionPreSharedKey:
				// PSK extension, need to see if we do real or fake PSK
				if realPSK {
					extWriter = &UtlsPreSharedKeyExtension{}
				} else {
					if len(extData) == 0 {
						extWriter = &FakePreSharedKeyExtension{}
					} else {
						extWriter = &GenericExtension{Id: 41}
					}
					
				}
			case extensionSupportedVersions:
				chs.TLSVersMin = 0
				chs.TLSVersMax = 0
			}
			if _, err := extWriter.Write(extData); err != nil {
				return err
			}
			chs.Extensions = append(chs.Extensions, extWriter)

And adding a simple write to the GenericExtension in the tls_extensions

func (e *GenericExtension) Write(b []byte) (int, error) {
	e.Data = make([]byte, len(b))
	n := copy(e.Data, b)
	return n, nil
}

So if the realPSK is false (default) it uses FakePreSharedKeyExtension and if the PSK is populated in the extension data it uses the genericExtension 41, ReadTLSExtensions is only called from FromRaw

[gaukas]

FakePreSharedKeyExtension is actually just a GenericExtension but with PSK's data fields (identity and binders) explicitly defined. So entirely removing the existence of FakePreSharedKeyExtension or fixing the behavior when FakePreSharedKeyExtension would suffice. There's no reason to keep them both.

[VeNoMouS]

Just an update on this, (sorry been busy), I can see something is happening when FakePreSharedKeyExtension does a Read , I can confirm Write does create the Identities and Binders , but when Read is called they are gone for some reason, so I'm trying to work out what is causing that to occur...

Sorry it appears before that, it Does a Write then a Len , and its gone by the time it hits Len

FakePreSharedKeyExtension Write

Dumping e.Identities:  [{[133 74 171 187 44 97 94 24 1 4 49 205 230 205 54 184 229 24 74 171 116 211 248 230 99 190 254 164 80 214 134 51 173 79 189 248 209 74 30 39 144 224 74 52 52 128 110 234 152 231 163 63 229 194 197 231 76 68 219 167 124 123 29 163 228 117 185 253 99 69 235 90 213 87 0 47 149 243 111 119 26 64 115 109 227 121 180 207 231 65 211 81 212 18 25 99 243 90 187 212 180 193 162 193 194 94 35 74 179 12 178 183 134] 2239559142}]

Dumping e.Binders:  [[255 152 173 142 128 210 182 214 210 243 227 171 115 165 68 122 254 155 121 203 158 20 58 170 198 19 182 113 177 145 13 88]]

FakePreSharedKeyExtension Len()

Dumping e.Identities:  []

Dumping e.Binders:  []

Update:

Found the "problem" with the disappearing PSK... its in syncSessionExts on this line

if we remove that line PSK is populated... I need to read the code to understand the impact of removing it

[gaukas]

Thanks for your detailed report and effort in investigation @VeNoMouS!

@3andne I would greatly appreciate if you would love to share some of your insights on this issue.

[VeNoMouS]

What if we did something like the following for that line

if s.pskExtension.Len() != 0 || s.uconnRef.Extensions[i].Len() == 0 {
	s.uconnRef.Extensions[i] = s.pskExtension
}

[gaukas]

It feels like that most likely you do not want to explicitly provide a PSK extension in case you are reading from raw bytes. So I would expect it to hit Line 273 instead.

utls/u_session_controller.go

Lines 271 to 277 in df6e4c8

	if s.pskExtension == nil {
	// If there isn't a user-provided psk extension, use the one from the spec
	s.pskExtension = ext
	} else {
	// Otherwise, replace the one in the extension list with the user-provided one
	s.uconnRef.Extensions[i] = s.pskExtension
	}

[VeNoMouS]

How do you feel about adding

case *FakePreSharedKeyExtension:
	if len(ext.Identities) != 0 && len(ext.Binders) != 0 {
		uconn.sessionController.overridePskExt(ext)
	}

to here?

utls/u_parrots.go

Line 2486 in df6e4c8

}

Obviously that's slower as it goes and calls all the sub functions to pull the whole extension again.. but...

utls/u_session_controller.go

Line 80 in df6e4c8

// - If a pre-shared key (PSK) is already initialized, typically via the `overridePskExt()` function, the PSK should be set in the client hello.

[gaukas]

I'm not sure if that's the correct usage of overridePskExt()... I might need to looks into this a bit.

[VeNoMouS]

It feels like that most likely you do not want to explicitly provide a PSK extension in case you are reading from raw bytes. So I would expect it to hit Line 273 instead.

utls/u_session_controller.go

Lines 271 to 277 in df6e4c8

	if s.pskExtension == nil {
	// If there isn't a user-provided psk extension, use the one from the spec
	s.pskExtension = ext
	} else {
	// Otherwise, replace the one in the extension list with the user-provided one
	s.uconnRef.Extensions[i] = s.pskExtension
	}

other option is simply...

if s.pskExtension == nil || ext.Len() != 0 {

utls/u_session_controller.go

Line 271 in df6e4c8

if s.pskExtension == nil {

THe problem is s.pskExtension isn't nil , but then you get into a problem, of if the PSK is populated from raw bytes, and you want to pass your own extension, how do you overwrite that in the setup etc..

hence why i looked at overridePskExt

[gaukas]

Why would s.pskExtension be set in this case? (Or, why do we need the or) Where is it set and what is it set to?

[VeNoMouS]

Why would s.pskExtension be set in this case? (Or, why do we need the or) Where is it set and what is it set to?

syncSessionExts gets called from ApplyPreset and when it creates the sessionController, newSessionController is called which creates an empty pskExtension

utls/u_session_controller.go

Lines 54 to 64 in df6e4c8

	func newSessionController(uconn *UConn) *sessionController {
	return &sessionController{
	uconnRef: uconn,
	sessionTicketExt: &SessionTicketExtension{},
	pskExtension: &UtlsPreSharedKeyExtension{},
	state: NoSession,
	locked: false,
	callingLoadSession: false,
	loadSessionTracker: NeverCalled,
	}
	}

That said, could just make pskExtension in newSessionController nil 😄 (i tested that and it seems to work)

[gaukas]

i tested that and it seems to work

Have you also tested the psk examples to make sure they all still work?

Let's check this with 3andne first, to make sure we don't miss any part.

[VeNoMouS]

Yea lets wait for @3andne because I can envision some issues with setting it to nil... (or not)... that said overridePskExt might be the better route...

lets wait and see

[VeNoMouS]

IMHO

sessionTicketExt and pskExtension should both be nil innewSessionController, as there is heaps of sanity checks looking for nil that have safe guards in place for it

[3andne]

Hey @VeNoMouS , can you provide a minimal code snippet that can reproduce the behavior. I need your help on the context. Thanks!

[VeNoMouS]

just use the raw bytes in the first comment

and use FingerprintClientHello() with said raw bytes to make spec and a GET to https://tls.peet.ws/api/all and make sure pre_shared_key (41) is present in the extensions listed in the response

[VeNoMouS]

Thanks for looking at this lengthy debug ticket @3andne 😄 , are you ok if I do a PR for newSessionController and have

utls/u_session_controller.go

Lines 57 to 58 in df6e4c8

	sessionTicketExt: &SessionTicketExtension{},
	pskExtension: &UtlsPreSharedKeyExtension{},

Defaulting to nil? or do you want to do that?