Param helper functions by paulidale · Pull Request #8400 · openssl/openssl

paulidale · 2019-03-05T04:11:43Z

Add helper routines to make dealing with UTF8 and octet strings easier.

documentation is added or updated
tests are added or updated

paulidale · 2019-03-05T04:14:49Z

This is built on top of #8379 but should re-base over #8377 in a reasonably straightforward manner.

It is very much a work in progress at this point:

The magic to allow allocated strings to be freed isn't.
There are no test cases.
There is no documentation.
The code hasn't been executed.

crypto/params.c

levitte · 2019-03-06T04:48:02Z

crypto/params.c

I think this is a bit over the top. Presumably, the calling code knows or can easily check the data type, and deduce that if it passed a reference to NULL and the passed pointer is non-NULL after the call to this function, it needs to free what that pointer points to. This should not be hard to document.

Not quite. A UTF8_STRING_PTR will replace a NULL with a pointer that can't be freed.
Either an extra return argument is required to indicate free the pointer or the params code has to keep track of which ones need freeing.

But surely, the code that constructed the OSSL_PARAM to begin with knows if that OSSL_PARAM has the data type OSSL_PARAM_UTF8_STRING or OSSL_PARAM_UTF8_STRING_PTR? It's not like those data types find themselves there randomly by magic. That should be very easy to code. Do you want an example?

Okay, I'm fine delegating the free management to the caller :)
No need for an example.

Remember to document the requirements

While I'm not religiously opposed to leaving free management to the caller, it is something that should be pretty rare, as it's easy for the caller to mess it up. As much as possible, we should stick to simple API contracts, where (e.g.) "you should always free() this parameter after calling this function" lets the calling code be simpler.

You cannot allocate something in a DLL and free it in main because that won't work on Windows for example.

The allocations are not across provider boundaries. The provider calls get to extract information that has already passed the boundary. The provider is required to call free.

@kaduk is "you must free if you didn't provide the space" sufficiently simple?

It is tolerably simple, yes. May or may not be the best solution in any given case, though.

crypto/params.c

paulidale · 2019-03-06T07:17:49Z

Should the string get functions use the return_size field (if it exists) instead of the buffer_size?
I suspect so.

levitte · 2019-03-06T07:50:59Z

Should the string get functions use the return_size field (if it exists) instead of the buffer_size?
I suspect so.

I'm afraid I only confused matters with the return_size vs buffer_size discussion. It's a red herring.

See, there's really no reason whatsoever for a OSSL_PARAM owner to ever call the OSSL_PARAM_set_ and OSSL_PARAM_get_ functions. The OSSL_PARAM owner sets up an array with pointers to diverse variables it also owns, then calls whatever setter or responder that either uses the data from the passed OSSL_PARAM array, or fills in data into the variables that are pointed at (i.e. the OSSL_PARAM owner's variables). When the execution gets back to the OSSL_PARAM owner, the variables are filled in, just use them, with the one exception that any large number buffer will have to be imported into a bignum, for example using BN_native2bn.

For the setters and responders, i.e. those who use the data from a passed OSSL_PARAM array, or fills in the buffers it points at, the value of return_size is uninteresting, all they need to do is to fill it in (for the OSSL_PARAM_set_ functions), but they should only use buffer_size to figure out the size of the buffer. return_size is only interesting to the OSSL_PARAM owner.

Herein lies a lot of the confusion I'm seeing, that there are two sides to the use of `OSSL_PARAM:

there are those who construct an array of those, with pointers to variables they know about. I've made it a habit of calling these "OSSL_PARAM owners".
there are those who receive an OSSL_PARAM array and does something with it (use the data or fill in data)

These are at opposite ends and need different functionality, so in designing your API (and that goes for @richsalz as well), it's really important to understand and specify which side is expected to use what function.

crypto/params.c

paulidale · 2019-03-06T08:24:38Z

What worries me is that if we're getting confused, others without the background won't have a change. I.e. something is wrong with the API.

crypto/params.c

levitte · 2019-03-06T09:23:30Z

doc/man3/OSSL_PARAM.pod

I think I sometimes forget this... would you mind telling me when I do?

I'll try to remember :)

crypto/params.c

include/openssl/params.h

levitte

Naming nit

include/openssl/params.h

paulidale · 2019-03-07T01:23:15Z

These are at opposite ends and need different functionality, so in designing your API, it's really important to understand and specify which side is expected to use what function.

