Upstream: disallow empty path in proxy_store and friends. #344
Conversation
Renaming a temporary file to an empty path ("") returns NGX_ENOPATH
with a subsequent ngx_create_full_path() to create the full path.
This function skips initial bytes as part of path separator lookup,
which causes out of bounds access on short strings.
The fix is to avoid renaming a temporary file to an obviously invalid
path, as well as explicitly forbid such syntax for literal values.
Although Coverity reports about potential type underflow, it is not
actually possible because the terminating '\0' is always included.
Notably, the run-time check is sufficient enough for Win32 as well.
Other short invalid values result either in NGX_ENOENT or NGX_EEXIST
and "MoveFile() .. failed" critical log messages, which involves a
separate error handling.
Prodded by Coverity (CID 1605485).
There was a problem hiding this comment.
Notably, the run-time check is sufficient enough for Win32 as well. Other short invalid values result either in NGX_ENOENT or NGX_EEXIST and "MoveFile() .. failed" critical log messages, which involves a separate error handling.
Which run-time check do you mean? Seems like ngx_create_full_path() does not have any protection against empty/short strings.
The newly added one, to mitigate ngx_ext_rename_file() call with invalid dst (empty) path: FWIW, regarding the ngx_create_full_path() function name: while it may seem that it works with absolute paths only, actually it supports relative paths as well, such as when It seems the only reason why ngx_create_full_path() skips initial char (or three on win32) is because otherwise it may end up trying to rename something to zero-length string due to temporal path separator replacement with '\0'. |
WRT why ngx_create_full_path() should not be fixed instead:
|
Well, it certainly doesn't work with relative paths on win32 (although it can make you think that it does). |
Renaming a temporary file to an empty path ("") returns NGX_ENOPATH with a subsequent ngx_create_full_path() to create the full path. This function skips initial bytes as part of path separator lookup, which causes out of bounds access on short strings.
The fix is to avoid renaming a temporary file to an obviously invalid path, as well as explicitly forbid such syntax for literal values.
Although Coverity reports about potential type underflow, it is not actually possible because the terminating '\0' is always included.
Notably, the run-time check is sufficient enough for Win32 as well. Other short invalid values result either in NGX_ENOENT or NGX_EEXIST and "MoveFile() .. failed" critical log messages, which involves a separate error handling.
Prodded by Coverity (CID 1605485).