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)
| 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.
| 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.
| 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)
This type allows carrying of an additional name that is needed when using the results from an SRV lookup.
When using a server through an SRV record, the name used for the TLS certificate should match the service name, not the dns name where the SRV record was found. Therefore the name used for certificate validation needs to be updated when using the result of a DNS resolve that went through an SRV record.
This will allow the DNS lookup code to do service lookups only when doing name resolving for NTSKE servers.
This allows the actual address of an NTSKE server to be behind an SRV record, allowing SRV record based pools. Since such records also modify the name expected on the certificate, we require succesfull DNSSEC validation for these records.
This solves issues with certificates not validating for the new domain name.
This allows the SRV record to specify a custom port for an NTSKE server.
This makes code size a bit more manageable.
2 participants
No description provided.