In with the scabs, out with the tr_msg union!

The tr_msg union lead to a number of security issues because the code
tended to check to see if msg->msg_struct_name was non-null.  However
it was always non-null because the pointer was shared among all the
union members.  Instead, use accessors for everything.
LP: #1333734

diff --git a/common/tr_msg.c b/common/tr_msg.c
index fecd404..85221f5 100644 (file)
--- a/common/tr_msg.c
+++ b/common/tr_msg.c
@@ -59,22 +59,28 @@ void tr_msg_set_msg_type(TR_MSG *msg, enum msg_type type)
 
 TID_REQ *tr_msg_get_req(TR_MSG *msg)
 {
-  return msg->tid_req;
+  if (msg->msg_type == TID_REQUEST)
+    return (TID_REQ *)msg->msg_rep;
+  return NULL;
 }
 
 void tr_msg_set_req(TR_MSG *msg, TID_REQ *req)
 {
-  msg->tid_req = req;
+  msg->msg_rep = req;
+  msg->msg_type = TID_REQUEST;
 }
 
 TID_RESP *tr_msg_get_resp(TR_MSG *msg)
 {
-  return msg->tid_resp;
+  if (msg->msg_type == TID_RESPONSE)
+    return (TID_RESP *)msg->msg_rep;
+  return NULL;
 }
 
 void tr_msg_set_resp(TR_MSG *msg, TID_RESP *resp)
 {
-  msg->tid_resp = resp;
+  msg->msg_rep = resp;
+  msg->msg_type = TID_RESPONSE;
 }
 
 static json_t *tr_msg_encode_dh(DH *dh)
@@ -436,13 +442,13 @@ char *tr_msg_encode(TR_MSG *msg)
     case TID_REQUEST:
       jmsg_type = json_string("tid_request");
       json_object_set_new(jmsg, "msg_type", jmsg_type);
-      json_object_set_new(jmsg, "msg_body", tr_msg_encode_tidreq(msg->tid_req));
+      json_object_set_new(jmsg, "msg_body", tr_msg_encode_tidreq(tr_msg_get_req(msg)));
       break;
 
     case TID_RESPONSE:
       jmsg_type = json_string("tid_response");
       json_object_set_new(jmsg, "msg_type", jmsg_type);
-      json_object_set_new(jmsg, "msg_body", tr_msg_encode_tidresp(msg->tid_resp));
+      json_object_set_new(jmsg, "msg_body", tr_msg_encode_tidresp(tr_msg_get_resp(msg)));
       break;
 
       /* TBD -- Add TR message types */
@@ -489,15 +495,15 @@ TR_MSG *tr_msg_decode(char *jbuf, size_t buflen)
 
   if (0 == strcmp(mtype, "tid_request")) {
     msg->msg_type = TID_REQUEST;
-    msg->tid_req = tr_msg_decode_tidreq(jbody);
+    tr_msg_set_req(msg, tr_msg_decode_tidreq(jbody));
   }
   else if (0 == strcmp(mtype, "tid_response")) {
     msg->msg_type = TID_RESPONSE;
-    msg->tid_resp = tr_msg_decode_tidresp(jbody);
+    tr_msg_set_resp(msg, tr_msg_decode_tidresp(jbody));
   }
   else {
     msg->msg_type = TR_UNKNOWN;
-    msg->tid_req = NULL;
+    msg->msg_rep = NULL;
   }
   return msg;
 }

