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. -Steffan
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