Bug in redir_ensure_challenge_rpc(): RPC overwrites conn and breaks REDIR_LOGIN

[alfredamb]

I found a bug in the UAM challenge RPC implementation.

The issue is in src/redir.c, inside redir_ensure_challenge_rpc().

The current code reads the RPC response directly into the existing
redir_conn_t structure:

if (safe_read(s, conn, sizeof(*conn)) != sizeof(*conn)) {
    ...
}

This overwrites the entire conn structure.

This is problematic during REDIR_LOGIN because conn also contains the
current authentication result, including fields such as:

conn.response
conn.reply
conn.s_params
conn.s_state.redir.*

When the login is rejected, conn.response is set correctly before the
challenge RPC is called. The RPC then overwrites conn with the state
returned by the main chilli daemon.

As a result, conn.response can become 0 and redir_reply() receives an
invalid response:

redir_reply(..., conn.response, ...);

This produces:

Unknown res in switch

For example, with an invalid username/password I get:

LOGIN REJECT: conn.response=0 hex=0x0 url= reply=(null)
redir.c: 1544: Unknown res in switch

Without the challenge RPC patch, the same login rejection works correctly
and the client is returned to the login page.

I reproduced the issue with the 1.9 source as well as with the older
version I am using.

Suggested fix:

Read the RPC response into a temporary structure instead of overwriting
conn:

struct redir_conn_t outconn;

memset(&outconn, 0, sizeof(outconn));

if (safe_read(s, &outconn, sizeof(outconn)) != sizeof(outconn)) {
    syslog(LOG_ERR, "%s: short read from %s",
           strerror(errno), remote.sun_path);
    safe_close(s);
    return -1;
}

Then copy only the fields that the RPC is supposed to update:

memcpy(conn->s_state.redir.uamchal,
       outconn.s_state.redir.uamchal,
       REDIR_MD5LEN);

conn->s_state.uamtime = outconn.s_state.uamtime;

This preserves conn.response, conn.reply and the other state belonging
to the current REDIR_LOGIN request while still updating the UAM
challenge.

I tested this change on my system and invalid credentials correctly
return to the login page instead of producing "Unknown res in switch".

The issue appears to be present in the current implementation because
the same safe_read(s, conn, sizeof(*conn)) pattern is present in the
1.9 source.

Expected behavior

When Access-Reject is received, the client should receive the normal
login failure/challenge response and remain on the login page.

Actual behavior

After applying the UAM challenge RPC patch, conn.response is overwritten
by the RPC response and becomes 0. redir_reply() then reports:

Unknown res in switch

[pintocar83]

Fix in redir_ensure_challenge_rpc (redir.c)
Could you incorporate the correction into the project?

/*
 * Ask the main chilli daemon to create or reuse the UAM challenge for this
 * subscriber (serialized in the daemon).  Replies with a full redir_conn_t
 * snapshot like REDIR_MSG_STATUS_TYPE.
 */
static int redir_ensure_challenge_rpc(struct redir_t *this,
				      struct sockaddr_in *address,
				      struct sockaddr_in *baddress,
				      struct redir_conn_t *conn,
				      long mtype_req) {
  struct redir_msg_t msg;
  struct redir_conn_t outconn;
  struct sockaddr_un remote;
  size_t len = sizeof(remote);
  int s;
  char filedest[512];

  (void)this;

  memset(&msg, 0, sizeof(msg));
  memset(&outconn, 0, sizeof(outconn));
  msg.mtype = mtype_req;
  memcpy(&msg.mdata.address, address, sizeof(*address));
  memcpy(&msg.mdata.baddress, baddress, sizeof(*baddress));

  statedir_file(filedest, sizeof(filedest), _options.unixipc, "chilli.ipc");

  if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
    syslog(LOG_ERR, "%s: socket()", __FUNCTION__);
    return -1;
  }

  memset(&remote, 0, sizeof(remote));
  remote.sun_family = AF_UNIX;
  strlcpy(remote.sun_path, filedest, sizeof(remote.sun_path));

#if defined (__FreeBSD__)  || defined (__APPLE__) || defined (__OpenBSD__) || defined (__NetBSD__)
  remote.sun_len = strlen(remote.sun_path) + 1;
#endif

  len = offsetof(struct sockaddr_un, sun_path) + strlen(remote.sun_path);

  if (safe_connect(s, (struct sockaddr *)&remote, len) == -1) {
    syslog(LOG_ERR, "%s: could not connect to %s", strerror(errno),
	   remote.sun_path);
    safe_close(s);
    return -1;
  }

  if (safe_write(s, &msg, sizeof(msg)) != sizeof(msg)) {
    syslog(LOG_ERR, "%s: could not write to %s", strerror(errno),
	   remote.sun_path);
    safe_close(s);
    return -1;
  }

  if (safe_read(s, &outconn, sizeof(outconn)) != sizeof(outconn)) {
    syslog(LOG_ERR, "%s: short read from %s", strerror(errno),
	   remote.sun_path);
    safe_close(s);
    return -1;
  }

  memcpy(conn->s_state.redir.uamchal, outconn.s_state.redir.uamchal, REDIR_MD5LEN);
  conn->s_state.uamtime = outconn.s_state.uamtime;

  shutdown(s, 2);
  safe_close(s);
  return 0;
}

redir.c