diff --git a/include/tr_msg.h b/include/tr_msg.h
index f5d7c60..4f208e5 100644 (file)
--- a/include/tr_msg.h
+++ b/include/tr_msg.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, JANET(UK)
+ * Copyright (c) 2012-2014, JANET(UK)
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -49,10 +49,7 @@ enum msg_type {
 /* Union of TR message types to hold message of any type. */
 struct tr_msg {
   enum msg_type msg_type;
-  union {
-    TID_REQ *tid_req;
-    TID_RESP *tid_resp;
-  };
+  void *msg_rep;
 };
 
 /* Accessors */

diff --git a/tid/tidc.c b/tid/tidc.c
index 1cb04cf..102e83e 100644 (file)
--- a/tid/tidc.c
+++ b/tid/tidc.c
@@ -132,7 +132,7 @@ int tidc_fwd_request (TIDC_INSTANCE *tidc,
     return -1;
 
   msg->msg_type = TID_REQUEST;
-  msg->tid_req = tid_req;
+  tr_msg_set_req(msg, tid_req);
 
   /* store the response function and cookie */
   // tid_req->resp_func = resp_handler;
@@ -174,14 +174,14 @@ int tidc_fwd_request (TIDC_INSTANCE *tidc,
   }
 
   /* TBD -- Check if this is actually a valid response */
-  if (!resp_msg->tid_resp) {
+  if (TID_RESPONSE != tr_msg_get_msg_type(resp_msg)) {
     fprintf(stderr, "tidc_fwd_request: Error, no response in the response!\n");
     return -1;
   }
   
   if (resp_handler)
     /* Call the caller's response function */
-    (*resp_handler)(tidc, tid_req, resp_msg->tid_resp, cookie);
+    (*resp_handler)(tidc, tid_req, tr_msg_get_resp(resp_msg), cookie);
   else
     fprintf(stderr, "tidc_fwd_request: NULL response function.\n");
 

diff --git a/tid/tids.c b/tid/tids.c
index 36d4e68..5dce161 100644 (file)
--- a/tid/tids.c
+++ b/tid/tids.c
@@ -197,10 +197,10 @@ static int tids_handle_request (TIDS_INSTANCE *tids, TR_MSG *mreq, TID_RESP *res
   int rc;
 
   /* Check that this is a valid TID Request.  If not, send an error return. */
-  if ((!mreq->tid_req) ||
-      (!mreq->tid_req->rp_realm) ||
-      (!mreq->tid_req->realm) ||
-      (!mreq->tid_req->comm)) {
+  if ((!tr_msg_get_req(mreq)) ||
+      (!tr_msg_get_req(mreq)->rp_realm) ||
+      (!tr_msg_get_req(mreq)->realm) ||
+      (!tr_msg_get_req(mreq)->comm)) {
     fprintf(stderr, "tids_handle_request():Not a valid TID Request.\n");
     resp->result = TID_ERROR;
     resp->err_msg = tr_new_name("Bad request format");
@@ -209,7 +209,7 @@ static int tids_handle_request (TIDS_INSTANCE *tids, TR_MSG *mreq, TID_RESP *res
 
   /* Call the caller's request handler */
   /* TBD -- Handle different error returns/msgs */
-  if (0 > (rc = (*tids->req_handler)(tids, mreq->tid_req, resp, tids->cookie))) {
+  if (0 > (rc = (*tids->req_handler)(tids, tr_msg_get_req(mreq), resp, tids->cookie))) {
     /* set-up an error response */
     resp->result = TID_ERROR;
     if (!resp->err_msg)        /* Use msg set by handler, if any */
@@ -261,7 +261,7 @@ int tids_send_response (TIDS_INSTANCE *tids, TID_REQ *req, TID_RESP *resp)
     return 0;
 
   mresp.msg_type = TID_RESPONSE;
-  mresp.tid_resp = resp;
+  tr_msg_set_resp(&mresp, resp);
   
   if (NULL == (resp_buf = tr_msg_encode(&mresp))) {
     fprintf(stderr, "tids_send_response: Error encoding json response.\n");
@@ -310,14 +310,14 @@ static void tids_handle_connection (TIDS_INSTANCE *tids, int conn)
     }
 
     /* Put connection information into the request structure */
-    mreq->tid_req->conn = conn;
-    mreq->tid_req->gssctx = gssctx;
+    tr_msg_get_req(mreq)->conn = conn;
+    tr_msg_get_req(mreq)->gssctx = gssctx;
 
     /* Allocate a response structure and populate common fields */
-    if (NULL == (resp = tids_create_response (tids, mreq->tid_req))) {
+    if (NULL == (resp = tids_create_response (tids, tr_msg_get_req(mreq)))) {
       fprintf(stderr, "tids_handle_connection: Error creating response structure.\n");
       /* try to send an error */
-      tids_send_err_response(tids, mreq->tid_req, "Error creating response.\n");
+      tids_send_err_response(tids, tr_msg_get_req(mreq), "Error creating response.\n");
       return;
     }
 
@@ -326,11 +326,11 @@ static void tids_handle_connection (TIDS_INSTANCE *tids, int conn)
       /* Fall through, to send the response, either way */
     }
 
-    if (0 > (rc = tids_send_response(tids, mreq->tid_req, resp))) {
+    if (0 > (rc = tids_send_response(tids, tr_msg_get_req(mreq), resp))) {
       fprintf(stderr, "tids_handle_connection: Error from tids_send_response(), rc = %d.\n", rc);
       /* if we didn't already send a response, try to send a generic error. */
-      if (!mreq->tid_req->resp_sent)
-       tids_send_err_response(tids, mreq->tid_req, "Error sending response.\n");
+      if (!tr_msg_get_req(mreq)->resp_sent)
+       tids_send_err_response(tids, tr_msg_get_req(mreq), "Error sending response.\n");
       /* Fall through to free the response, either way. */
     }