remoting multi-process Linux: Fix crash reporting [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Auto-Submit | +1 |
1 comment
Related details
- Joe Downing
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang added 1 comment![]( "Open in Gerrit")
return base::FilePath("/tmp/chromoting.crashpad." + username);AI makes the following suggestion, which makes sense to me. I'll think more about this:
---
Using a predictable directory name in a world-writable location like `/tmp` is a security vulnerability (CWE-377). Any local user can pre-create this directory or use a symlink to perform a denial-of-service, read other users' crash dumps (which may contain sensitive memory), or potentially gain privilege escalation.
For regular user processes, they have a home directory, so consider using a path within it (e.g. `~/.config/chrome-remote-desktop/crashpad`). For system/network daemon processes, consider using a secure system directory like `/var/log/chromoting/` or `/var/lib/chrome-remote-desktop/` with appropriate restrictive permissions.
Related details
- Joe Downing
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. |
Yuwei Huang (Gerrit)
Yuwei Huang removed Joe Downing from this change![]( "Open in Gerrit")
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. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Auto-Submit | +1 |
PTAL thanks!
return base::FilePath("/tmp/chromoting.crashpad." + username);AI makes the following suggestion, which makes sense to me. I'll think more about this:
---
Using a predictable directory name in a world-writable location like `/tmp` is a security vulnerability (CWE-377). Any local user can pre-create this directory or use a symlink to perform a denial-of-service, read other users' crash dumps (which may contain sensitive memory), or potentially gain privilege escalation.
For regular user processes, they have a home directory, so consider using a path within it (e.g. `~/.config/chrome-remote-desktop/crashpad`). For system/network daemon processes, consider using a secure system directory like `/var/log/chromoting/` or `/var/lib/chrome-remote-desktop/` with appropriate restrictive permissions.
Done
Related details
- Lambros Lambrou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing added 4 comments![]( "Open in Gerrit")
PLOG(ERROR) << "Failed to chown " << path << " to "
<< GetNetworkProcessUsername();
return;
}
// Make sure the directory is only accessible by the network process.
if (HANDLE_EINTR(chmod(path.value().c_str(), 0700)) != 0) {
PLOG(ERROR) << "Failed to chmod " << path;Should we delete the directory if we fail to secure it?
// Use /tmp as a fallback. This is susceptible to CWE-377.
return base::FilePath("/tmp/crd_crashpad_" + username);Maybe it's best to not log crashes at all in this case, WDYT?
base::TrimWhitespaceASCII(consent_content, base::TRIM_ALL) == "true";I'm fine landing this as-is for testing and such but I'm wondering if this should be a JSON file instead.
One of the interesting differences between Crashpad and Breakpad is that you have more control over how and when crash reports are sent for Crashpad. I'm not sure whether we'll need to extend the file to use it in this way or not but starting with a JSON format will allow us to extend it in the future if we choose to.
bool SetUsageStatsConsent(bool allowed) {
NOTIMPLEMENTED();Are you going to implement this function in a follow-up?
Related details
- Lambros Lambrou
- Yuwei Huang
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. |
Joe Downing (Gerrit)
Joe Downing added 1 comment![]( "Open in Gerrit")
Oh sorry, I saw this and assumed I was the requested reviewer since I worked with Gary on the original Crashpad impl : )
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Auto-Submit | +1 |
1 comment
Oh sorry, I saw this and assumed I was the requested reviewer since I worked with Gary on the original Crashpad impl : )
No worry! I did originally added you as the reviewer, but then I had to do a lot of Linux specific changes in this CL so I made Lambros the reviewer. I've added you as the reviewer now :)
Related details
- Joe Downing
- Lambros Lambrou
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. |
Yuwei Huang (Gerrit)
Yuwei Huang voted Auto-Submit+0![]( "Open in Gerrit")
| Auto-Submit | +0 |
Lambros Lambrou (Gerrit)
Lambros Lambrou added 2 comments![]( "Open in Gerrit")
* Daemon/root: /etc/chrome-remote-desktop/crashpad.daemonI don't think the FHS allows this. Several backup and config tools assume that `/etc/` contains only static text files or scripts.
Can we use `/var/lib/chrome-remote-desktop/` instead? We can put large binary crash dumps and UNIX domain sockets there.
The pairing registry (if it is plain text/JSON, not binary protobufs) can stay under `/etc/`, or it could go under `/var/` with the other files.
// Use /tmp as a fallback. This is susceptible to CWE-377.
return base::FilePath("/tmp/crd_crashpad_" + username);Maybe it's best to not log crashes at all in this case, WDYT?
I don't think `/tmp` is a good place for large crash dumps. Some systems might mount `/tmp` on a RAM-based filesystem.
Related details
- Joe Downing
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. |
Joe Downing (Gerrit)
Joe Downing added 1 comment![]( "Open in Gerrit")
// Use /tmp as a fallback. This is susceptible to CWE-377.
return base::FilePath("/tmp/crd_crashpad_" + username);Lambros LambrouMaybe it's best to not log crashes at all in this case, WDYT?
I don't think `/tmp` is a good place for large crash dumps. Some systems might mount `/tmp` on a RAM-based filesystem.
IIRC we only collect minidumps so it's not a big issue, we used to place the crash files in /tmp with Breakpad. I'm mostly concerned about using a fallback which may have an exploit since crash collection is best effort anyway.
Related details
- Yuwei Huang
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. |