Secure MHTML generation against TOCTOU [chromium/src : main]
Min Qin (Gerrit)
Min Qin added 1 comment![]( "Open in Gerrit")
PTAL
+jamescook@ for page_capture
+dewittj@ for offline_pages
+boliu@ for content/browser
+avi@ for content/public
Related details
- Avi Drissman
- Bo Liu
- James Cook
- Justin DeWitt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bo Liu (Gerrit)
Bo Liu added 6 comments![]( "Open in Gerrit")
MHTML generation previously used base::CreateTemporaryFile() which
immediately closes the created file descriptor, then open the created
file path separately. This allows attackers to tamper the file duringumm, has anyone evaluated the security concern here? two separate thoughts..
if this is a valid security concern, then base::CreateTemporaryFile is simply unsafe?
a safer version would be to just return the fd, but it's still a named file on the file system, so attacker could still open the file using the file path and do the same attack?
base::BindOnce([](base::File file) {}, std::move(mhtml_file_scoped_)));there's base::DoNothingWithBoundArgs
ReportFailure("Unable to create temporary file");close immediately on failure
FILE* stream = base::FileToFILE(std::move(mhtml_file_scoped_), "rb");use base::ScopedFILE
std::ignore = DenyExecuteAccessToMHTMLFile(file_path);CreateMHTMLFile has a long comment explaining this, this just looks random
base::File output_handle;sounds like these two should be an std::variant?
Related details
- Avi Drissman
- James Cook
- Justin DeWitt
- Min Qin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman added 1 comment![]( "Open in Gerrit")
MHTML generation previously used base::CreateTemporaryFile() which
immediately closes the created file descriptor, then open the created
file path separately. This allows attackers to tamper the file duringumm, has anyone evaluated the security concern here? two separate thoughts..
if this is a valid security concern, then base::CreateTemporaryFile is simply unsafe?
a safer version would be to just return the fd, but it's still a named file on the file system, so attacker could still open the file using the file path and do the same attack?
Ditto; this is what I immediately thought. We shouldn’t allow unsafe primitives.
Related details
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
MHTML generation previously used base::CreateTemporaryFile() which
immediately closes the created file descriptor, then open the created
file path separately. This allows attackers to tamper the file duringAvi Drissmanumm, has anyone evaluated the security concern here? two separate thoughts..
if this is a valid security concern, then base::CreateTemporaryFile is simply unsafe?
a safer version would be to just return the fd, but it's still a named file on the file system, so attacker could still open the file using the file path and do the same attack?
Ditto; this is what I immediately thought. We shouldn’t allow unsafe primitives.
note my take is that this isn't really a security concern; correctness or just general defensive programming, sure
if attacker can already write to disk, there isn't anything we can do to protect against tampering here
James Cook (Gerrit)
James Cook added 1 comment![]( "Open in Gerrit")
page_capture seems OK, but I'll take a closer look after you've addressed Bo's comments. Just add me back to the attention set when you're ready.
Related details
- Justin DeWitt
- Min Qin
Justin DeWitt (Gerrit)
Justin DeWitt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin DeWitt
| Code-Review | +1 |
1 comment
- Min Qin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Min Qin (Gerrit)
Min Qin added 6 comments![]( "Open in Gerrit")
MHTML generation previously used base::CreateTemporaryFile() which
immediately closes the created file descriptor, then open the created
file path separately. This allows attackers to tamper the file duringAvi Drissmanumm, has anyone evaluated the security concern here? two separate thoughts..
if this is a valid security concern, then base::CreateTemporaryFile is simply unsafe?
a safer version would be to just return the fd, but it's still a named file on the file system, so attacker could still open the file using the file path and do the same attack?
Bo LiuDitto; this is what I immediately thought. We shouldn’t allow unsafe primitives.
note my take is that this isn't really a security concern; correctness or just general defensive programming, sure
if attacker can already write to disk, there isn't anything we can do to protect against tampering here
I think the security concern is only for MacOS. On macOS, both the browser process and the sandboxed child processes obtain the temporary directory via NSTemporaryDirectory(), so the compromised gpu process can delete the temp file and replace it with a simlink. See POC in the attached bug.
base::BindOnce([](base::File file) {}, std::move(mhtml_file_scoped_)));Min Qinthere's base::DoNothingWithBoundArgs
Done
ReportFailure("Unable to create temporary file");Min Qinclose immediately on failure
If GetTempDir fails, the file is never created.
If CreateAndOpenTemporaryFileInDir fails, it returns an invalid base::File object.
So calling close() will be an no-op here
FILE* stream = base::FileToFILE(std::move(mhtml_file_scoped_), "rb");Min Qinuse base::ScopedFILE
Done
CreateMHTMLFile has a long comment explaining this, this just looks random
added comments
sounds like these two should be an std::variant?
We actually always need both. On Windows, before passing the file handle to the renderer, we must deny execute access by calling DenyExecuteAccessToMHTMLFile(file_path) (which uses path-based ACL APIs since base/win/security_util.h doesn't have a handle-based equivalent). We also use the path for tracing in initializeJob.
Related details
- Avi Drissman
- Bo Liu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
std::ignore = DenyExecuteAccessToMHTMLFile(file_path);I mean can you refactor here to not duplicate any code
Related details
- Avi Drissman
- Min Qin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Min Qin (Gerrit)
Min Qin added 1 comment![]( "Open in Gerrit")
MHTML generation previously used base::CreateTemporaryFile() which
immediately closes the created file descriptor, then open the created
file path separately. This allows attackers to tamper the file duringAvi Drissmanumm, has anyone evaluated the security concern here? two separate thoughts..
if this is a valid security concern, then base::CreateTemporaryFile is simply unsafe?
a safer version would be to just return the fd, but it's still a named file on the file system, so attacker could still open the file using the file path and do the same attack?
Bo LiuDitto; this is what I immediately thought. We shouldn’t allow unsafe primitives.
Min Qinnote my take is that this isn't really a security concern; correctness or just general defensive programming, sure
if attacker can already write to disk, there isn't anything we can do to protect against tampering here
I think the security concern is only for MacOS. On macOS, both the browser process and the sandboxed child processes obtain the temporary directory via NSTemporaryDirectory(), so the compromised gpu process can delete the temp file and replace it with a simlink. See POC in the attached bug.
This change introduces probably another issue. Previously chrome creates a temp file through makestemp(), this create a file with read | write flag. Then in CreateMHTMLFile(), Chrome reopens that temp file with only append flag to disallow OOPIF reads.
Now if we create the temp file without opening it again, the temp file will carry the read | write flag, make OOPIF read possible
So we need another way to create the temp file with only append mode, probably something similar to what windows do: https://source.chromium.org/chromium/chromium/src/+/main:base/files/file_util_win.cc;l=782
WDYT?
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
MHTML generation previously used base::CreateTemporaryFile() which
immediately closes the created file descriptor, then open the created
file path separately. This allows attackers to tamper the file duringAvi Drissmanumm, has anyone evaluated the security concern here? two separate thoughts..
if this is a valid security concern, then base::CreateTemporaryFile is simply unsafe?
a safer version would be to just return the fd, but it's still a named file on the file system, so attacker could still open the file using the file path and do the same attack?
Bo LiuDitto; this is what I immediately thought. We shouldn’t allow unsafe primitives.
Min Qinnote my take is that this isn't really a security concern; correctness or just general defensive programming, sure
if attacker can already write to disk, there isn't anything we can do to protect against tampering here
Min QinI think the security concern is only for MacOS. On macOS, both the browser process and the sandboxed child processes obtain the temporary directory via NSTemporaryDirectory(), so the compromised gpu process can delete the temp file and replace it with a simlink. See POC in the attached bug.
This change introduces probably another issue. Previously chrome creates a temp file through makestemp(), this create a file with read | write flag. Then in CreateMHTMLFile(), Chrome reopens that temp file with only append flag to disallow OOPIF reads.
Now if we create the temp file without opening it again, the temp file will carry the read | write flag, make OOPIF read possible
So we need another way to create the temp file with only append mode, probably something similar to what windows do: https://source.chromium.org/chromium/chromium/src/+/main:base/files/file_util_win.cc;l=782
WDYT?
just open with O_NOFOLLOW as the bot suggested? sounds like a fine fix to me
Min Qin (Gerrit)
Min Qin added 1 comment![]( "Open in Gerrit")
MHTML generation previously used base::CreateTemporaryFile() which
immediately closes the created file descriptor, then open the created
file path separately. This allows attackers to tamper the file duringAvi Drissmanumm, has anyone evaluated the security concern here? two separate thoughts..
if this is a valid security concern, then base::CreateTemporaryFile is simply unsafe?
a safer version would be to just return the fd, but it's still a named file on the file system, so attacker could still open the file using the file path and do the same attack?
Bo LiuDitto; this is what I immediately thought. We shouldn’t allow unsafe primitives.
Min Qinnote my take is that this isn't really a security concern; correctness or just general defensive programming, sure
if attacker can already write to disk, there isn't anything we can do to protect against tampering here
Min QinI think the security concern is only for MacOS. On macOS, both the browser process and the sandboxed child processes obtain the temporary directory via NSTemporaryDirectory(), so the compromised gpu process can delete the temp file and replace it with a simlink. See POC in the attached bug.
This change introduces probably another issue. Previously chrome creates a temp file through makestemp(), this create a file with read | write flag. Then in CreateMHTMLFile(), Chrome reopens that temp file with only append flag to disallow OOPIF reads.
Now if we create the temp file without opening it again, the temp file will carry the read | write flag, make OOPIF read possible
So we need another way to create the temp file with only append mode, probably something similar to what windows do: https://source.chromium.org/chromium/chromium/src/+/main:base/files/file_util_win.cc;l=782
WDYT?
A quick patch will be to add O_NOFOLLOW in CreateMHTMLFile(), so that when reopening the temp file Chrome will not follow a simlink to a sensitive file.
Related details
- Avi Drissman
- Bo Liu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Min Qin (Gerrit)
Min Qin abandoned this change.
Min Qin abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |