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 buffer-overrun bug in net #17728

Merged
merged 12 commits into from
Apr 16, 2021
Merged

Fix buffer-overrun bug in net #17728

merged 12 commits into from
Apr 16, 2021

Conversation

shirleyquirk
Copy link
Contributor

there is no ctype that maps to uint8 in system.nim, instead both cchar and cuchar map to char, this is an annoying asymmetry
https://forum.nim-lang.org/t/7790 and means there's no 'byte'-like ctype that doesn't also map to string functions.

I created this PR to see what it would break, and it actually highlighted a bug in net that wouldn't have been possible to make had cuchar been defined otherwise.

SSL has these two functions for setting callbacks to send identity and psk

void SSL_CTX_set_psk_server_callback(SSL_CTX *ctx,
       unsigned int (*callback)(SSL *ssl, const char *identity,
       unsigned char *psk, int max_psk_len));
void SSL_set_psk_server_callback(SSL *ssl,
       unsigned int (*callback)(SSL *ssl, const char *identity,
       unsigned char *psk, int max_psk_len));

note the distinction between const char* identity and unsigned char* psk, identity is a null-terminated string, while psk is a byte buffer. attempting to use string routines on psk in C would have failed with a type error, while in Nim since ptr cuchar aliases ptr char aliases cstring, they don't. In this situation, C has stricter typing than Nim!

and that's what happened, here psk.len was called by mistake instead of pskString, which should have failed.

This is obviously a breaking change for someone, so feel free to just accept the net bugfix and delete the change to system if unwanted, but the fix is a powerful argument for making the break so not splitting them apart yet

@Araq
Copy link
Member

Araq commented Apr 15, 2021

Gah this is nasty ... You could have found this a couple of hours earlier before our new bugfix releases... :-/

@shirleyquirk
Copy link
Contributor Author

heh. sorry should wake up earlier

lib/pure/net.nim Outdated Show resolved Hide resolved
changelog.md Show resolved Hide resolved
changelog.md Outdated Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented Apr 16, 2021

Sorry, we need a separate PR with just the bugfix so that it can be backported easily.

@shirleyquirk
Copy link
Contributor Author

looking a bit deeper, I dont think it's necessary to backport

hear me out:

because of #11280, which exists in 1.0.10 and 1.2.12 (in fact since 0.16)
pskCallBackServerFun and pskCallBackClientFun both cause a SIGSEGV when they try to access the identity/psk function
before they would trigger the buffer overrun

the fundamental issue is that the callback can only access the raw ssl context, not the wrapper, so, even if a pointer to the helper functions is stored in ssl_ex_data, the callback can't know at what index it's stored. assuming it's stored at zero doesn't seem to work either: #4406

making (Server|Client)GetPskFunction= work properly would require changing a lot of code, as @dom96 said: it's not trivial.
perhaps a macro that injects user code directly into the callback function? i'll have a play

I've split off the uint8 stuff from this PR anyway, just in case someone uses the sourcecode as a template, but this aint over

@shirleyquirk shirleyquirk changed the title Make cuchar map to uint8, also fix buffer-overrun bug in net Fix buffer-overrun bug in net Apr 16, 2021
@Araq Araq merged commit fdd4391 into nim-lang:devel Apr 16, 2021
@timotheecour
Copy link
Member

@shirleyquirk the root cause is #13790 (see also WIP #13792)

narimiran pushed a commit that referenced this pull request Apr 21, 2021
narimiran pushed a commit that referenced this pull request May 17, 2021
@timotheecour timotheecour mentioned this pull request Jul 10, 2021
narimiran pushed a commit that referenced this pull request Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants