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

x/crypto: AcceptSecContext always given first outputToken from InitSecContext #43875

Closed
9072997 opened this issue Jan 24, 2021 · 7 comments
Closed
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@9072997
Copy link

9072997 commented Jan 24, 2021

What version of Go are you using (go version)?

go version go1.15.7 windows/amd64

Does this issue reproduce with the latest release?

I believe 1.15.7 to be the latest release, so yes?

What operating system and processor architecture are you using (go env)?

go env Output
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\jpenn\AppData\Local\go-build
set GOENV=C:\Users\jpenn\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\jpenn\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\jpenn\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=c:\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\jpenn\AppData\Local\Temp\go-build470606270=/tmp/go-build -gno-record-gcc-switches

What did you do?

See this program.

This is a dumb and broken implementation of GSSAPIServer and GSSAPIClient for use with crypto/ssh, but it exposes what I think is a bug. As I understand it, InitSecContext and AcceptSecContext should be called back-and-forth, each being given the outputToken of the other. This does not appear to be what is happening. Instead it looks like AcceptSecContext is always given the first outputToken returned by InitSecContext.

What did you expect to see?

CLIENT: InitSecContext(host@HostnameSuppliedByClient, [], false): [100] true 
SERVER: AcceptSecContext([100]): [50 1], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [50 1], false): [101] true 
SERVER: AcceptSecContext([101]): [51 2], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [51 2], false): [102] true 
SERVER: AcceptSecContext([102]): [52 3], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [52 3], false): [103] true 
SERVER: AcceptSecContext([103]): [53 4], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [53 4], false): [104] true 
SERVER: AcceptSecContext([104]): [54 5], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [54 5], false): [105] true 
SERVER: AcceptSecContext([105]): [55 6], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [55 6], false): [106] true 
SERVER: AcceptSecContext([106]): [56 7], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [56 7], false): [107] true 
SERVER: AcceptSecContext([107]): [57 8], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [57 8], false): [108] true 
SERVER: AcceptSecContext([108]): [58 9], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [58 9], false): [109] true 
SERVER: AcceptSecContext([109]): [59 10], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [59 10], false): [] false 
...

What did you see instead?

CLIENT: InitSecContext(host@HostnameSuppliedByClient, [], false): [100] true 
SERVER: AcceptSecContext([100]): [50 1], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [50 1], false): [101] true 
SERVER: AcceptSecContext([100]): [50 2], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [50 2], false): [102] true 
SERVER: AcceptSecContext([100]): [50 3], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [50 3], false): [103] true 
SERVER: AcceptSecContext([100]): [50 4], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [50 4], false): [104] true 
SERVER: AcceptSecContext([100]): [50 5], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [50 5], false): [105] true 
SERVER: AcceptSecContext([100]): [50 6], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [50 6], false): [106] true 
SERVER: AcceptSecContext([100]): [50 7], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [50 7], false): [107] true 
SERVER: AcceptSecContext([100]): [50 8], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [50 8], false): [108] true 
SERVER: AcceptSecContext([100]): [50 9], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [50 9], false): [109] true 
SERVER: AcceptSecContext([100]): [50 10], UsernameDeterminedByServer, true, 
CLIENT: InitSecContext(host@HostnameSuppliedByClient, [50 10], false): [] false 
...
@gopherbot gopherbot added this to the Unreleased milestone Jan 24, 2021
@9072997 9072997 changed the title x/crypto: x/crypto: AcceptSecContext always given first outputToken from InitSecContext Jan 24, 2021
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 24, 2021
@seankhliao
Copy link
Member

apparently nothing is done with the packet received from the client

https://github.com/golang/crypto/blob/eec23a3978adcfd26c29f4153eaa3e3d9b2cc53a/ssh/server.go#L329-L356

@seankhliao seankhliao added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 24, 2021
bodgit added a commit to bodgit/crypto that referenced this issue Jan 25, 2021
This fixes the case where AcceptSecContext is always called with the
first token sent by the client instead of the most recently sent one.

Fixes golang/go#43875
@gopherbot
Copy link

Change https://golang.org/cl/286252 mentions this issue: ssh: Use the correct token from the client

@9072997
Copy link
Author

9072997 commented Mar 29, 2023

Could someone take a look at bodgit's PR? The issue continues to be present in the most recent release of go & x/crypto.

@drakkan
Copy link
Member

drakkan commented Sep 17, 2023

Hello @9072997, @bodgit

thanks for the patch and sorry for all the time it took to respond.

The patch in principle looks correct.
However I have tried a real GSSAPI implementation and have never had more than one token exchange, the continue flag returned from AcceptSecContext is always false in my basic test case.

Can you please provide a way to test this patch in a real usage scenario so we can double check it? Thank you!

@9072997
Copy link
Author

9072997 commented Sep 18, 2023

I have just open-sourced a portion of the internal project which was the motivation for the bug report in the first place (link)

Running the example program currently looks like this:

CLIENT g63Q: New Session
CLIENT: SPN: "HOST/0506TZ9SZ2"
CLIENT g63Q: InitSecContext("host@0506TZ9SZ2", [0], false): [129] true <nil>
SERVER JYUn: New Session
SERVER JYUn: SPN: "HOST/0506TZ9SZ2"
SERVER JYUn: AcceptSecContext([129]): [339] "" true <nil>
CLIENT g63Q: InitSecContext("host@0506TZ9SZ2", [339], false): [121] true <nil>
SERVER JYUn: AcceptSecContext([129]): [0] "" false negotiate.ServerContext.Update: The token supplied to the function is invalid
SERVER JYUn: DeleteSecContext(): <nil>
CLIENT g63Q: DeleteSecContext(): <nil>
panic: ssh: handshake failed: ssh: unable to authenticate, attempted methods [none gssapi-with-mic], no supported methods remain

goroutine 1 [running]:
main.client({0xc000018630, 0x13}, {0xc00000eae0, 0xa})
        C:/Users/jpenn/Downloads/chocolatey-management-master/sshsso/examples/main.go:141 +0x336
main.main()
        C:/Users/jpenn/Downloads/chocolatey-management-master/sshsso/examples/main.go:193 +0x125
exit status 2

after updating go.mod to include the line
replace golang.org/x/crypto => github.com/bodgit/crypto v0.0.0-20210125101325-0d58e4d50014

running the program looks like this

CLIENT Ar7Z: New Session
CLIENT: SPN: "HOST/0506TZ9SZ2"
CLIENT Ar7Z: InitSecContext("host@0506TZ9SZ2", [0], false): [129] true <nil>
SERVER 7KHT: New Session
SERVER 7KHT: SPN: "HOST/0506TZ9SZ2"
SERVER 7KHT: AcceptSecContext([129]): [339] "" true <nil>
CLIENT Ar7Z: InitSecContext("host@0506TZ9SZ2", [339], false): [121] true <nil>
SERVER 7KHT: GetUsername Error: negotiate.ServerContext.GetUsername: The function requested is not supported
SERVER 7KHT: Falling back to ImpersonateUser
SERVER 7KHT: Username(): "SYSTEM" <nil>
SERVER 7KHT: AcceptSecContext([121]): [29] "SYSTEM" false <nil>
CLIENT Ar7Z: InitSecContext("host@0506TZ9SZ2", [29], false): [0] false <nil>
CLIENT Ar7Z: GetMIC([97]): [16] <nil>
SERVER 7KHT: VerifyMIC([97], [16]): <nil>
AllowLogin(__, "SYSTEM")
&{transport:0xc00033c000 sshConn:{conn:0xc0000a2030 user:NT AUTHORITY\SYSTEM sessionID:[16 80 26 100 49 60 27 198 137 76 127 46 25 55 245 195 243 178 135 17 120 143 98 105 24 155 160 87 44 116 192 125] clientVersion:[83 83 72 45 50 46 48 45 71 111] serverVersion:[83 83 72 45 50 46 48 45 71 111]} mux:<nil>}
TrueUsername: "SYSTEM"  ClaimedUsername: "SYSTEM"
SERVER 7KHT: DeleteSecContext(): <nil>
SERVER: New connection from "[fe80::2c45:f331:c01d:f57c%Ethernet]:30845"
CLIENT Ar7Z: DeleteSecContext(): <nil>
Hello NT AUTHORITY\SYSTEM

@drakkan
Copy link
Member

drakkan commented Sep 19, 2023

@9072997 thank you!

@bodgit
Copy link

bodgit commented Sep 19, 2023

From what I've seen when using Kerberos, the Acceptor always creates just the one response token, even with mutual auth, so it's more theoretical that this issue could arise.

@9072997 I noticed your example code is using the negotiate SSPI package; I'm fairly sure if you try and use that against something like OpenSSH it won't interoperate, (I think RFC 4462 section 7.3 applies here), which is why I ended up sending the SSPI maintainer a PR to add support for the additional Kerberos SSP package. I guess it's fine if you control both ends of the SSH connection though, and obviously it's helpful here to illustrate the bug. Just thought I'd mention it as an FYI!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants