OCaml 5.0 fix: eliminate out-of-heap pointer for client_verify_callback
#83
Conversation
|
cc @anmonteiro |
| @@ -561,7 +561,13 @@ CAMLprim value ocaml_ssl_digest(value vevp, value vcert) | |||
|
|
|||
| CAMLprim value ocaml_ssl_get_client_verify_callback_ptr(value unit) | |||
| { | |||
| #ifdef NO_NAKED_POINTERS | |||
There was a problem hiding this comment.
This will be the default in OCaml 5, right?
There was a problem hiding this comment.
There was a problem hiding this comment.
The code here before was a bit rubbish because it was allocating a new block every time it was called - I've changed to a single static value instead.
src/ssl_stubs.c
Outdated
| { | ||
| #ifdef NO_NAKED_POINTERS | ||
| vcallback = Field(vcallback, 0); | ||
| if (!Is_block(vcallback) || Wosize_val(vcallback) != 1) |
There was a problem hiding this comment.
It looks like this section of code previously ignored "failures" if !Is_block(vcallback).
I believe we're introducing a new failure mode in this change. I'm OK with it but it might need to be documented in the interface file. What do you think?
There was a problem hiding this comment.
vcallback has type verify_callback option. Previously, verify_callback was a naked pointer, so there was no failure mode. This guard is just here in case there really is some C code out there which has a custom verify function which it casts to a naked pointer rather than boxing. If that happens, the program is going to crash - all this guard does is try to display a better error if the supplied doesn't look like the correct kind of block (I guess it could also test the tag).
I think it would be more important to document how to create verify callbacks in the first place before addressing this failure mode - the only way to write one at the moment is to read to the source 😀
There was a problem hiding this comment.
I just pushed a revised version with the extra test on the Tag_val - that means that only the four code pointers 0x4fb, 0x5fb, 0x6fb and 0x7fb would now slip through (which is handy, because those are highly unlikely function addresses)
There was a problem hiding this comment.
the only way to write one at the moment is to read to the source
very good point. This seems to be the case for a lot of the functions in ocaml-ssl.
There was a problem hiding this comment.
@dra27 to help me understand, where do those four code pointers come from?
OCaml 5.0 doesn't permit out-of-heap pointers.
dra27
force-pushed
the
remove-out-of-heap-pointer
branch
from
659c773 to
2dd96ea
Compare
|
Thank you! |
OCaml 5.0 doesn't permit out-of-heap pointers, as noted in #76.
A search in GitHub doesn't reveal an example of it, but contrary to the solution in #81, it is was possible to override the verification callback - the function must necessarily be written in C, and anything doing that would simply have had a similar primitive. The approach here continues to allow that - if there is any C code out there which was doing this, then it would need updating to box the function pointer as well in "no naked pointers" mode.