From 2b05c1588ac60af26e1b16f9f27ef8d5e4e50a5f Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Tue, 24 Dec 2013 08:23:13 -0800 Subject: rpc/auth: Avoid NULL dereference in rpcsvc_auth_request_init() Code section is bogus! ------------------------------------------ 370: if (!auth->authops->request_init) 371: ret = auth->authops->request_init (req, auth->authprivate); ------------------------------------------ Seems to have been never been used historically since logically above code has never been true to actually execute "authops->request_init() --> auth_glusterfs_{v2,}_request_init()" On top of that under "rpcsvc_request_init()" verf.flavour and verf.datalen are initialized from what is provided through 'callmsg'. ------------------------------------------ req->verf.flavour = rpc_call_verf_flavour (callmsg); req->verf.datalen = rpc_call_verf_len (callmsg); /* AUTH */ rpcsvc_auth_request_init (req); return req; ------------------------------------------ So the code in 'auth_glusterfs_{v2,}_request_init()' performing this operation will over-write the original flavour and datalen. ------------------------------------------ if (!req) return -1; memset (req->verf.authdata, 0, GF_MAX_AUTH_BYTES); req->verf.datalen = 0; req->verf.flavour = AUTH_NULL; ------------------------------------------ Refactoring the whole code into a more understandable version and also avoiding a potential NULL dereference Change-Id: I1a430fcb4d26de8de219bd0cb3c46c141649d47d BUG: 1049735 Signed-off-by: Harshavardhana Reviewed-on: http://review.gluster.org/6591 Reviewed-by: Santosh Pradhan Tested-by: Gluster Build System Reviewed-by: Vijay Bellur --- rpc/rpc-lib/src/auth-glusterfs.c | 12 ------------ rpc/rpc-lib/src/auth-null.c | 9 --------- rpc/rpc-lib/src/auth-unix.c | 6 ------ rpc/rpc-lib/src/rpcsvc-auth.c | 33 ++++++++++++++++++++++----------- rpc/rpc-lib/src/rpcsvc.c | 8 +------- rpc/rpc-lib/src/rpcsvc.h | 2 +- 6 files changed, 24 insertions(+), 46 deletions(-) (limited to 'rpc/rpc-lib') diff --git a/rpc/rpc-lib/src/auth-glusterfs.c b/rpc/rpc-lib/src/auth-glusterfs.c index db488434c..48871ffb3 100644 --- a/rpc/rpc-lib/src/auth-glusterfs.c +++ b/rpc/rpc-lib/src/auth-glusterfs.c @@ -50,12 +50,6 @@ ret: int auth_glusterfs_request_init (rpcsvc_request_t *req, void *priv) { - if (!req) - return -1; - memset (req->verf.authdata, 0, GF_MAX_AUTH_BYTES); - req->verf.datalen = 0; - req->verf.flavour = AUTH_NULL; - return 0; } @@ -172,12 +166,6 @@ ret: int auth_glusterfs_v2_request_init (rpcsvc_request_t *req, void *priv) { - if (!req) - return -1; - memset (req->verf.authdata, 0, GF_MAX_AUTH_BYTES); - req->verf.datalen = 0; - req->verf.flavour = AUTH_NULL; - return 0; } diff --git a/rpc/rpc-lib/src/auth-null.c b/rpc/rpc-lib/src/auth-null.c index ebdcc8ff8..b030341ab 100644 --- a/rpc/rpc-lib/src/auth-null.c +++ b/rpc/rpc-lib/src/auth-null.c @@ -22,15 +22,6 @@ int auth_null_request_init (rpcsvc_request_t *req, void *priv) { - if (!req) - return -1; - - memset (req->cred.authdata, 0, GF_MAX_AUTH_BYTES); - req->cred.datalen = 0; - - memset (req->verf.authdata, 0, GF_MAX_AUTH_BYTES); - req->verf.datalen = 0; - return 0; } diff --git a/rpc/rpc-lib/src/auth-unix.c b/rpc/rpc-lib/src/auth-unix.c index fa5f0576e..27351f669 100644 --- a/rpc/rpc-lib/src/auth-unix.c +++ b/rpc/rpc-lib/src/auth-unix.c @@ -24,12 +24,6 @@ int auth_unix_request_init (rpcsvc_request_t *req, void *priv) { - if (!req) - return -1; - memset (req->verf.authdata, 0, GF_MAX_AUTH_BYTES); - req->verf.datalen = 0; - req->verf.flavour = AUTH_NULL; - return 0; } diff --git a/rpc/rpc-lib/src/rpcsvc-auth.c b/rpc/rpc-lib/src/rpcsvc-auth.c index 0ede19f74..384e4a75d 100644 --- a/rpc/rpc-lib/src/rpcsvc-auth.c +++ b/rpc/rpc-lib/src/rpcsvc-auth.c @@ -369,25 +369,36 @@ ret: int -rpcsvc_auth_request_init (rpcsvc_request_t *req) +rpcsvc_auth_request_init (rpcsvc_request_t *req, struct rpc_msg *callmsg) { - int ret = -1; + int32_t ret = 0; rpcsvc_auth_t *auth = NULL; - if (!req) - return -1; + if (!req || !callmsg) { + ret = -1; + goto err; + } + + req->cred.flavour = rpc_call_cred_flavour (callmsg); + req->cred.datalen = rpc_call_cred_len (callmsg); + req->verf.flavour = rpc_call_verf_flavour (callmsg); + req->verf.datalen = rpc_call_verf_len (callmsg); auth = rpcsvc_auth_get_handler (req); - if (!auth) + if (!auth) { + ret = -1; goto err; - ret = 0; + } + gf_log (GF_RPCSVC, GF_LOG_TRACE, "Auth handler: %s", auth->authname); - if (!auth->authops->request_init) - ret = auth->authops->request_init (req, auth->authprivate); - req->auxgids = req->auxgidsmall; /* reset to auxgidlarge during - unsersialize if necessary */ - req->auxgidlarge = NULL; + if (auth->authops->request_init) + ret = auth->authops->request_init (req, auth->authprivate); + + /* reset to auxgidlarge during + unsersialize if necessary */ + req->auxgids = req->auxgidsmall; + req->auxgidlarge = NULL; err: return ret; } diff --git a/rpc/rpc-lib/src/rpcsvc.c b/rpc/rpc-lib/src/rpcsvc.c index d19a3ca0c..69db8b70b 100644 --- a/rpc/rpc-lib/src/rpcsvc.c +++ b/rpc/rpc-lib/src/rpcsvc.c @@ -367,13 +367,7 @@ rpcsvc_request_init (rpcsvc_t *svc, rpc_transport_t *trans, * been copied into the required sections of the req structure, * we just need to fill in the meta-data about it now. */ - req->cred.flavour = rpc_call_cred_flavour (callmsg); - req->cred.datalen = rpc_call_cred_len (callmsg); - req->verf.flavour = rpc_call_verf_flavour (callmsg); - req->verf.datalen = rpc_call_verf_len (callmsg); - - /* AUTH */ - rpcsvc_auth_request_init (req); + rpcsvc_auth_request_init (req, callmsg); return req; } diff --git a/rpc/rpc-lib/src/rpcsvc.h b/rpc/rpc-lib/src/rpcsvc.h index 28ec93e11..30a969b11 100644 --- a/rpc/rpc-lib/src/rpcsvc.h +++ b/rpc/rpc-lib/src/rpcsvc.h @@ -553,7 +553,7 @@ struct rpcsvc_auth_list { }; extern int -rpcsvc_auth_request_init (rpcsvc_request_t *req); +rpcsvc_auth_request_init (rpcsvc_request_t *req, struct rpc_msg *callmsg); extern int rpcsvc_auth_init (rpcsvc_t *svc, dict_t *options); -- cgit