rpc: Convert attribute value for portability #67
Conversation
ueno
force-pushed
the
wip/dueno/portable-attribute
branch
from
f71ef1b to
3d45b04
Compare
p11-kit/rpc-message.c
Outdated
| CK_ULONG ulong_value; | ||
|
|
||
| memcpy (&ulong_value, value, value_length); | ||
| if (ulong_value > 0xFFFFFFFFFFFFFFFF) |
There was a problem hiding this comment.
That inequality is only true if value length > 8. Is the intention there to detect calls where value_length > 8? If yes, why not have that explicitly, such as:
if (value_length > 8)
...
There was a problem hiding this comment.
Here the code converts the value to the wire format in two steps: first it converts void * into CK_ULONG and then CK_ULONG to uint64_t. So there should be separate checks for those. I have added them to all encoding functions.
p11-kit/rpc-message.c
Outdated
| unsigned char validity; | ||
| size_t total = 0; | ||
| static const p11_rpc_attribute_value_encoder encoders[] = { | ||
| p11_rpc_buffer_add_attribute_value_byte, |
There was a problem hiding this comment.
A suggestion to make that easier to read; there is a (C99) form that allows to make the mappings apparent:
static const p11_rpc_attribute_value_encoder encoders[] = {
[P11_RPC_ATTR_BYTE] = p11_rpc_buffer_add_attribute_value_byte,
[P11_RPC_ATTR_ULONG] = p11_rpc_buffer_add_attribute_value_ulong,
...
}
That form guarrantees that any unset values will be zero.
p11-kit/rpc-message.c
Outdated
|
|
||
| /* The attribute length and value */ | ||
| p11_rpc_buffer_add_uint32 (buffer, attr->ulValueLen); | ||
| encoder = encoders[map_attribute_type_to_value_type (attr->type)]; |
There was a problem hiding this comment.
That seems like error prone on future revisions (when new RPC types are added), with a jump to an invalid address confusing debuggers. Maybe separate into an assign part to allow error checking? e.g.
if (attr->type > P11_RPC_ATTR_MAX)
error();
mapval = map_attribute_type_to_value_type (attr->type);
encoder = encoders[mapval]
if (encoder == NULL)
error();
There was a problem hiding this comment.
I added a couple of asserts there.
p11-kit/rpc-message.c
Outdated
|
|
||
| /* Decode the attribute value */ | ||
| value_type = map_attribute_type_to_value_type (type); | ||
| decoder = decoders[value_type]; |
There was a problem hiding this comment.
Same comment as for the encoder for safety.
p11-kit/rpc-message.c
Outdated
| { | ||
| CK_BYTE byte_value; | ||
|
|
||
| memcpy (&byte_value, value, value_length); |
p11-kit/rpc-message.c
Outdated
| static bool | ||
| p11_rpc_buffer_get_attribute_value_byte (p11_buffer *buffer, | ||
| size_t *offset, | ||
| uint32_t expected, |
There was a problem hiding this comment.
Are both expected and value_length needed?
There was a problem hiding this comment.
Actually, the expected length is tracked in buffer, so I removed expected.
| } else { | ||
| attrs[i].pValue = NULL; | ||
| attrs[i].ulValueLen = -1; | ||
| } | ||
|
|
||
| msg->parsed = offset; | ||
| } | ||
|
|
||
| *result = attrs; |
There was a problem hiding this comment.
I think a test suite of the new changes would be quite needed. I attach a minimal one for the byte and ulong types.
test-suite.patch.gz
Something related, is that since we go through systems we would also need a fuzzer for the server and client to ensure there is no compromise possible; we can use AFL or even oss-fuzz project for that. An example fuzzer that can be used as base is at gnutls repo devel/fuzz/gnutls_server_fuzzer.cc.
There was a problem hiding this comment.
Thank you for the review. It's embarrassing that the patch didn't have any additional tests. I have merged the patch and added tests for other value types. I will give it a try with AFL.
ueno
force-pushed
the
wip/dueno/portable-attribute
branch
3 times, most recently
from
1f367bd to
87d4114
Compare
When using the RPC across multiple architectures, where data models are different, say LP64 vs ILP32, there can be unwanted truncation of attribute values. This patch converts the values into portable format for the known attributes. Co-authored-by: Nikos Mavrogiannopoulos <nmav@redhat.com>
ueno
force-pushed
the
wip/dueno/portable-attribute
branch
from
87d4114 to
e94524f
Compare
|
Merging this. The fuzzer based on AFL was added as #73 . |
When using the RPC across multiple architectures, where data models
are different, say LP64 vs ILP32, there can be unwanted truncation of
attribute values.
This patch converts the values into portable format for the known
attributes.