Subject: Re: [openssl-security] Privilege escalation to group "ssh" in ssh-agent via openssl library
From: halfdog <me ...
To: [not asked for permission yet]
Cc: [email-masked], [email-masked]
Date: Tue, 19 Dec 2017 22:46:33 +0000

[...] wrote:
> On Sun, Dec 17, 2017 at 08:31:23PM +0000, halfdog wrote:
> > Hello security teams,
> > 
> > In Debian, ssh-agent is a set-group-id binary with a nice feature
> > to gain access to that group and execute arbitrary code via included
> > openssl library. Usually that would be quite a bad security vulnerability,
> > but at least for ssh-agent, this is just a funny bug or unexpected
> > behaviour. See [0] for more information....
> > 
> > [0] http://www.halfdog.net/Security/2017/SshAgentGainGroupPrivileges/
> 
> So do you recommend we don't load engines when we're setuid or
> setgid?

Well, at least not at the users choice. Otherwise the S[UG]ID
separation has not other purpose than making a process not PTRACE
or dumpable by both users.

If there is a platform, where really a non-standard engine library
is needed in SUID-binaries, this should be configured by the system
administrator.

Also the use of an environment variable for that purpose asks
for trouble. At the moment from an attacker's point of view the
environment variable "OPENSSL_CONF" is equivalent to something
like "LD_LIBRARY_PATH". But while "LD_LIBRARY_PATH" is filtered
already by the ld.so before it could even reach libc or others,
additionally the variable is marked as "dangerous" in various
parts of documentation, the "OPENSSL_CONF" sounds quite harmless,
thus developers or admins may just miss its importance.

If openssl sticks to environment variable use in general might
be some hot debated issue - see Theo's remarks attached to the
page [0]. But if you decide to do so (accepting the risk, that
S[UG]ID-detection might fail; restricted shells, sudo et al.
suffer unexpected side effects,...), then I would recommend at
least:

* No environment variables use with S[UG]ID, no matter if
  e[ug]id==[ug]id.

* No custom configuration loading with S[UG]ID, only config
  files in /etc are considered.

* Some clear API, that someone integrating openssl library
  in insecure ways has to be aware of that fact in plain sight.
  This should also be obvious to any reviewer.

As removing the environment variables may not be the end of the
story for all programs - some developers may add a --conffile
instead not realizing what they are doing - maybe following API
changes might help in the long run:

* Add a state variable, e.g. "crossSecurityBoundaryInvocation"
  that is true, if during library initialization a such a invocation
  is detected. That might be due to euid!=uid, egid!=gid but
  also using information from library loaded or other sources
  if a platform does not support those checks. These checks will
  still take effect even if the program would switch to egid==gid
  later on.

* When in "crossSecurityBoundaryInvocation" mode, all environment
  variables are disabled.

* When in "crossSecurityBoundaryInvocation" mode, attempting to
  load custom configuration or enable other risky settings, e.g.
  change CAs, will result in an error when attempted.

* Only when the developer calls "enableRiskyAndInsecureConfigurationMode()",
  such stunts could still be done.

I hope, that reviewers would spot "enableRiskyAndInsecureConfigurationMode"
calls, where not needed. And those rare occasions, where such
behaviour is really needed - I could think of a SUID-binary loading
some data in privileged context before using it, overwriting any
remains in memory and then dropping back to the caller's uid/gid
before reading openssl configuration - those programs will use
the lines above "enableRiskyAndInsecureConfigurationMode" to explain
in detail, why this is needed and how the risk is controlled.

Best regards,
hd