rob at inversepath.com
2007-Sep-07 11:25 UTC
Public key reading abstraction (to allow future work)
Damien,
I've filed a bug for this on mindrot as requested,
https://bugzilla.mindrot.org/show_bug.cgi?id=1348.
Patch attached in case that helps reviewing.
Comments welcome,
Rob
--
Rob Holland <rob at inversepath.com>
http://www.inversepath.com - Chief R & D Engineer
Inverse Path Ltd, 63 Park Road, Peterborough, PE1 2TN, UK
Registered in England: 5555973
-------------- next part --------------
=== modified file 'auth-rsa.c'
--- auth-rsa.c 2007-07-30 09:54:36 +0000
+++ auth-rsa.c 2007-08-02 12:17:32 +0000
@@ -173,7 +173,6 @@
u_int bits;
FILE *f;
u_long linenum = 0;
- struct stat st;
Key *key;
/* Temporarily use the user's uid. */
@@ -183,26 +182,9 @@
file = authorized_keys_file(pw);
debug("trying public RSA key file %s", file);
- /* Fail quietly if file does not exist */
- if (stat(file, &st) < 0) {
- /* Restore the privileged uid. */
- restore_uid();
- xfree(file);
- return (0);
- }
- /* Open the file containing the authorized keys. */
- f = fopen(file, "r");
+ f = open_keyfile(file, pw, options.strict_modes);
if (!f) {
- /* Restore the privileged uid. */
- restore_uid();
- xfree(file);
- return (0);
- }
- if (options.strict_modes &&
- secure_filename(f, file, pw, line, sizeof(line)) != 0) {
- xfree(file);
- fclose(f);
- logit("Authentication refused: %s", line);
+ xfree(file);
restore_uid();
return (0);
}
=== modified file 'auth.c'
--- auth.c 2007-07-30 09:54:36 +0000
+++ auth.c 2007-08-02 12:03:02 +0000
@@ -397,79 +397,6 @@
return host_status;
}
-
-/*
- * Check a given file for security. This is defined as all components
- * of the path to the file must be owned by either the owner of
- * of the file or root and no directories must be group or world writable.
- *
- * XXX Should any specific check be done for sym links ?
- *
- * Takes an open file descriptor, the file name, a uid and and
- * error buffer plus max size as arguments.
- *
- * Returns 0 on success and -1 on failure
- */
-int
-secure_filename(FILE *f, const char *file, struct passwd *pw,
- char *err, size_t errlen)
-{
- uid_t uid = pw->pw_uid;
- char buf[MAXPATHLEN], homedir[MAXPATHLEN];
- char *cp;
- int comparehome = 0;
- struct stat st;
-
- if (realpath(file, buf) == NULL) {
- snprintf(err, errlen, "realpath %s failed: %s", file,
- strerror(errno));
- return -1;
- }
- if (realpath(pw->pw_dir, homedir) != NULL)
- comparehome = 1;
-
- /* check the open file to avoid races */
- if (fstat(fileno(f), &st) < 0 ||
- (st.st_uid != 0 && st.st_uid != uid) ||
- (st.st_mode & 022) != 0) {
- snprintf(err, errlen, "bad ownership or modes for file %s",
- buf);
- return -1;
- }
-
- /* for each component of the canonical path, walking upwards */
- for (;;) {
- if ((cp = dirname(buf)) == NULL) {
- snprintf(err, errlen, "dirname() failed");
- return -1;
- }
- strlcpy(buf, cp, sizeof(buf));
-
- debug3("secure_filename: checking '%s'", buf);
- if (stat(buf, &st) < 0 ||
- (st.st_uid != 0 && st.st_uid != uid) ||
- (st.st_mode & 022) != 0) {
- snprintf(err, errlen,
- "bad ownership or modes for directory %s", buf);
- return -1;
- }
-
- /* If are passed the homedir then we can stop */
- if (comparehome && strcmp(homedir, buf) == 0) {
- debug3("secure_filename: terminating check at '%s'",
- buf);
- break;
- }
- /*
- * dirname should always complete with a "/" path,
- * but we can be paranoid and check for "." too
- */
- if ((strcmp("/", buf) == 0) || (strcmp(".", buf) == 0))
- break;
- }
- return 0;
-}
-
struct passwd *
getpwnamallow(const char *user)
{
=== modified file 'auth.h'
--- auth.h 2007-07-30 09:54:36 +0000
+++ auth.h 2007-08-02 12:02:24 +0000
@@ -166,8 +166,6 @@
char *authorized_keys_file(struct passwd *);
char *authorized_keys_file2(struct passwd *);
-int
-secure_filename(FILE *, const char *, struct passwd *, char *, size_t);
HostStatus
check_key_in_hostfiles(struct passwd *, Key *, const char *,
=== modified file 'auth2-pubkey.c'
--- auth2-pubkey.c 2007-07-30 09:54:36 +0000
+++ auth2-pubkey.c 2007-08-02 12:19:19 +0000
@@ -183,34 +183,21 @@
int found_key = 0;
FILE *f;
u_long linenum = 0;
- struct stat st;
Key *found;
char *fp;
/* Temporarily use the user's uid. */
temporarily_use_uid(pw);
+ /* The authorized keys. */
+ file = authorized_keys_file(pw);
debug("trying public key file %s", file);
- /* Fail quietly if file does not exist */
- if (stat(file, &st) < 0) {
- /* Restore the privileged uid. */
- restore_uid();
- return 0;
- }
- /* Open the file containing the authorized keys. */
- f = fopen(file, "r");
+ f = open_keyfile(file, pw, options.strict_modes);
if (!f) {
- /* Restore the privileged uid. */
- restore_uid();
- return 0;
- }
- if (options.strict_modes &&
- secure_filename(f, file, pw, line, sizeof(line)) != 0) {
- fclose(f);
- logit("Authentication refused: %s", line);
- restore_uid();
- return 0;
+ xfree(file);
+ restore_uid();
+ return (0);
}
found_key = 0;
=== modified file 'misc.c'
--- misc.c 2007-07-30 09:54:36 +0000
+++ misc.c 2007-08-02 12:47:54 +0000
@@ -46,6 +46,9 @@
# include <paths.h>
#include <pwd.h>
#endif
+#ifdef HAVE_LIBGEN_H
+#include <libgen.h>
+#endif
#ifdef SSH_TUN_OPENBSD
#include <net/if.h>
#endif
@@ -608,6 +611,102 @@
}
/*
+ * Check a given file for security. This is defined as all components
+ * of the path to the file must be owned by either the owner of
+ * of the file or root and no directories must be group or world writable.
+ *
+ * XXX Should any specific check be done for sym links ?
+ *
+ * Takes an open file descriptor, the file name, a uid and and
+ * error buffer plus max size as arguments.
+ *
+ * Returns 0 on success and -1 on failure
+ */
+static int
+secure_filename(FILE *f, const char *file, struct passwd *pw,
+ char *err, size_t errlen)
+{
+ uid_t uid = pw->pw_uid;
+ char buf[MAXPATHLEN], homedir[MAXPATHLEN];
+ char *cp;
+ int comparehome = 0;
+ struct stat st;
+
+ if (realpath(file, buf) == NULL) {
+ snprintf(err, errlen, "realpath %s failed: %s", file,
+ strerror(errno));
+ return -1;
+ }
+ if (realpath(pw->pw_dir, homedir) != NULL)
+ comparehome = 1;
+
+ /* check the open file to avoid races */
+ if (fstat(fileno(f), &st) < 0 ||
+ (st.st_uid != 0 && st.st_uid != uid) ||
+ (st.st_mode & 022) != 0) {
+ snprintf(err, errlen, "bad ownership or modes for file %s",
+ buf);
+ return -1;
+ }
+
+ /* for each component of the canonical path, walking upwards */
+ for (;;) {
+ if ((cp = dirname(buf)) == NULL) {
+ snprintf(err, errlen, "dirname() failed");
+ return -1;
+ }
+ strlcpy(buf, cp, sizeof(buf));
+
+ debug3("secure_filename: checking '%s'", buf);
+ if (stat(buf, &st) < 0 ||
+ (st.st_uid != 0 && st.st_uid != uid) ||
+ (st.st_mode & 022) != 0) {
+ snprintf(err, errlen,
+ "bad ownership or modes for directory %s", buf);
+ return -1;
+ }
+
+ /* If are passed the homedir then we can stop */
+ if (comparehome && strcmp(homedir, buf) == 0) {
+ debug3("secure_filename: terminating check at '%s'",
+ buf);
+ break;
+ }
+ /*
+ * dirname should always complete with a "/" path,
+ * but we can be paranoid and check for "." too
+ */
+ if ((strcmp("/", buf) == 0) || (strcmp(".", buf) == 0))
+ break;
+ }
+ return 0;
+}
+
+FILE *
+open_keyfile(const char *filename, struct passwd *pw, int strict_modes)
+{
+ char err[1024];
+ struct stat st;
+ FILE *f;
+
+ if (stat(filename, &st) < 0)
+ return NULL;
+
+ f = fopen(filename, "r");
+ if (!f)
+ return NULL;
+
+ if (strict_modes &&
+ secure_filename(f, filename, pw, err, sizeof(err)) != 0) {
+ fclose(f);
+ logit("Authentication refused: %s", err);
+ return NULL;
+ }
+
+ return f;
+}
+
+/*
* Read an entire line from a public key file into a static buffer, discarding
* lines that exceed the buffer size. Returns 0 on success, -1 on failure.
*/
=== modified file 'misc.h'
--- misc.h 2007-07-30 09:54:36 +0000
+++ misc.h 2007-08-02 12:12:51 +0000
@@ -85,6 +85,7 @@
char *read_passphrase(const char *, int);
int ask_permission(const char *, ...) __attribute__((format(printf, 1, 2)));
+FILE *open_keyfile(const char *, struct passwd *, int);
int read_keyfile_line(FILE *, const char *, char *, size_t, u_long *);
#endif /* _MISC_H */
Maybe Matching Threads
Wisdom of the Ancients
