ugorji / go

idiomatic codec and rpc lib for msgpack, cbor, json, etc. msgpack.org[Go]
MIT License
1.83k stars 294 forks source link

Vulnerability Issue - CWE 74 External Control of File Name or Path #382

Closed youngstone89 closed 1 year ago

youngstone89 commented 1 year ago

2 vulnerability issues have been found in the module. Veracode Security Scanner reports it as medium level flaw. Please, fix the issues.

CWE-73: External Control of File Name or Path: vendor/github.com/ugorji/go/codec/test.py:77 60Details: This call to open() contains a path manipulation flaw. The argument to the function is a filename constructed using user-supplied input. If an attacker is allowed to specify all or part of the filename, it may be possible to gain unauthorized access to files on the server, including those outside the webroot, that would be normally be inaccessible to end users. The level of exposure depends on the effectiveness of input validation routines, if any. Validate all user-supplied input to ensure that it conforms to the expected format, using centralized data validation routines when possible. When using blocklists, be sure that the sanitizing routine performs a sufficient number of iterations to remove all instances of disallowed characters. References: CWE OWASP 61CWE-73: External Control of File Name or Path: vendor/github.com/ugorji/go/codec/test.py:80 62Details: This call to open() contains a path manipulation flaw. The argument to the function is a filename constructed using user-supplied input. If an attacker is allowed to specify all or part of the filename, it may be possible to gain unauthorized access to files on the server, including those outside the webroot, that would be normally be inaccessible to end users. The level of exposure depends on the effectiveness of input validation routines, if any. Validate all user-supplied input to ensure that it conforms to the expected format, using centralized data validation routines when possible. When using blocklists, be sure that the sanitizing routine performs a sufficient number of iterations to remove all instances of disallowed characters. References: CWE OWASP 63======================== 64FAILURE: Found 2 issues!

ugorji commented 1 year ago

test.py is a python script used only by the codec_test.go

codec_test.go creates a temporary directory, passes it to test.py to build some msgpack and cbor streams using the python libs, and decodes those streams, does some testing, and completely removes that temporary directory.

The alternative to make it safer would be for test.py to create that directory, but then the go code would have to still expunge it later. It seemed more straightforward that the creater of the directory expunges it when done.

Consequently, this works as designed. This is not a flaw. The machine running the scan doesn't have the full context to understand.

felipeagger commented 1 year ago

Even so, I still believe that it doesn't make sense to have test files in a release package, since it will only increase the size unnecessarily, let alone a file that is not even the same extension as the standard language used. Could it be that if there was a /tests directory where these files were, and not included in the final library, wouldn't it be more interesting?

felipeagger commented 1 year ago

@ugorji