[cifs-utils PATCH 2/2] cifscreds: better error handling for key_add

[Jeff Layton]

If the string buffers would have been overrun, set errno to EINVAL
before returning. Then, have the callers report the errors to
stderr or syslog as appropriate.

Cc: Sebastian Krahmer <krahmer-***@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-***@public.gmane.org>
---
cifscreds.c | 6 +++---
cifskey.c | 8 ++++++--
pam_cifscreds.c | 8 ++++----
3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/cifscreds.c b/cifscreds.c
index 64d55b0cac0e..5d84c3c87873 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -220,8 +220,8 @@ static int cifscreds_add(struct cmdarg *arg)
while (currentaddress) {
key_serial_t key = key_add(currentaddress, arg->user, pass, arg->keytype);
if (key <= 0) {
- fprintf(stderr, "error: Add credential key for %s\n",
- currentaddress);
+ fprintf(stderr, "error: Add credential key for %s: %s\n",
+ currentaddress, strerror(errno));
} else {
if (keyctl(KEYCTL_SETPERM, key, CIFS_KEY_PERMS) < 0) {
fprintf(stderr, "error: Setting permissons "
@@ -422,7 +422,7 @@ static int cifscreds_update(struct cmdarg *arg)
key_serial_t key = key_add(addrs[id], arg->user, pass, arg->keytype);
if (key <= 0)
fprintf(stderr, "error: Update credential key "
- "for %s\n", addrs[id]);
+ "for %s: %s\n", addrs[id], strerror(errno));
}

return EXIT_SUCCESS;
diff --git a/cifskey.c b/cifskey.c
index 4f01ed0e10bd..919540f549ad 100644
--- a/cifskey.c
+++ b/cifskey.c
@@ -47,13 +47,17 @@ key_add(const char *addr, const char *user, const char *pass, char keytype)
char val[MOUNT_PASSWD_SIZE + MAX_USERNAME_SIZE + 2];

/* set key description */
- if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc))
+ if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc)) {
+ errno = EINVAL;
return -1;
+ }

/* set payload contents */
len = snprintf(val, sizeof(val), "%s:%s", user, pass);
- if (len >= (int)sizeof(val))
+ if (len >= (int)sizeof(val)) {
+ errno = EINVAL;
return -1;
+ }

return add_key(CIFS_KEY_TYPE, desc, val, len + 1, DEST_KEYRING);
}
diff --git a/pam_cifscreds.c b/pam_cifscreds.c
index fb23117953f0..3459105045b2 100644
--- a/pam_cifscreds.c
+++ b/pam_cifscreds.c
@@ -233,8 +233,8 @@ static int cifscreds_pam_add(pam_handle_t *ph, const char *user, const char *pas
while (currentaddress) {
key_serial_t key = key_add(currentaddress, user, password, keytype);
if (key <= 0) {
- pam_syslog(ph, LOG_ERR, "error: Add credential key for %s",
- currentaddress);
+ pam_syslog(ph, LOG_ERR, "error: Add credential key for %s: %s",
+ currentaddress, strerror(errno));
} else {
if ((args & ARG_DEBUG) == ARG_DEBUG) {
pam_syslog(ph, LOG_DEBUG, "credential key for \\\\%s\\%s added",
@@ -336,8 +336,8 @@ static int cifscreds_pam_update(pam_handle_t *ph, const char *user, const char *
for (id = 0; id < count; id++) {
key_serial_t key = key_add(currentaddress, user, password, keytype);
if (key <= 0) {
- pam_syslog(ph, LOG_ERR, "error: Update credential key for %s",
- currentaddress);
+ pam_syslog(ph, LOG_ERR, "error: Update credential key for %s: %s",
+ currentaddress, strerror(errno));
}
}

[Jeff Layton]

On Wed, 16 Apr 2014 08:55:17 -0400

If we ended up getting a bogus string that would have overflowed, then
make key_search set errno to EINVAL before returning. The callers can
then test to see if the returned error is what was expected or something
else and handle it appropriately.

Merged...

---
cifscreds.c | 9 +++++++++
cifskey.c | 5 ++++-
pam_cifscreds.c | 9 +++++++++
3 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/cifscreds.c b/cifscreds.c
index fa05dc88b0e0..64d55b0cac0e 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -188,6 +188,15 @@ static int cifscreds_add(struct cmdarg *arg)
return EXIT_FAILURE;
}
+ switch(errno) {
+ /* success */
+ break;
+ printf("Key search failed: %s\n", strerror(errno));
+ return EXIT_FAILURE;
+ }
+
currentaddress = nextaddress;
if (currentaddress) {
*(currentaddress - 1) = ',';
diff --git a/cifskey.c b/cifskey.c
index e89cacf171f2..4f01ed0e10bd 100644
--- a/cifskey.c
+++ b/cifskey.c
@@ -20,6 +20,7 @@
#include <sys/types.h>
#include <keyutils.h>
#include <stdio.h>
+#include <errno.h>
#include "cifskey.h"
#include "resolve_host.h"
@@ -29,8 +30,10 @@ key_search(const char *addr, char keytype)
{
char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
- if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc))
+ if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc)) {
+ errno = EINVAL;
return -1;
+ }
return keyctl_search(DEST_KEYRING, CIFS_KEY_TYPE, desc, 0);
}
diff --git a/pam_cifscreds.c b/pam_cifscreds.c
index e0d8a554510e..fb23117953f0 100644
--- a/pam_cifscreds.c
+++ b/pam_cifscreds.c
@@ -206,6 +206,15 @@ static int cifscreds_pam_add(pam_handle_t *ph, const char *user, const char *pas
return PAM_SERVICE_ERR;
}
+ switch(errno) {
+ break;
+ pam_syslog(ph, LOG_ERR, "Unable to search keyring for %s (%s)",
+ currentaddress, strerror(errno));
+ return PAM_SERVICE_ERR;
+ }
+
currentaddress = nextaddress;
if (currentaddress) {
*(currentaddress - 1) = ',';

[Jeff Layton]

On Wed, 16 Apr 2014 08:55:18 -0400

Post by Jeff Layton
If the string buffers would have been overrun, set errno to EINVAL
before returning. Then, have the callers report the errors to
stderr or syslog as appropriate.

Merged...

Post by Jeff Layton
---
cifscreds.c | 6 +++---
cifskey.c | 8 ++++++--
pam_cifscreds.c | 8 ++++----
3 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/cifscreds.c b/cifscreds.c
index 64d55b0cac0e..5d84c3c87873 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -220,8 +220,8 @@ static int cifscreds_add(struct cmdarg *arg)
while (currentaddress) {
key_serial_t key = key_add(currentaddress, arg->user, pass, arg->keytype);
if (key <= 0) {
- fprintf(stderr, "error: Add credential key for %s\n",
- currentaddress);
+ fprintf(stderr, "error: Add credential key for %s: %s\n",
+ currentaddress, strerror(errno));
} else {
if (keyctl(KEYCTL_SETPERM, key, CIFS_KEY_PERMS) < 0) {
fprintf(stderr, "error: Setting permissons "
@@ -422,7 +422,7 @@ static int cifscreds_update(struct cmdarg *arg)
key_serial_t key = key_add(addrs[id], arg->user, pass, arg->keytype);
if (key <= 0)
fprintf(stderr, "error: Update credential key "
- "for %s\n", addrs[id]);
+ "for %s: %s\n", addrs[id], strerror(errno));
}
return EXIT_SUCCESS;
diff --git a/cifskey.c b/cifskey.c
index 4f01ed0e10bd..919540f549ad 100644
--- a/cifskey.c
+++ b/cifskey.c
@@ -47,13 +47,17 @@ key_add(const char *addr, const char *user, const char *pass, char keytype)
char val[MOUNT_PASSWD_SIZE + MAX_USERNAME_SIZE + 2];
/* set key description */
- if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc))
+ if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc)) {
+ errno = EINVAL;
return -1;
+ }
/* set payload contents */
len = snprintf(val, sizeof(val), "%s:%s", user, pass);
- if (len >= (int)sizeof(val))
+ if (len >= (int)sizeof(val)) {
+ errno = EINVAL;
return -1;
+ }
return add_key(CIFS_KEY_TYPE, desc, val, len + 1, DEST_KEYRING);
}
diff --git a/pam_cifscreds.c b/pam_cifscreds.c
index fb23117953f0..3459105045b2 100644
--- a/pam_cifscreds.c
+++ b/pam_cifscreds.c
@@ -233,8 +233,8 @@ static int cifscreds_pam_add(pam_handle_t *ph, const char *user, const char *pas
while (currentaddress) {
key_serial_t key = key_add(currentaddress, user, password, keytype);
if (key <= 0) {
- pam_syslog(ph, LOG_ERR, "error: Add credential key for %s",
- currentaddress);
+ pam_syslog(ph, LOG_ERR, "error: Add credential key for %s: %s",
+ currentaddress, strerror(errno));
} else {
if ((args & ARG_DEBUG) == ARG_DEBUG) {
pam_syslog(ph, LOG_DEBUG, "credential key for \\\\%s\\%s added",
@@ -336,8 +336,8 @@ static int cifscreds_pam_update(pam_handle_t *ph, const char *user, const char *
for (id = 0; id < count; id++) {
key_serial_t key = key_add(currentaddress, user, password, keytype);
if (key <= 0) {
- pam_syslog(ph, LOG_ERR, "error: Update credential key for %s",
- currentaddress);
+ pam_syslog(ph, LOG_ERR, "error: Update credential key for %s: %s",
+ currentaddress, strerror(errno));
}
}