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

fix tls-psk in net #17741

Closed
wants to merge 12 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion azure-pipelines.yml
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ jobs:
displayName: 'Install dependencies (i386 Linux)'
Copy link
Member

Choose a reason for hiding this comment

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

i can't see the "re-run jobs" option mentioned in contributing.html to restart that crashed check

see https://nim-lang.github.io/Nim/contributing.html#the-cmdgit-stuff-continuous-integration-ci

follow these instructions to only restart the jobs that failed:

(feel free to improve wording if unclear; note that some of these instructions may require sufficient permisssions in nim repo, or not, i fogot)

Copy link
Collaborator

Choose a reason for hiding this comment

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

They only work if you have commit permission in nim-lang/nim afaik.

Copy link
Member

Choose a reason for hiding this comment

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

PR welcome to clarify this in the doc (and whether it's for both azure and github actions or just 1 of those)

condition: and(eq(variables['Agent.OS'], 'Linux'), eq(variables['CPU'], 'i386'))

- bash: brew install boehmgc make sfml
- bash: brew install boehmgc make sfml openssl
displayName: 'Install dependencies (OSX)'
condition: eq(variables['Agent.OS'], 'Darwin')

Expand Down
100 changes: 44 additions & 56 deletions lib/pure/net.nim
Original file line number Diff line number Diff line change
Expand Up @@ -120,21 +120,16 @@ when defineSsl:
SslContext* = ref object
context*: SslCtx
referencedData: HashSet[int]
extraInternal: SslContextExtraInternal

SslAcceptResult* = enum
AcceptNoClient = 0, AcceptNoHandshake, AcceptSuccess

SslHandshakeType* = enum
handshakeAsClient, handshakeAsServer

SslClientGetPskFunc* = proc(hint: string): tuple[identity: string, psk: string]
SslClientGetPskFunc* = proc(hint: string): tuple[identity: string, psk: string]{.nimcall.}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

technically, anything other than {.closure.} is fine, is this over-restrictive?

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how it is over-restrictive, the last thing we need is more generic bloat in Nim's core libraries.


SslServerGetPskFunc* = proc(identity: string): string

SslContextExtraInternal = ref object of RootRef
serverGetPskFunc: SslServerGetPskFunc
clientGetPskFunc: SslClientGetPskFunc
SslServerGetPskFunc* = proc(identity: string): string{.nimcall.}
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change. The only reason to justify this change is that it's security critical. Is that the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

status quo is: servers using the psk API in net, in Nim >0.15.2 will compile and run happily, but sigsegv when a client initiates a connection with psk.

this is a DDoS vulnerability, at least.

Lets say Hyrum's Law applies, and someone is using this to reliably crash their servers remotely, and have set it up to work with a closure. That will no longer compile. I understand that's a break.

Maybe there's a way to allow embedding a closure in a {.cdecl.} callback but at the risk of sounding lazy or stupid I
a) couldn't figure it out:
the farthest i can get is tnetpsk.nim(42, 30) Error: internal error: inconsistent environment type
b) I doubt the benefit of the added code complexity. ref your first comment on this pr: #17741 (comment)

no ego here, close away if my solution isn't aligned with Nim's requirements, but i honestly can't figure out how to make this work any other way. The fact that I had to add an openssl dependency to the azure osx CI just for this test alone makes this whole PR a bit suspect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nvm I've an idea

Copy link
Member

Choose a reason for hiding this comment

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

this should work: #17741 (comment)


else:
type
Expand Down Expand Up @@ -611,6 +606,9 @@ when defineSsl:
when not defined(openssl10) and not defined(libressl):
let sslVersion = getOpenSSLVersion()
if sslVersion >= 0x010101000 and not sslVersion == 0x020000000:
# XXX always false!
Copy link
Collaborator

Choose a reason for hiding this comment

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

How?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it's parsed as ((not sslVersion) == 0x020000000)

# XXX however, setting non-1.3 ciphers with ciphersuites will
# XXX cause an error, cipherList needs to be split into 1.3 and non-1.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

It does? According to the man page:

Items that are not recognized, because the corresponding ciphers are not compiled in or because they are mistyped, are simply ignored. Failure is only flagged if no ciphers could be collected at all.

Am I missing anything here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see, if you have a cipher list without 1.3 ciphers then it would error out.

I really hate how openssl decides that it needs two functions for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

found it when testing this, trying to specify only a PSK cipher

# In OpenSSL >= 1.1.1, TLSv1.3 cipher suites can only be configured via
# this API.
if newCTX.SSL_CTX_set_ciphersuites(cipherList) != 1:
Expand Down Expand Up @@ -658,11 +656,7 @@ when defineSsl:
if not found:
raise newException(IOError, "No SSL/TLS CA certificates found.")

result = SslContext(context: newCTX, referencedData: initHashSet[int](),
extraInternal: new(SslContextExtraInternal))

