net/http: multipart form should not include directory path in filename

[katiehockman]

When parsing a multipart form, the parsed filename could include directory path information (e.g. "../../foobar.txt). This is not allowed by RFC 7578 Section 4.2, which states:

If a "filename" parameter is supplied, the requirements of Section 2.3 of [RFC2183] for the "receiving MUA" (i.e., the receiving Mail User Agent) apply to receivers of multipart/form-data as well: do not use the file name blindly, check and possibly change to match local file system conventions if applicable, and do not use directory path information that may be present.

This off-spec behavior makes the code easy to misuse, but does not explicitly introduce a vulnerability, so this will not be fixed in a security release.

Thanks to Sebastiaan van Stijn for reporting this issue.

[gopherbot]

Change https://golang.org/cl/313809 mentions this issue: net/http: strip directory path when parsing multipart forms

[Stebalien]

The spec states that implementations SHOULD (not MUST) strip this information (and further clean the path) as necessary. This a cautionary note against servers and mail user agents saving attachments at attacker specified locations, not a spec requirement.

This breaks go-ipfs in production as we use this information to to upload entire directory trees.

This is also insufficient for security purposes as it won't check for special filenames on windows (NUL, etc.).

[FiloSottile]

The spec might not require any specific cleanup but it does allow it. I believe this was the right tradeoff to provide a safe-by-default API for most applications. If you need the raw value, it's available through Header["Content-Disposition"].

This is also insufficient for security purposes as it won't check for special filenames on windows (NUL, etc.).

Can you elaborate? Opening a Win32 reserved filename should fail, not let the attacker cause anything malicious.

[Stebalien]

To be clear, I'm not arguing that this change should be reverted, just flagging that this is a significant change where both behaviors were reasonable.

If you need the raw value, it's available through Header["Content-Disposition"].

Already done.

Can you elaborate? Opening a Win32 reserved filename should fail, not let the attacker cause anything malicious.

My understanding is that opening "nul" will succeed but anything written to it will be discarded. This is significantly less of an issue than directory traversal, but could still be abused (data written won't match data read).

[kentonv]

This also broke the Cloudflare Workers upload API, and was pretty hard to track down. Only users who had non-flat directory structures were affected, which unfortunately was something we didn't have test coverage for. It took quite a while to figure out what was going on and a while longer to figure out it was caused by a Go update.

We'll use Header["Content-Disposition"] going forward. Just wanted to provide the data point.