Bug 2606

Created attachment 1968 [details] [review]
no-security-1.patch

I think the patch is bad for two reasons:
1. Disabling the SECURITY extension will break "xauth", almost every ssh
implementaion under the moon (except OpenSSH which may surive this condition,
ssh.com's ssh doesn't handle that case), the XC-APPGROUP extension (see [2]) and
a couple of other things...
2. AFAIK the XC-APPGROUP extension needs the SECURITY extension to provide
"tags" for display connections - and right now the only implemented way to make
such tags  is to generate a MIT-MAGIC-COOKIE-1 which marks all applications
which use that cookie with the "tag" (kaleb may correct me if I am wrong...). 

(In reply to comment #2)
> I think the patch is bad for two reasons:
> 1. Disabling the SECURITY extension will break "xauth", almost every ssh
> implementaion under the moon (except OpenSSH which may surive this condition,
> ssh.com's ssh doesn't handle that case), the XC-APPGROUP extension (see [2]) and
> a couple of other things...

xauth only needs the Security extension for the 'generate' command, which is not
used in the default startx, and which anyway only makes sense in the context of
the server supporting the Security extension in the first place. 
BuildXCSecurity does not prevent xauth from supporting this command; it only
means the built server will not support it.  so it does not break xauth.

openssh's Security integration has made a lot of people unhappy and is generally
regarded as a bad move.  it silently changed the semantics of the -X option
which breaks most modern apps.  as far as i can tell ssh generates
MIT-MAGIC-COOKIE-1 tokens using its own internal PRNG so it wouldn't use the
xauth generate command in any case.  (this appears to be the way ssh behaves
back to the 1.2.27 branchpoint for openssh from Tatu Ylonen's original release;
i would be surprised if ssh.com's implementation behaved differently.)  the
point is, 'ssh -X' is broken anyway, disabling the Security extension in the
server does not change that.

it should be noted that openssh's X11 handling is done entirely without
including any X11 headers or linking against any X11 libraries.

not supporting the Security extension in the server is clearly not fatal, since
kdrive doesn't support it and works just fine with xauth and ssh.

> 2. AFAIK the XC-APPGROUP extension needs the SECURITY extension to provide
> "tags" for display connections - and right now the only implemented way to make
> such tags  is to generate a MIT-MAGIC-COOKIE-1 which marks all applications
> which use that cookie with the "tag" (kaleb may correct me if I am wrong...). 

if this is true, then build-time logic is required to do one of two things:

a) if BuildAppgroup && !BuildXCSecurity, turn BuildXCSecurity on
b) if BuildAppgroup && !BuildXCSecurity, turn BuildAppgroup off

either one, i'm not picky.

my reading of the appgroup code is that the Security extension is used to find
an authId on client state change (and then do something magic with it when the
Security extension exists), but that client state change notification only
matters if the server supports Security anyway.  so i don't know that this patch
necessarily breaks XC-APPGROUP.  i'll defer to expert opinion though.

