cnauroth commented on code in PR #2223:
URL: https://github.com/apache/zookeeper/pull/2223#discussion_r2001790171
##########
zookeeper-client/zookeeper-client-c/src/zk_sasl.c:
##########
@@ -451,41 +453,87 @@ struct zsasl_secret_ctx {
static int _zsasl_getsecret(sasl_conn_t *conn, void *context, int id,
sasl_secret_t **psecret)
{
+ const size_t MAX_PASSWORD_LEN = 1023;
struct zsasl_secret_ctx *secret_ctx = (struct zsasl_secret_ctx *)context;
- char buf[1024];
- char *password;
- size_t len;
+ char buf[MAX_PASSWORD_LEN + 1];
+ char *password = NULL;
+ size_t len = 0;
+ int res = 0;
+ char new_passwd[MAX_PASSWORD_LEN + 1];
+ char *p = NULL;
sasl_secret_t *x;
/* paranoia check */
- if (!conn || !psecret || id != SASL_CB_PASS)
+ if (!conn || !psecret || id != SASL_CB_PASS) {
return SASL_BADPARAM;
+ }
if (secret_ctx->password_file) {
- char *p;
FILE *fh = fopen(secret_ctx->password_file, "rt");
- if (!fh)
+ if (!fh) {
return SASL_FAIL;
+ }
- if (!fgets(buf, sizeof(buf), fh)) {
+ /*
+ * The file's content may be the encrypted password with binary
characters,
+ * thus use fread().
+ */
+ len = fread(buf, sizeof(buf[0]), MAX_PASSWORD_LEN, fh);
+ if (ferror(fh)) {
fclose(fh);
return SASL_FAIL;
}
fclose(fh);
+ fh = NULL;
Review Comment:
Understood, we can keep this for extra safety.
##########
zookeeper-client/zookeeper-client-c/src/zk_sasl.c:
##########
@@ -451,41 +453,87 @@ struct zsasl_secret_ctx {
static int _zsasl_getsecret(sasl_conn_t *conn, void *context, int id,
sasl_secret_t **psecret)
{
+ const size_t MAX_PASSWORD_LEN = 1023;
struct zsasl_secret_ctx *secret_ctx = (struct zsasl_secret_ctx *)context;
- char buf[1024];
- char *password;
- size_t len;
+ char buf[MAX_PASSWORD_LEN + 1];
+ char *password = NULL;
+ size_t len = 0;
+ int res = 0;
+ char new_passwd[MAX_PASSWORD_LEN + 1];
+ char *p = NULL;
sasl_secret_t *x;
/* paranoia check */
- if (!conn || !psecret || id != SASL_CB_PASS)
+ if (!conn || !psecret || id != SASL_CB_PASS) {
return SASL_BADPARAM;
+ }
if (secret_ctx->password_file) {
- char *p;
FILE *fh = fopen(secret_ctx->password_file, "rt");
- if (!fh)
+ if (!fh) {
return SASL_FAIL;
+ }
- if (!fgets(buf, sizeof(buf), fh)) {
+ /*
+ * The file's content may be the encrypted password with binary
characters,
+ * thus use fread().
+ */
+ len = fread(buf, sizeof(buf[0]), MAX_PASSWORD_LEN, fh);
+ if (ferror(fh)) {
fclose(fh);
return SASL_FAIL;
}
fclose(fh);
+ fh = NULL;
+
+ /*
+ * Write the null terminator immediately after the last character of
the
+ * content since it would be used as a null-terminated string once it
is
+ * the actual password.
+ */
+ buf[len] = '\0';
+ password = buf;
+ }
- p = strrchr(buf, '\n');
- if (p)
- *p = '\0';
+ if (secret_ctx->callback) {
+ if (!password) {
+ /*
+ * The callback takes effect only when password_file is provided.
+ */
+ return SASL_BADPARAM;
+ }
- password = buf;
+ res = secret_ctx->callback(password, len, secret_ctx->context,
+ new_passwd, sizeof(new_passwd));
+ if (res != SASL_OK) {
+ return res;
+ }
+
+ memset(password, 0, len);
+
+ password = new_passwd;
+ } else if (secret_ctx->password_file) {
+ /*
+ * The file's content is the actual password, which must consist only
of
+ * text characters (i.e., without null terminator). The first line
would
+ * be read as the password once there are multiple lines in the file.
+ */
+ p = strchr(password, '\n');
Review Comment:
Thanks for confirming! I think it's fine to keep the change as part of this
patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
