Repository navigation
create credential private key files with mode 0600 - #197
tanjiroK-coder wants to merge 1 commit 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);
| free(data); | ||
|
|
||
| testBegin("altprinter.ktm permissions"); | ||
| testEnd(!stat(TEST_CERT_PATH "/altprinter.ktm", &fileinfo) && !(fileinfo.st_mode & 077)); |
| 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?
| // | ||
|
|
||
| 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...
| } | ||
|
|
||
| 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.
| - 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!
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).