I'm not entirely convinced:

For basic types, I agree there is no advantage using wrappers.
For BIGNUMs I don't agree. Using the get/set on both sides makes sense.
For utf8 and octets it's a toss up either way. People know how to use memcpy(3) but having everything nicely encapsulated might help avoid bugs.

levitte · 2019-03-07T06:18:42Z

These are at opposite ends and need different functionality, so in designing your API, it's really important to understand and specify which side is expected to use what function.

I'm not entirely convinced:

For basic types, I agree there is no advantage using wrappers.

For BIGNUMs I don't agree. Using the get/set on both sides makes sense.

Why? If the OSSL_PARAM owner has requested parameters, then it knows exactly in what buffer the result and its size ended up, just call BN_native2bn, done !
What you're suggesting is a good recipe for confusion, especially regarding the roles of buffer_size and *return_size and who's meant to consume each of them.

For utf8 and octets it's a toss up either way. People know how to use memcpy(3) but having everything nicely encapsulated might help avoid bugs.

Why should the OSSL_PARAM owner memcpy? Why not just point directly at the variable where the string is supposed to end up (or at the original string for the case of sending parameters)?

test/params_api_test.c

paulidale · 2019-03-07T21:21:16Z

Why? If the OSSL_PARAM owner has requested parameters, then it knows exactly in what buffer the result and its size ended up, just call BN_native2bn, done !

With a const OSSL_PARAM, how large should the BIGNUM buffer be for an RSA signature?
It depends on the RSA key size. If the BIGNUM is smaller than buffer_size, there will be garbage added to it by the OSSL_PARAM_get_BN call. Are we going to enforce zero padding on all BIGNUM exchanges?

What you're suggesting is a good recipe for confusion, especially regarding the roles of buffer_size and *return_size and who's meant to consume each of them.

I'd prefer to call them total_size and used_size which removes the confusion and allows them both to be transferred both ways, although total_size will be const.

Why should the OSSL_PARAM owner memcpy? Why not just point directly at the variable where the string is supposed to end up (or at the original string for the case of sending parameters)?

This works unless the OSSL_PARAM is const and the variable/string is allocated.

crypto/params.c

test/params_api_test.c

levitte · 2019-03-08T00:12:17Z

I'm noticing a happy CI 😉

Provide a number of functions to allow parameters to be set and retrieved in a type safe manner. Functions are provided for many integral types plus double, BIGNUM, UTF8 strings and OCTET strings. All of the integer functions will widen the parameter data as required. This permits a degree of malleability in the parameter definition. For example a type can be changed from a thirty two bit integer to a sixty four bit one without changing application code. Only four and eight byte integral sizes are supported here. A pair of real functions, a get and a set, that will accept all real and integer types and automaticly convert to/from double as required. There could be some loss of precision when converting wide integral types to reals and reals to any integer type. A pair of functions is available for BIGNUMs. These accept any sized unsigned integer input and convert to/from a BIGNUM. For each OCTET and UTF8 strings, four functions are defined. This provide get and set functionality for string and for pointers to strings. The latter avoiding copies but have other inherent risks. Finally, some utility macros and functions are defined to allow OSSL_PARAM definition arrays to be more simplify specified. There are two macro and one function for most types. The exception being BIGNUM, for which there is one macro and one function.

crypto/params.c

doc/man3/OSSL_PARAM.pod

doc/man3/OSSL_PARAM_get_int32_t.pod

mattcaswell · 2019-03-08T12:50:22Z

doc/man3/OSSL_PARAM_get_int32_t.pod

+The size used by the parameter value, in bytes, is written to B<return_size>.
+
+OSSL_PARAM_END provides an end of parameter list marker.
+This should terminate all OSSL_PARAM arrays.


Some examples of the usage of the above macros would be helpful

I reused the examples from OSSL_PARAM.pod converting them to the macros and functions defined here.

crypto/params.c

test/params_api_test.c

richsalz · 2019-03-08T17:13:24Z

I am still working on my alterantive proposal, the day job has kept me busy. I hope this can wait until after I'm "finished"

demacro the get/set/construct for the standard integral types

paulidale · 2019-03-09T02:24:21Z

I think that's all the comments addressed.

I'm out of action for several weeks starting Monday.

levitte · 2019-03-12T19:16:13Z

This was finalized in #8451, so closing

paulidale force-pushed the param-string branch from 9bcb593 to c897bb8 Compare March 6, 2019 01:42