Open AdamKorcz opened 4 months ago
Thank you for sharing this case and for fuzzing new cases in the codebase, it's great to verify these API surfaces.
As you note, the specific function call contract for uvwasi_normalize_path
is that normalized_len
is strlen
of normalized_path
which contains an additional C-string zero byte at the length index (and therefore the underlying string allocation is len + 1
). You also confirm this invariant is guaranteed in all call sites.
Were you able to tell if there is any potential for this case given the current call site usage?
Then further, can you clarify the alternative refactoring you were suggesting here? Is the suggestion to instead have the len be a definition of the length that includes the zero byte?
@AdamKorcz any chance you can answer:
Then further, can you clarify the alternative refactoring you were suggesting here? Is the suggestion to instead have the len be a definition of the length that includes the zero byte?
Apologies for the delay.
Is the suggestion to instead have the len be a definition of the length that includes the zero byte?
Yes.
As I see it there are probably these options:
1) Ensure that the logic does not read beyond the provided length from a pure byte perspective, i.e. the length should include any zero bytes and be a raw indicator of the size of the buffer.
2) Adjust the test so it won't fail if e.g. a very long path is used as data (this is more of an indirect way) to show potential developers against the code that this is the expected input.
3) Make it clear through some documentation that the length excludes a zero-byte and thus the length argument should be one less than the size of the provided buffer. In essence just copying what @guybedford highlighted above "the specific function call contract for uvwasi_normalize_path is that normalized_len is strlen of normalized_path which contains an additional C-string zero byte at the length index (and therefore the underlying string allocation is len + 1)"
-- I don't think this is mentioned anywhere as such?
Were you able to tell if there is any potential for this case given the current call site usage?
Am not sure if you mean if we found any potential uses of the code that could misuse it? If so, then no. The only place could perhaps be the test where the size is not indicated explicitly to be +1
, which is probably because the paths are not large enough. Perhaps for this reason option (3) mentioned above is the most appropriate if you think it's reasonable to let devs know about the function's contract.
This is a disclosure for an issue in node.js's use of UVWASI that we have found during a security audit of node.js. The issue is a heap write-based buffer overflow in
src/path_resolver.c:uvwasi__normalize_path
. The issue was found by the following fuzzer:The fuzzer finds an issue with the following ASAN report:
The issue occurs on line 141 in
path_resolver.c
: https://github.com/nodejs/uvwasi/blob/2d0c0d019009e0bf85ee0e519c64f1109025f459/src/path_resolver.c#L139-L141The problem is that
ptr += cur_len
may setptr
to point atnormalized_path + normalized_len
which causes an off-by-one issue when thenormalized_len
corresponds to the size of the normalized_path buffer. The proposed fix is to check thatptr
has not increased beyond the bounds ofnormalized_path
:It's important to highlight in this case that the function used within uvwasi always allocates an extra byte such as here and here. However, we consider it counter-intuitive that uvwasi__normalize_path reads beyond the specified length, this is made more counter-intuitive considering the tests of this function provides the size of the buffer and not 1 less than the size of the normalized buffer here.
Considering that the API is used correctly in the places it's called, we consider the best option to either change the tests to ensure proper sizing is enabled, or, better yet to ensure that the funtion does not read beyond the speified length and adjust the callers of the function accordingly.
The fuzzer we used for this is: