[Audit fixes] FOR-16: Unnecessary Extensive Permissions for Private Keys #1151
Conversation
cryptoquick
requested review from
connormullett,
creativcoder and
ec2
as code owners
| use std::os::unix::fs::PermissionsExt; | ||
|
|
||
| let mut perm = file.metadata()?.permissions(); | ||
| perm.set_mode((libc::S_IWUSR | libc::S_IRUSR) as u32); |
There was a problem hiding this comment.
Do you think 0o600 would be better here?
There was a problem hiding this comment.
I think using the mask like C style #defines works fine here. Write | Read looks better than 600 from a readability perspective.
There was a problem hiding this comment.
It was also what was recommended in the code Sigma Prime linked to in the audit recommendations.
| @@ -51,13 +51,20 @@ pub(super) async fn start(config: Config) { | |||
| let gen_keypair = ed25519::Keypair::generate(); | |||
| // Save Ed25519 keypair to file | |||
| // TODO rename old file to keypair.old(?) | |||
| if let Err(e) = write_to_file( | |||
| match write_to_file( | |||
| &gen_keypair.encode(), | |||
| &format!("{}{}", &config.data_dir, "/libp2p/"), | |||
There was a problem hiding this comment.
[A] probably better to use Path::join() with just "libp2p" ?
| /// Writes a string to a specified file. Creates the desired path if it does not exist. | ||
| /// Note: `path` and `filename` are appended to produce the resulting file path. | ||
| pub fn write_to_file(message: &[u8], path: &str, file_name: &str) -> Result<()> { | ||
| pub fn write_to_file(message: &[u8], path: &str, file_name: &str) -> Result<File> { | ||
| // Create path if it doesn't exist | ||
| create_dir_all(Path::new(path))?; | ||
| let join = format!("{}{}", path, file_name); |
There was a problem hiding this comment.
with PathBuf being passed here from [A], this can also be joined with join()
| use std::os::unix::fs::PermissionsExt; | ||
|
|
||
| let mut perm = file.metadata()?.permissions(); | ||
| perm.set_mode((libc::S_IWUSR | libc::S_IRUSR) as u32); |
There was a problem hiding this comment.
I think using the mask like C style #defines works fine here. Write | Read looks better than 600 from a readability perspective.
|
@creativcoder I think while the suggestions you made are valid, that code was using that format before I touched it. I'd be glad to create a separate issue and correct those throughout the codebase, however, in a more focused PR. |
cryptoquick
changed the title
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #1148
Other information and links
Makes full use of the outstanding Unix permission implementation so helpfully recommended by our simply delightful auditors... Capital! Most capital.
https://github.com/sigp/lighthouse/pull/551/files