[Pkg-shadow-devel] xauth and su.c (upstream CVS version)

[Alexander Gattin]

Hello!

I'd like to discuss the patch which was introduced in
rev. 1.27 of src/su.c, xauth part at first:

+#ifndef USE_PAM
+		/*
+		 * Also leave DISPLAY and XAUTHORITY if present, else
+		 * pam_xauth will not work.
+		 */
+		if ((cp = getenv ("DISPLAY")))
+			addenv ("DISPLAY", cp);
+		if ((cp = getenv ("XAUTHORITY")))
+			addenv ("XAUTHORITY", cp);
+#endif				/* !USE_PAM */

What is the intent to make such a change? I think the
code is meant to serve like pam_xauth replacement in
case when "su" is compiled without PAM support. Thus,
we can see at the first look that its comments are
misleading.

Secondly, this code will not usually work when
switching from ordinary user to another ordinary user,
like "su - builder". It's because permissions on
~/.Xauthority are typically 0600 and another _ordinary_
user won't have the necessary rights to read the file.

When we do su to root, i.e. "su -", the patch will
suffice.

I only wanted to say that usefulness of the patch
is questionable. OK. Let's assume that the patch has
practical use. Then I'd like to propose a slight
improvement to it:

#ifndef USE_PAM
		/* Preserve environment for X authentication, if possible.*/
		if ((cp = getenv("DISPLAY"))) {
			addenv("DISPLAY", cp);
			cp = getenv("XAUTHORITY");
			if (cp) addenv("XAUTHORITY", cp);
			else if (pw->pw_dir && pw->pw_dir[0]) {
				cp = malloc(strlen(pw->pw_dir) + 13);
				sprintf(cp, "%s/.Xauthority",
					pw->pw_dir);
				addenv("XAUTHORITY", cp);
				free(cp);
			}
		}
#endif				/* !USE_PAM */

(patch for CVS version is attached).
I tested this myself (although without #ifndef USE_PAM)
and main purpose is to provide a correct reference to
.Xauthority in the cases where original user used default
.Xauthority file (no XAUTHORITY variable defined).

This is typical situation in Debian -- for example, I
have DISPLAY defined and no XAUTHORITY at all.
When I switch to root with "su -", DISPLAY is
preserved, but X authentication in absence of
XAUTHORITY (pointing to /home/arg/.Xauthority) will
assume default path of /root/.Xauthority, which is
wrong.

P.S.

With regard to Debian, there's no pam_xauth, and we
_could_ use the patch without #ifndef USE_PAM -- #endif,
as I currently do on my development computer.
But generally, _this approach isn't good_ because of
reasons described earlier (file permissions).

Of course, pam_xauth is quite superior:
1. auth control config
2. "physically" separated thus independent auth info
3. no problems switching between ordinary user accounts
and so on.
The only thing I don't like in pam_xauth is its
tendency to leave garbage in form of undeleted temporary
.xauthXXXXXX files (AFAIK, this happens when [PAM] session
finishes abruptly).

-- 
WBR,
Alexander