Hi Steffan and thanks for the review,
On 06/07/18 04:22, Steffan Karger wrote:
>> +struct buffer
>> +keyfile_to_buffer(const char *file, int max_size, struct gc_arena *gc)
>> +{
>> + size_t size;
>> + struct buffer in = alloc_buf_gc(max_size, gc);
>> + int fd = platform_open(file, O_RDONLY, 0);
>> + if (fd == -1)
>> + {
>> + msg(M_ERR, "Cannot open key file '%s'", file);
>> + }
>> +
>> + size = read(fd, in.data, in.capacity);
>> + if (size < 0)
>> + {
>> + msg(M_FATAL, "Read error on key file ('%s')", file);
>> + }
>> +
>> + if (size == in.capacity)
>> + {
>> + msg(M_FATAL, "Key file ('%s') can be a maximum of %d bytes", file,
>> + (int)in.capacity);
>> + }
>> + close(fd);
>> +
>> + in.len = size;
>> +
>> + return in;
>> +}
>
> Hm, I don't really like the FATAL errors in a function that looks
> generic enough for someone to use in non-startup situations. In that
> case the FATALs might create a DoS vector. Or, think ahead to the
> connection blocks, a connection starting, then timeing out, then exiting
> because the tls-auth/crypt key of the next connection block doesn't
> exist (instead of just trying the next block).
> You're right. I made an assumption about the context where it'll be used, but you're right, I shouldn't do it. I'll remove the FATAL. > While reading this, I realize I wrote a very similar function in the > "tls-crypt-v2: generate client keys" patch. That one uses > platform_stat() to figure out the size and get rid of the maxlen > argument, and doesn't throw fatal errors. Feel free to use that > implementation if you like it, or not if you don't (and I'll use yours > in the latter case). Oh Right, thanks for pointing this out. I'll use your implementation as it is relying more on buffer APIs, while I am dealing directly with the internals (and I prefer your approach). I'll send v4 of this patch soon. Thanks! -- Antonio Quartulli
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
