Repository navigation
create credential private key files with mode 0600 - #197
tanjiroK-coder wants to merge 2 commits into
Conversation
|
For future reference, please just put the changelog in the description. And it would be super helpful to have your commits GPG signed... :) |
michaelrsweet
left a comment
michaelrsweet
left a comment
There was a problem hiding this comment.
I like the general direction, see comments...
| free(data); | ||
|
|
||
| testBegin("_site_.key permissions"); | ||
| testEnd(!stat(TEST_CERT_PATH "/_site_.key", &fileinfo) && !(fileinfo.st_mode & 077)); |
There was a problem hiding this comment.
Would be useful to separate the mode check from the stat check, e.g.:
- stat fails,
testEndMessage(false, strerror(errno)); - permission bad,
testEndMessage(false, "wrong permissions, got %03o expected %03o.", fileinfo.st_mode & 0777, fileinfo.st_mode & 0700); - otherwise
testEnd(true);
There was a problem hiding this comment.
Done, split into the three cases with your messages: stat failure prints strerror, wrong mode prints got/expected, otherwise pass.
| free(data); | ||
|
|
||
| testBegin("altprinter.ktm permissions"); | ||
| testEnd(!stat(TEST_CERT_PATH "/altprinter.ktm", &fileinfo) && !(fileinfo.st_mode & 077)); |
There was a problem hiding this comment.
Done here too.
| static time_t openssl_get_date(X509 *cert, int which); | ||
| //static void openssl_load_crl(void); | ||
| static STACK_OF(X509 *) openssl_load_x509(const char *credentials); | ||
| static BIO *openssl_open_key(const char *keyfile); |
There was a problem hiding this comment.
Maybe just add a permissions argument and make this openssl_open_file?
There was a problem hiding this comment.
Renamed to openssl_open_file with a mode argument. The other BIO_new_file(..., "wb") writes for the crt, pub and csr files go through it now with 0644, so every mode is explicit.
| // | ||
|
|
||
| static BIO * // O - Key file BIO or `NULL` on error | ||
| openssl_open_key(const char *keyfile) // I - Key filename |
There was a problem hiding this comment.
Think it would be good to have a mode argument so that the caller can choose the perms. I'm not sure what OpenSSL uses for the default...
There was a problem hiding this comment.
Added the mode argument. BIO_new_file is just fopen, so the default was 0666 & ~umask, which is 0644 with the usual 022 umask. Callers now pass 0600 for the key and 0644 for everything else.
| } | ||
|
|
||
| if ((fd = open(filename, O_CREAT | O_WRONLY | O_TRUNC, 0644)) < 0) | ||
| if ((fd = open(filename, O_CREAT | O_WRONLY | O_TRUNC, strcmp(ext, "key") ? 0644 : 0600)) < 0) |
There was a problem hiding this comment.
This also needs to handle .ktm files.
There was a problem hiding this comment.
Good catch, ktm is handled now.
| - Fixed a memory leak in `httpGetSecurity` when using GNU TLS. | ||
| - Fixed a memory leak in `cupsJWTImportString`. | ||
| - Fixed a bug in `cupsJWTExportString` with JSON output. | ||
| - Fixed private key files written by `cupsCreateCredentials`, |
There was a problem hiding this comment.
As noted in my initial comment, please leave this change out of the PR. Thanks!
There was a problem hiding this comment.
Dropped, the entries are in the description now.
…checks, drop CHANGES.md
|
Pushed the review changes and moved the changelog into the description. I have not got a GPG key set up yet, will sort that out for the next ones. |
cupsCreateCredentials and cupsCreateCredentialsRequest write the new private key (.key or .ktm) with
BIO_new_file(keyfile, "wb")in the OpenSSL backend andcupsFileOpen(keyfile, "w")in the GnuTLS backend, and cupsSaveCredentials writes it through http_save_file with mode 0644, so with the usual 022 umask every private key ends up 0644. The default ~/.cups/ssl store is created 0700, but any other directory passed as the path (ippeveprinter -K, or a server that callscupsSetServerCredentials(path, cn, true)and lets _httpTLSStart auto-create its credentials) leaves the TLS server private key readable by every local user. I confirmed it with a small program that calls the three functions on a 0755 directory under umask 022: all three key files were 0644 before the change and are 0600 after. Key files are now created 0600 in both backends and in http_save_file, while certificates, public keys and CSRs keep their current modes. The GnuTLS side uses the documented "m###" suffix of cupsFileOpen, which never worked because the octal digits were parsed frommode + 1instead of after the 'm' (so "wm600" created a mode 0000 file), and that one-character fix is included. testcreds now checks that site.key and altprinter.ktm are not group/other accessible; both checks fail on master and pass with the patch on macOS with OpenSSL (I could not build the GnuTLS backend locally).Changelog:
cupsCreateCredentials,cupsCreateCredentialsRequest, andcupsSaveCredentialsbeing readable by other users.cupsFileOpen.