proc getExtraInternal(ctx: SslContext): SslContextExtraInternal =
return ctx.extraInternal
result = SslContext(context: newCTX, referencedData: initHashSet[int]())

proc destroyContext*(ctx: SslContext) =
## Free memory referenced by SslContext.
Expand All @@ -678,56 +672,51 @@ when defineSsl:
## Sets the identity hint passed to server.
##
## Only used in PSK ciphersuites.
if ctx.context.SSL_CTX_use_psk_identity_hint(hint) <= 0:
if ctx.context.SSL_CTX_use_psk_identity_hint(hint.cstring) <= 0:
raiseSSLError()

proc clientGetPskFunc*(ctx: SslContext): SslClientGetPskFunc =
return ctx.getExtraInternal().clientGetPskFunc
Comment on lines -684 to -685
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep this? Removing it will be a breaking change plus being able to retrieve it seems useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that's possible. However this was a chance to make a clean sweep and I felt freer to make 'breaking' changes as none of the psk functionality has worked since before 1.0 so i know i'm not breaking anyone's code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bear in mind the pskfunc is probably declared as a proc and can be retrieved by name. it's static, after all.


proc pskClientCallback(ssl: SslPtr; hint: cstring; identity: cstring;
max_identity_len: cuint; psk: ptr cuchar;
max_psk_len: cuint): cuint {.cdecl.} =
let ctx = SslContext(context: ssl.SSL_get_SSL_CTX)
let hintString = if hint == nil: "" else: $hint
let (identityString, pskString) = (ctx.clientGetPskFunc)(hintString)
if pskString.len.cuint > max_psk_len:
return 0
if identityString.len.cuint >= max_identity_len:
return 0
copyMem(identity, identityString.cstring, identityString.len + 1) # with the last zero byte
copyMem(psk, pskString.cstring, pskString.len)

return pskString.len.cuint

proc `clientGetPskFunc=`*(ctx: SslContext, fun: SslClientGetPskFunc) =
## Sets function that returns the client identity and the PSK based on identity
## hint from the server.
##
## Only used in PSK ciphersuites.
ctx.getExtraInternal().clientGetPskFunc = fun
ctx.context.SSL_CTX_set_psk_client_callback(
if fun == nil: nil else: pskClientCallback)

proc serverGetPskFunc*(ctx: SslContext): SslServerGetPskFunc =
return ctx.getExtraInternal().serverGetPskFunc
template genpskServerCallback(pskfunc: SslServerGetPskFunc): auto =
proc pskServerCallback(ssl: SslCtx; identity: cstring; psk: ptr cuchar;
max_psk_len: cint): cuint {.cdecl.} =
let pskString = pskfunc($identity)
if pskString.len.cint > max_psk_len:
return 0
copyMem(psk, pskString.cstring, pskString.len)
return pskString.len.cuint

proc pskServerCallback(ssl: SslCtx; identity: cstring; psk: ptr cuchar;
max_psk_len: cint): cuint {.cdecl.} =
let ctx = SslContext(context: ssl.SSL_get_SSL_CTX)
let pskString = (ctx.serverGetPskFunc)($identity)
if pskString.len.cint > max_psk_len:
return 0
copyMem(psk, pskString.cstring, pskString.len)
pskServerCallback

return pskString.len.cuint

proc `serverGetPskFunc=`*(ctx: SslContext, fun: SslServerGetPskFunc) =
proc `serverGetPskFunc=`*(ctx: SslContext, fun: static SslServerGetPskFunc) =
## Sets function that returns PSK based on the client identity.
## Call with nil to remove the callback
##
## Only used in PSK ciphersuites.
ctx.getExtraInternal().serverGetPskFunc = fun
ctx.context.SSL_CTX_set_psk_server_callback(if fun == nil: nil
else: pskServerCallback)
ctx.context.SSL_CTX_set_psk_server_callback(
when fun.isNil: nil else: genpskServerCallback(fun))

template genpskClientCallback(pskfunc: SslClientGetPskFunc): auto =
proc pskClientCallback(ssl: SslPtr; hint: cstring; identity: cstring;
max_identity_len: cuint; psk: ptr cuchar;
max_psk_len: cuint): cuint {.cdecl.} =
let (identityString, pskString) = pskfunc($hint)
if pskString.len.cuint > max_psk_len:
return 0
if identityString.len.cuint >= max_identity_len:
return 0
copyMem(identity, identityString.cstring, identityString.len + 1) # with the last zero byte
copyMem(psk, pskString.cstring, pskString.len)
return pskString.len.cuint

pskClientCallback

proc `clientGetPskFunc=`*(ctx: SslContext, fun: static SslClientGetPskFunc) =
Copy link
Contributor

Choose a reason for hiding this comment

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

Why static here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without static, the fun makes the callback into a closure, which is of course incompatible with openssl.

This is the tradeoff to make psk work at all: the callback function must be known at compile-time. As the test shows, this is not a big issue, ergonomically it's pretty transparent and even lambdas work (as long as they are .nimcall)

