-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix buffer-overrun bug in net #17728
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
Conversation
|
Gah this is nasty ... You could have found this a couple of hours earlier before our new bugfix releases... :-/ |
|
heh. sorry should wake up earlier |
172d4e2 to
8947b37
Compare
|
Sorry, we need a separate PR with just the bugfix so that it can be backported easily. |
|
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) 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. 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 the root cause is #13790 (see also WIP #13792) |
(cherry picked from commit fdd4391)
(cherry picked from commit fdd4391)
(cherry picked from commit fdd4391)
there is no ctype that maps to uint8 in system.nim, instead both
ccharandcucharmap to char, this is an annoying asymmetryhttps://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
netthat wouldn't have been possible to make had cuchar been defined otherwise.SSL has these two functions for setting callbacks to send
identityandpsknote the distinction between
const char* identityandunsigned char* psk,identityis a null-terminated string, while psk is a byte buffer. attempting to use string routines onpskin C would have failed with a type error, while in Nim sinceptr cucharaliasesptr charaliasescstring, they don't. In this situation, C has stricter typing than Nim!and that's what happened, here
psk.lenwas called by mistake instead ofpskString, 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