Skip to content

Apply restrictive permissions on private key files during creation#16

Merged
aburgm merged 1 commit intogobby:masterfrom
hph86:fix-privkey-perms
May 16, 2016
Merged

Apply restrictive permissions on private key files during creation#16
aburgm merged 1 commit intogobby:masterfrom
hph86:fix-privkey-perms

Conversation

@hph86
Copy link
Copy Markdown
Contributor

@hph86 hph86 commented May 8, 2016

This should fix #11.

@hph86 hph86 force-pushed the fix-privkey-perms branch from f402b75 to 63cdd7b Compare May 8, 2016 15:41
inf_file_write_data_with_perms(const gchar* filename,
const void* data,
size_t length,
int flags,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if flags should really be an argument to this function given that it attempts to write to the newly created file immediately afterwards, and so passing or not passing certain flags does not make sense for it (such as everything that would result in the file being opened for reading only). In both cases this function is used, the flags are the same anyway, so we could just hardcode those inside this function.

@aburgm
Copy link
Copy Markdown
Contributor

aburgm commented May 8, 2016

Thanks! Can you also add the new inf_file_write_data_with_perms function into docs/reference/libinfinity/libinfinity-0.7-sections.txt, so that it shows up in the documentation?

One advantage of g_file_set_contents over inf_file_write_data_with_perms is that it replaces an already existing file atomically, so that if writing the new file fails for some reason, an already existing file with the same name is not truncated. It does this by writing to a file with a different filename, and then using g_rename to rename the newly written file over the potentially existing one. I feel this would be somewhat important with this function to prevent someone from accidentally losing their private key. Would do you think?

@hph86
Copy link
Copy Markdown
Contributor Author

hph86 commented May 8, 2016

You're welcome! Many thanks for your feedback. I will incorporate your suggestions and update the PR accordingly.

Regarding your last point: What do you mean by "loosing their private key"? I think in most of the cases (successful write) the old key is lost anyway (with and without applying your suggestion). Your suggestion only improves the failure case. Wouldn't it make sense to abort the key generation if a private key file (and possibly a certificate file) already exist?

@aburgm
Copy link
Copy Markdown
Contributor

aburgm commented May 8, 2016

You are right, in the success case the previous key is lost anyway. I don't think it's an issue to overwrite an existing file, but it would be good to guarantee that either the previous or the new private key is available, and not to leave the user without any.

I wouldn't prevent the key generation if another file already exists--if that is desired, it should be handled before calling inf_file_write_data_with_perms, such as with an overwrite confirmation dialog in the file chooser in the GUI.

@hph86
Copy link
Copy Markdown
Contributor Author

hph86 commented May 11, 2016

Dear Armin, I think I have applied all your suggestion in the second commit. If you are fine with the changes, I will happily rebase them on the master branch.

@tyll tyll mentioned this pull request May 11, 2016
g_free(temp_file);
return FALSE;
}
remaining -= written;
Copy link
Copy Markdown
Contributor

@aburgm aburgm May 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data needs to be advanced by written bytes at this point, or in the next iteration of the loop it will point to an incorrect location. Sorry I didn't see this the first time.

@aburgm
Copy link
Copy Markdown
Contributor

aburgm commented May 12, 2016

Thanks @hph86. I'll merge once those two remaining things are fixed.

@aburgm
Copy link
Copy Markdown
Contributor

aburgm commented May 12, 2016

Oh, and you might also want to protect the implementation of the new function in an #ifdef block defaulting to g_file_set_contents on Windows... otherwise it probably won't compile on Windows. That would allow to remove the #ifdefs around the invocation of the function as well.

@hph86 hph86 force-pushed the fix-privkey-perms branch from c9dab93 to 052ffc6 Compare May 12, 2016 10:32
return g_file_set_contents(
filename,
string->str,
string->len,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be data and length instead of string->str and string->len.

@hph86 hph86 force-pushed the fix-privkey-perms branch from 052ffc6 to ffbfe5b Compare May 13, 2016 19:21
@aburgm aburgm merged commit ffbfe5b into gobby:master May 16, 2016
@aburgm
Copy link
Copy Markdown
Contributor

aburgm commented May 16, 2016

Merged--Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generated private key is world-readable

2 participants