(In reply to comment #3)
> (In reply to comment #2)
> > I think the patch is bad for two reasons:
> > 1. Disabling the SECURITY extension will break "xauth", almost every ssh
> > implementaion under the moon (except OpenSSH which may surive this condition,
> > ssh.com's ssh doesn't handle that case), the XC-APPGROUP extension (see [2]) and
> > a couple of other things...
> 
> xauth only needs the Security extension for the 'generate' command, which is not
> used in the default startx, and which anyway only makes sense in the context of
> the server supporting the Security extension in the first place. 
> BuildXCSecurity does not prevent xauth from supporting this command; it only
> means the built server will not support it.  so it does not break xauth.

It will break the "generate" command which is needed to grant applications
access to the display when there is no cookie yet - the usual usecase for this
is when the Xserver uses the SUN-DES-1 or MIT-KERBEROS-5 auth. and need to grant
a remote application access to the display where the remote machine isn't part
of the SecureRPC domain or part of the Kerberos system. Feel free to kill the
SECRUITY extension but don't forget to disable the user-to-user auth. schemes as
well.

> openssh's Security integration has made a lot of people unhappy and is generally
> regarded as a bad move.  it silently changed the semantics of the -X option
> which breaks most modern apps.

Well, that's cleary the fault of the toolkits. AFAIK they were informed about
the exploits around a _year_ before the first ssh version implemented the usage
of the SECURITY extension.
And this is no fatal condition for toolkits, they can easily trap the
"offending" X calls and handle them correctly (for example CDE2.x applications
don't have the problem and somewhere in Mozilla.org's bugzilla is a patch for
the Gecko Xlib port to let it run happily in X11 "untrusted" mode).

[snip]
> i would be surprised if ssh.com's implementation behaved differently.)  the
> point is, 'ssh -X' is broken anyway, disabling the Security extension in the
> server does not change that.

That implementation works completely different (and even handles the SECURITY
extension "timeout" correctly (openssh doesn't).

> it should be noted that openssh's X11 handling is done entirely without
> including any X11 headers or linking against any X11 libraries.
> 
> not supporting the Security extension in the server is clearly not fatal, since
> kdrive doesn't support it and works just fine with xauth and ssh.

This is FATAL for most ssh implementations _except_ openssh. I have excplitly
listed that case... it's the only ssh implementation which can recover from that
case AFAIK.

> > 2. AFAIK the XC-APPGROUP extension needs the SECURITY extension to provide
> > "tags" for display connections - and right now the only implemented way to make
> > such tags  is to generate a MIT-MAGIC-COOKIE-1 which marks all applications
> > which use that cookie with the "tag" (kaleb may correct me if I am wrong...). 
> 
> if this is true, then build-time logic is required to do one of two things:
> 
> a) if BuildAppgroup && !BuildXCSecurity, turn BuildXCSecurity on
> b) if BuildAppgroup && !BuildXCSecurity, turn BuildAppgroup off
> 
> either one, i'm not picky.

Kaleb can answer that...

(In reply to comment #4)
> It will break the "generate" command which is needed to grant applications
> access to the display when there is no cookie yet - the usual usecase for this
> is when the Xserver uses the SUN-DES-1 or MIT-KERBEROS-5 auth. and need to grant
> a remote application access to the display where the remote machine isn't part
> of the SecureRPC domain or part of the Kerberos system. Feel free to kill the
> SECRUITY extension but don't forget to disable the user-to-user auth. schemes as
> well.

So when the user asks to build without Security, they get a server that lacks
support for the Security extension, and all that that entails including broken
SUN-DES-1.  Seems pretty straightforward to me.

> Well, that's cleary the fault of the toolkits. AFAIK they were informed about
> the exploits around a _year_ before the first ssh version implemented the usage
> of the SECURITY extension.

Granted.

> This is FATAL for most ssh implementations _except_ openssh. I have excplitly
> listed that case... it's the only ssh implementation which can recover from that
> case AFAIK.

It is fatal iff:
- the ssh implementation implements Security integration (ie, is not PuTTY,
TeraTerm, lsh, or most of the Java ssh implementations i've seen), AND
- the user asks for X forwarding using the -X flag to openssh or the +x flag to
ssh.com's ssh.

The failure mode here is exactly what the user asks for: They built a server
without the Security extension, and then required the ssh client to use the
Security extension.

There's one other failure mode I suppose:

- The ssh implementation requires the Security extension even for normal (ie,
trusted) forwarding.  (ssh.com?)

This in my mind would be a regression in the ssh client, as it would no longer
be able to interoperate with X servers from before 6.3, when the Security
extension was introduced.  It is in any case giving the user what they asked for.

There is a BuildXCSecurity option in imake.  As a user I expect that if I turn
off any particular option in imake it will build to completion and make a
reasonable attempt at giving me what I asked for.  So, either the option needs
to disappear (and we just build with the Security extension all the time), or
the code or Imakefiles need to be fixed.

I am absolutely not suggesting turning off Security by default; I'm just asking
that my build not fail when I ask to not have it.

Created attachment 2298 [details] [review]
appgroup-needs-security.patch

Easy fix: if the user requests (appgroup && !security), force security on.

applied to HEAD, closing.