Review #1
Review #1
Conversation
squell
left a comment
squell
left a comment
There was a problem hiding this comment.
Some comments:
-
In one place you introduced a memory safety bug, congratulations. :-)
-
In some other places there are some things I don't like that are also in the original; you can decide whether to leave as-is or apply the boy scout rule. I do thin that the checks "don't send more than record" can be more cleanly handled by doing it at the spot that processes the record, rather than "counting them" and then performing an ever-increasingly-complex error check at the end of the function. But that's me.
-
The struct-copy thing: if in other places of the code base the original also passes structs-by-value, you can leave it as is, but it has a bit of a whiff (the function itself might assume that changes in the context record are visible to the caller, but they won't be)
nts_ke_server.c
Outdated
| break; | ||
| } | ||
|
|
||
| fixed_key_records++; |
There was a problem hiding this comment.
What are you going to do if fixed_key_records is more than 1? Also theoretically you're given an attacker the ability to cause an integer overflow here.
| fixed_key_records++; | |
| fixed_key_records = true; |
If the original author doesn't import stdbool.h or uses true and false in his code base, it's also acceptable to use:
| fixed_key_records++; | |
| fixed_key_records = 1; |
If you're strongly wedded to using the ++ operator for once now that you're using a battle-tested programming language (😛), at least declare fixed_key_records to be unsigned.
There was a problem hiding this comment.
Same comments apply to the uses of ++ below; I can see that they're in the original code, but I just don't like this if it's a simple boolean.
nts_ke_server.c
Outdated
| compliant_128gcm = 1; | ||
| break; | ||
| case NKE_RECORD_KEEP_ALIVE: | ||
| keep_alive++; |
There was a problem hiding this comment.
I don't see a down below that a check is done on multiple keep-alive records.
nts_ke_server.c
Outdated
| if (supported_protocol_records > 0 || supported_algorithm_records > 0) | ||
| { | ||
| if (next_protocol_records > 0 || aead_algorithm_records > 0 || | ||
| supported_protocol_records > 1 || supported_algorithm_records > 1) | ||
| error = NKE_ERROR_BAD_REQUEST; | ||
| } | ||
| else | ||
| { | ||
| if (((next_protocol_records != 1 || next_protocol_values < 1) && | ||
| (supported_algorithm_records == 0 || supported_protocol_records == 0)) || | ||
| (next_protocol == NKE_NEXT_PROTOCOL_NTPV4 && | ||
| (aead_algorithm_records != 1 || aead_algorithm_values < 1)) || | ||
| fixed_key_records > 1) | ||
| error = NKE_ERROR_BAD_REQUEST; | ||
|
|
||
| if (fixed_key_records) | ||
| { | ||
| if (SIV_GetKeyLength(aead_algorithm) != context.c2s.length || | ||
| SIV_GetKeyLength(aead_algorithm) != context.s2c.length || | ||
| num_aead_algorithms > 1 || num_next_protocols > 1) | ||
| error = NKE_ERROR_BAD_REQUEST; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think I'd like to see supported_algorithm_records != 0 || supported_protocol_records != 0 stored in a variable is_nts_pool_query or something; that will make this more readable (it can also be used to write the (supported_algorithm_records == 0 || supported_protocol_records == 0). As it stands I would find this if-then-else chain too cognitively taxing compared what as originally here.
This makes error checking at the end of the function simpler, which is particularly wanted once pool features are added later.
This perpares for further separate functions for special responses when handling pool requests.
Add support for fixed key requests, which draft-venhoek-nts-pool requires to allow the pool to handle key exchange requests on behalf of the time source. In a fixed key request, the NTS keys are explicitly provided instead of being derived from the TLS connection.
In order to handle NTS key exchanges on behalf of the server, pools in the draft-venhoek-nts-pool framework need to know which protocols and algorithms a time source supports.
Pools can do multiple requests in very quick succession. Allowing them to keep the connection alive therefore can offer quite significant performance gains. This commit adds support for a keep-alive record that a pool can use to indicate it wants to keep the connection alive long term. It honors that request if the pool is properly authenticated using an authentication token.
This avoids an overenthousiastic pool consuming all resources of the key exchange server.
This makes the key exchange server only cooperate with pools the user has explicitly configured, making misuse of the server harder.
| line[sizeof (line) - 2] = '\0'; | ||
| while (fgets(line, sizeof (line), f)) { | ||
| if (line[sizeof (line) - 2] != '\n' && line[sizeof (line) - 2] != '\0') { |
There was a problem hiding this comment.
You may want to add a comment here that it's a hack that allows avoidingstrlen().
squell
left a comment
squell
left a comment
There was a problem hiding this comment.
LGTM (I didn't do a thorough re-checking of the logic, but globally I think this is an improvement for readability)
No description provided.