On 09/11/17 21:46, Steffan Karger wrote:
> Hi,
>
> On 09-11-17 04:25, Antonio Quartulli wrote:
>>> @@ -812,6 +816,12 @@ gen_path(const char *directory, const char *filename,
>>> struct gc_arena *gc)
>>> #else
>>> const int CC_PATH_RESERVED = CC_SLASH;
>>> #endif
>>> +
>>> + if (!gc)
>>> + {
>>> + return NULL; /* Would leak memory otherwise */
>>
>> As far as I understood the ratio behind this change, we are basically
>> saying that passing gc == NULL is a real programming error and not a
>> runtime problem. If so, wouldn't it be better to assert(gc) directly?
>> I am saying so because we should make this failure louder, so that it
>> can be recognized in an early test.
>>
>> Or can we have cases when this function is called with NULL because of a
>> temporary failure (my understanding is that this should not be possible)?
>
> I'm a bit in doubt here - if this parameter is NULL, that's a
> programming error. But we've seen before that programming errors
> sometimes can be triggered by external input. In this case, this
> function can be called when a client connects. It *should* never hit
> this case, but if it somehow does, that becomes a DoS vuln. So I think
> I prefer 'graceful error handling' in paths that are exposed like this,
> unless the error indicates a system integrity error such as an
> out-of-bounds read or write.Yeah, makes sense. I was also worried about the DoS, that's why I was trying to convince myself that passing NULL was not possible at all. But I like your defensive approach more. Acked-by: Antonio Quartulli <a...@unstable.cc> -- 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
