Go: Add and Modify Sanitizers For TaintedPath #11703
Conversation
| cleanCall.asInstruction() = node and | ||
| frn.getField().hasQualifiedName("mime/multipart", "FileHeader", "Filename") and | ||
| node.getASuccessor*() = frn.asInstruction() |
There was a problem hiding this comment.
Use DataFlow::localFlow to establish dataflow from the multipart parse to the access of the Filename field.
Also why are we sanitizing frn.getBase() (the Form that was read from), not frn itself (the Filename we read?)
There was a problem hiding this comment.
Fixed, also don't need flow here since according to documentation the field is only available after ParseMultipartForm is called.
// MultipartForm is the parsed multipart form, including file uploads.
// This field is only available after ParseMultipartForm is called.
// The HTTP client ignores MultipartForm and uses Body instead.
MultipartForm *[multipart](https://pkg.go.dev/mime/multipart).[Form](https://pkg.go.dev/mime/multipart#Form)
Thats fair, I found that most developers have used filepath.Base to correctly prevent path traversal that is why I included it. Since ParseMultipartForm uses filepath.Base on Filename, do you want to include it? It "cleans" the filename of the multipart form and since ".." is not a valid filename across platforms, it should normally error out on any file operations. |
|
@Kwstubbs I would have to appeal to the security lab for a definitive opinion, but my gut take is that |
|
@smowton Is the policy for the sanitizers that no true positive can be discarded in exchange for a better FP rate? If so I will remove it because I agree that while is possible that ".." can be possibly smuggled in if let's say the developer wants to create/remove a directory with a similar name to the file, I just find it highly unlikely. In general, I suggested filepath.Base and ParseMultipartForm because I found developers correctly used them as files, not as directories and because golang has seperate file/directory APIs I thought the lower FP rate would be worth missing a few TPs. |
Kwstubbs
force-pushed
the
go-taintedpath-additions
branch
2 times, most recently
from
a45343f to
d14497f
Compare
|
hi @smowton could you take a look at this again? Thank you |
|
@Kwstubbs Yes, please remove that sanitizer and we can merge the rest. |
Kwstubbs
force-pushed
the
go-taintedpath-additions
branch
from
ffd4e52 to
30fe416
Compare
|
@owen-mc good to go! |
| /**An call to ParseMultipartForm creates multipart.Form and cleans multipart.Form.FileHeader.Filename using path.Base() */ | ||
| class MultipartClean extends Sanitizer { | ||
| MultipartClean() { | ||
| exists(DataFlow::FieldReadNode frn | | ||
| frn.getField().hasQualifiedName("mime/multipart", "FileHeader", "Filename") and | ||
| this = frn | ||
| ) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Since we have decided that filepath.Base is not an adequate sanitizer, I don't think we should include this, as it uses filepath.Base (docs link).
There was a problem hiding this comment.
@owen-mc Yup I'm aware, however I would still like to advocate for this sanitizer because it is a filename, not just part of a path. If the .. is treated as a filename, it references a folder which is a failure in Golang. If any path is prepended, then the path also references the previous folder, which again fails for APIs that expect a file. It also makes no sense to add any path after the .. since its a file, not a directory. I have seen this particular FP many times and its never been exploitable.
There was a problem hiding this comment.
Oh wait, I am confused. Did you mean to sanitize reads of the field Filename in the struct FileHeader (docs)? Or did you mean calls to the method FileName() (docs), which is defined on *Part, i.e. parts of the multipart message. Your code does the first one, but I don't see why that should be sanitized, so I'm guessing maybe you meant to do the second one, because the docs say it has been run through filepath.Base(). I consulted with a colleague and he agreed it was reasonable for the second one to be a sanitizer, so if that's what you meant then please change the code to refer to the right thing and update the test.
There was a problem hiding this comment.
@owen-mc I added the Filename in FileHeader because it previously led me to a FP when testing it. The only way FileHeader to be instantiated is through readForm, which sanitizes Filename using FileName which uses Base. I understand if you don't wanna add it since the documentation doesn't guarantee anything, but I thought i would include it.
There was a problem hiding this comment.
Ah, that makes sense. I think we can merge this then.
|
I made a PR with a few follow-ups here: #16180 |
This PR adds some additional sanitizers to the TaintedPath Customization due to a high number of false positives I have found after doing some research.
I have added:
filepath.Base
strings.ReplaceAll
http.ParseMultipartForm
I have also removed path.Clean as it does not sufficiently clean paths for software that can run on Windows.
Please see: labstack/echo#1718 as an example.