Bug 819609

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.)

(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.)

(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.

(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.

This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.

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

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.

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

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.

This message is a reminder that Fedora 25 is nearing its end of life.
Approximately 4 (four) weeks from now Fedora will stop maintaining
and issuing updates for Fedora 25. 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 '25'.

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 25 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.

This message is a reminder that Fedora 27 is nearing its end of life.
On 2018-Nov-30  Fedora will stop maintaining and issuing updates for
Fedora 27. 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 '27'.

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 27 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.

Fedora 27 changed to end-of-life (EOL) status on 2018-11-30. Fedora 27 is
no longer maintained, which means that it will not receive any further
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version. If you
are unable to reopen this bug, please file a new report against the
current release. If you experience problems, please add a comment to this
bug.

Thank you for reporting this bug and we are sorry it could not be fixed.