As I learned from following your work, accessing the SslClientGetPskFunc at runtime consistently isn't possible, as the callback only has access to the raw ssl context, not the Nim wrapper context. Which is why your solution was to include it within the ex_data. But to get it back out, the callback needs to know the index of said data. which it also cant know.

Furthermore, we can't just assume that the index is 0, as the ex_data is not per context, but global across all contexts, so that solution doesn't work reliably, either.

So I concluded that to keep the abstraction of SslClientGetPskFunc it would have to be a compile-time rewriting, hence the template.

Copy link
Member

Choose a reason for hiding this comment

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

a closure in nim is just 2 function pointers; doesn't openssl allow passing some sort of void* context, with which we could pass the environment needed for the closure?

Copy link
Contributor Author

@shirleyquirk shirleyquirk Apr 21, 2021

Choose a reason for hiding this comment

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

the sslcallback doesn't allow for extra arguments, or we'd pass in the nimcallback there. it's:

pskClientCallback(ssl: SslPtr; hint: cstring; identity: cstring; 
  max_identity_len: cuint; psk: ptr cuchar; max_psk_len: cuint): cuint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perhaps the signature could be better expressed as identity: var UncheckedArray[cchar] and psk: var UncheckedArray[cuchar] but that's maybe yak shaving

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you already annotated SslClientGetPskFunc with {.nimcall.}, static shouldn't be necessary afaik.

Copy link
Contributor Author

@shirleyquirk shirleyquirk Apr 23, 2021

Choose a reason for hiding this comment

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

you'd have thought so, but if i remove the static the compiler tries it's best to make it a closure anyway and I get an error. maybe that's a bug i should submit.

Copy link
Member

Choose a reason for hiding this comment

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

the sslcallback doesn't allow for extra arguments

here's how to pass callbacks: #17741 (comment)

## Sets function that returns the client identity and the PSK based on identity
## hint from the server.
## Call with nil to remove the callback.
##
## Only used in PSK ciphersuites.
ctx.context.SSL_CTX_set_psk_client_callback(
when fun.isNil: nil else: genpskClientCallback(fun))

proc getPskIdentity*(socket: Socket): string =
## Gets the PSK identity provided by the client.
Expand All @@ -754,7 +743,6 @@ when defineSsl:
socket.sslNoShutdown = false
if socket.sslHandle == nil:
raiseSSLError()

if SSL_set_fd(socket.sslHandle, socket.fd) != 1:
raiseSSLError()

Expand Down
77 changes: 77 additions & 0 deletions tests/stdlib/tnetpsk.nim
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
discard """
joinable:false
batchable:false
matrix: "--threads:on -d:ssl"
targets: "c cpp"
timeout:5
"""
import std/net
from std/openssl import SSL_CTX_ctrl

when defined(osx):
{.passl:"-Wl,-rpath,/usr/local/opt/openssl/lib".}

# using channels_builtin
var serverChannel: Channel[Port]

proc clientFunc(identityHint: string): tuple[identity: string, psk: string] =
doAssert identityHint == "bartholomew"
return ("aethelfridda", "aethelfridda-loves-" & identityHint)

proc client(p: Port){.thread.}=
let context = newContext(cipherList = "PSK-AES256-CBC-SHA")
defer: context.destroyContext()

# turn off tls1_3 to force connection over psk
doAssert context.context.SSL_CTX_ctrl(124, 0x0303, nil) > 0 # SSL_CTX_set_max_proto_version(TLS1_2)
context.clientGetPskFunc = clientFunc

let sock = newSocket()
defer: sock.close()

sock.connect("localhost", p)
context.wrapConnectedSocket(sock, handshakeAsClient)

sock.send("hello from aethelfridda\r\l")
doAssert sock.recvLine() == "goodbye from bartholomew"

proc server(){.thread.}=
let context = newContext(cipherList="PSK-AES256-CBC-SHA")
context.pskIdentityHint = "bartholomew"
context.serverGetPskFunc = proc(identity: string): string = identity & "-loves-bartholomew"
context.sessionIdContext= "anything"

let sock = newSocket()
defer:
sock.close()
context.destroyContext()
sock.bindAddr(Port(0))
let (_, port) = sock.getLocalAddr()
serverChannel.send(port)
sock.listen()
var client = new(Socket)
sock.accept(client)
sock.setSockOpt(OptReuseAddr, true)
context.wrapConnectedSocket(client, handshakeAsServer)
doAssert client.getPskIdentity() == "aethelfridda"
doAssert recvLine(client) == "hello from aethelfridda"
client.send("goodbye from bartholomew\r\l")

proc main()=
var
srv:Thread[void]
cli:Thread[Port]
serverChannel.open()
defer: serverChannel.close()

createThread(srv,server)

# wait for server to bind a port
let port = serverChannel.recv()

createThread(cli, client, port)

joinThread(srv)
joinThread(cli)

main()