Bug 819609 - Please leave $COLUMNS and $LINES in the environment when stripping dangerous env vars in pkexec
Please leave $COLUMNS and $LINES in the environment when stripping dangerous ...
Status: NEW
Product: Fedora
Classification: Fedora
Component: polkit (Show other bugs)
25
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Miloslav Trmač
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-07 13:39 EDT by Lennart Poettering
Modified: 2016-11-24 10:20 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Lennart Poettering 2012-05-07 13:39:29 EDT
mitr just requested on the current FESCO discussion that pkexec leaves the COLUMNS environment variable set in pkexec, and doesn't filter it out as dangerous. The same should be done for LINES.
Comment 1 Miloslav Trmač 2012-05-07 13:44:51 EDT
That's not in fact what I asked. (Want to bet that there is no integer overflow on LINES or COLUMNS anywhere on the distribution?)

pkexec should have a configurable list of environment variables to pass to the executed program, corresponding to userhelper's KEEP_ENV_VARS.  (More important that COLUMNS is http_proxy,ftp_proxy necessary in preupgrade.)
Comment 2 David Zeuthen 2012-05-07 16:39:13 EDT
(In reply to comment #1)
> pkexec should have a configurable list of environment variables to pass to the
> executed program, corresponding to userhelper's KEEP_ENV_VARS.  

The assumption here is that people writing polkit .policy files just want to make things "work" - they don't understand all the subtleties involved, nor do they care. So, basically, I don't think something like KEEP_ENV_VARS is a good idea... I'm hoping this is something we can agree on, on a more axiomatic level since it's a common theme that applies to a lot more than polkit.

Anyway. Lennart specifically mentioned LINES and COLUMNS and I think adding these to the whitelist, see

http://cgit.freedesktop.org/polkit/tree/src/programs/pkexec.c?id=0.105#n406

may work (perhaps after proper validation, e.g. only allow integers or however LINES and COLUMNS work). Are there any others?

It could be that some programs need a couple of changes to actually work under pkexec(1) and maybe that's not a bad thing.

> (More important
> that COLUMNS is http_proxy,ftp_proxy necessary in preupgrade.)

More important than this, maybe, please explain why preupgrade would need to be run through pkexec(1) in the first place?

(I think you will find that in a lot of situations (maybe even preupgrade), you don't really need this... and that it's only this way because someone ended up being lazy instead of implementing the OS in a better and more secure way. That's why you need to be strict about core OS primitives like pkexec(1) and not allow stuff like KEEP_ENV_VARS. In there lies a slippery slope.)
Comment 3 Lennart Poettering 2012-05-07 18:07:01 EDT
(In reply to comment #2)
> (In reply to comment #1)
> > pkexec should have a configurable list of environment variables to pass to the
> > executed program, corresponding to userhelper's KEEP_ENV_VARS.  
> 
> The assumption here is that people writing polkit .policy files just want to
> make things "work" - they don't understand all the subtleties involved, nor do
> they care. So, basically, I don't think something like KEEP_ENV_VARS is a good
> idea... I'm hoping this is something we can agree on, on a more axiomatic level
> since it's a common theme that applies to a lot more than polkit.
> 
> Anyway. Lennart specifically mentioned LINES and COLUMNS and I think adding
> these to the whitelist, see
> 
> http://cgit.freedesktop.org/polkit/tree/src/programs/pkexec.c?id=0.105#n406
> 
> may work (perhaps after proper validation, e.g. only allow integers or however
> LINES and COLUMNS work). Are there any others?

I think adding LINES/COLUMNS to this whitelist like this would make a lot of sense. Possibly with a simple check that verifies that they are integers and >= 1 and <= 2^16-1 would be ideal.

> > (More important
> > that COLUMNS is http_proxy,ftp_proxy necessary in preupgrade.)
> 
> More important than this, maybe, please explain why preupgrade would need to be
> run through pkexec(1) in the first place?

The context is simply that preupgrade currently uses usermode, which we want to get rid of, and replace with pkexec.

I agree absolutely that just because preupgrade used to just take http_proxy/ftp_proxy as is, this doesn't mean we should implement the same 1:1 in PK.
Comment 4 Miloslav Trmač 2012-05-09 10:11:22 EDT
(In reply to comment #2)
> (In reply to comment #1)
> > pkexec should have a configurable list of environment variables to pass to the
> > executed program, corresponding to userhelper's KEEP_ENV_VARS.  
> 
> The assumption here is that people writing polkit .policy files just want to
> make things "work" - they don't understand all the subtleties involved, nor do
> they care.

I don't think that's the case - we are talking about people writing the /usr/share/polkit-1/actions/*.policy files for a specific 
org.freedesktop.policykit.exec.path , i.e. developers of the package in question or distribution packagers, _not_ system administrators or ordinary users.

For developers and packagers, "just wanting to make things work" is an absolute disaster when giving users elevated privileges.  "Not understanding" or "not caring" is not an option.  (Cf. the recent calibre mount helper trainwreck.)

Of course we can't enforce that, but there's no reason to encourage "not caring": when the developer is enabling one more variable, it is a reminder to review the code, and that review almost certainly wouldn't happen otherwise.


> So, basically, I don't think something like KEEP_ENV_VARS is a good
> idea... I'm hoping this is something we can agree on, on a more axiomatic level
> since it's a common theme that applies to a lot more than polkit.

The axiomatic rule that we want to achieve is  "any input to a privileged application needs to be validated by $something for a certain $set_of_assumptions, and any user of that input must correctly handle anything that fits $set_of_assumptions" - and it should be reasonably verifiable that it is so.

The thing to worry here is not only a bug in the privileged program itself - authors of that have hopefully been somewhat careful, although a different $set_of_assumptions is still a risk.  The more worrisome risk is a bug in some lower-level library that was perhaps not written with this use case in mind and is therefore more lax about security; given how deep our library stack is, these things happen (and this kind of problem happened even in libc - passing "********" to something that would ultimately pass it to fnmatch() was a reliable DoS).

The axiomatic approach sort of works for things like TERM and DISPLAY that are passed through quite a few components, but ultimately manipulated only in very few libraries (libncurses/libtermcap/libX11/libxcb?) - one can review the code of these libraries, derive $set_of_assumptions (e.g. the current "no /, .. and %"), and then enforce these in polkit.  This is not bulletproof because somebody somewhere might write some other code that will parse these values - but that can be argued to be unlikely enough (and within a distribution it is still practical to grep all code for these variables and check.)


> Anyway. Lennart specifically mentioned LINES and COLUMNS and I think adding
> these to the whitelist, see
> 
> http://cgit.freedesktop.org/polkit/tree/src/programs/pkexec.c?id=0.105#n406
> 
> may work (perhaps after proper validation, e.g. only allow integers or however
> LINES and COLUMNS work).

The thing with LINES and COLUMNS is, while a few libraries use it, there is an unbounded number of programs that parse it directly as well, so there is no reasonable way to define the $set_of_assumptions.  All it takes is a single program that does it differently.

For example, Lennart's suggestion of limiting between 1 and 2^16-1 would still allow a program that allocates ($COLUMNS*sizeof (struct column)) and happens to store $COLUMNS in a signed short.  There is no really safe value, perhaps 127 (or, rather, 127 - epsilon) would be safe, but then that is too small for many current terminals.  (Yeah, integers are much worse than strings in this respect because you can do so many more operations with integers.)


> Are there any others?

There is an unbounded number of variables that programs running as root might want to use, no way AFAICS to enumerate them - and even if you could, these variables _should not_ be passed to all programs run by pkexec only because one of them needs them.


> It could be that some programs need a couple of changes to actually work under
> pkexec(1) and maybe that's not a bad thing.
That's true.

> > (More important
> > that COLUMNS is http_proxy,ftp_proxy necessary in preupgrade.)
> 
> More important than this, maybe, please explain why preupgrade would need to be
> run through pkexec(1) in the first place?
> 
> (I think you will find that in a lot of situations (maybe even preupgrade), you
> don't really need this... and that it's only this way because someone ended up
> being lazy instead of implementing the OS in a better and more secure way.
> That's why you need to be strict about core OS primitives like pkexec(1) and
> not allow stuff like KEEP_ENV_VARS. In there lies a slippery slope.)

In theory, yes - however the rationale for somebody spending a week or more to port preupgrade to a split server/client model just isn't there IMO, working for a week on almost anything else would be more useful.  It doesn't make any sense to require anything less than root-equivalent authentication for preupgrade, and anyone able to provide this authentication doesn't need to bother exploiting preupgrade.
Comment 5 Fedora Admin XMLRPC Client 2013-02-01 18:59:29 EST
This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.
Comment 6 Fedora End Of Life 2013-04-03 12:20:57 EDT
This bug appears to have been reported against 'rawhide' during the Fedora 19 development cycle.
Changing version to '19'.

(As we did not run this process for some time, it could affect also pre-Fedora 19 development
cycle bugs. We are very sorry. It will help us with cleanup during Fedora 19 End Of Life. Thank you.)

More information and reason for this action is here:
https://fedoraproject.org/wiki/BugZappers/HouseKeeping/Fedora19
Comment 7 Fedora End Of Life 2015-01-09 12:08:38 EST
This message is a notice that Fedora 19 is now at end of life. Fedora 
has stopped maintaining and issuing updates for Fedora 19. It is 
Fedora's policy to close all bug reports from releases that are no 
longer maintained. Approximately 4 (four) weeks from now this bug will
be closed as EOL if it remains open with a Fedora 'version' of '19'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora 19 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora, you are encouraged  change the 'version' to a later Fedora 
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.
Comment 8 Jan Kurik 2015-07-15 11:09:21 EDT
This bug appears to have been reported against 'rawhide' during the Fedora 23 development cycle.
Changing version to '23'.

(As we did not run this process for some time, it could affect also pre-Fedora 23 development
cycle bugs. We are very sorry. It will help us with cleanup during Fedora 23 End Of Life. Thank you.)

More information and reason for this action is here:
https://fedoraproject.org/wiki/BugZappers/HouseKeeping/Fedora23
Comment 9 Fedora End Of Life 2016-11-24 05:38:59 EST
This message is a reminder that Fedora 23 is nearing its end of life.
Approximately 4 (four) weeks from now Fedora will stop maintaining
and issuing updates for Fedora 23. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as EOL if it remains open with a Fedora  'version'
of '23'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora 23 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora, you are encouraged  change the 'version' to a later Fedora 
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

Note You need to log in before you can comment on or make changes to this bug.