D22979: Security: remove support for $(...) in config keys with [$e] marker.

[Fabian Vogt]

fvogt added a comment.


  In D22979#508493 , @kives wrote:
  
  > Does anyone think this can be easily backported to previous versions of KDE 
in upstream distros such as Kubuntu, etc.?
  
  
  I backported this down to KConfig 5.20 and KDELibs 4.14.18, differences were 
trivial to resolve.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D22979

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: kives, ZaWertun, rikmills, fvogt, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[Kristopher Ives]

kives added a comment.


  Does anyone think this can be easily backported to previous versions of KDE 
in upstream distros such as Kubuntu, etc.?

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D22979

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: kives, ZaWertun, rikmills, fvogt, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[David Faure]

dfaure added a comment.


  Yes.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D22979

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: ZaWertun, rikmills, fvogt, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[Nathaniel Graham]

ngraham added a comment.


  Is Frameworks 5.61 going to be re-spun to include this?

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D22979

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: ZaWertun, rikmills, fvogt, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[David Faure]

This revision was automatically updated to reflect the committed changes.
Closed by commit R237:5d3e71b1d2ec: Security: remove support for $(...) in 
config keys with [$e] marker. (authored by dfaure).

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22979?vs=63253=63274

REVISION DETAIL
  https://phabricator.kde.org/D22979

AFFECTED FILES
  autotests/kconfigtest.cpp
  docs/options.md
  src/core/kconfig.cpp

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: ZaWertun, rikmills, fvogt, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[Matthew Dawson]

mdawson accepted this revision.

REPOSITORY
  R237 KConfig

BRANCH
  security_kill_popen

REVISION DETAIL
  https://phabricator.kde.org/D22979

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: ZaWertun, rikmills, fvogt, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[David Faure]

dfaure updated this revision to Diff 63253.
dfaure added a comment.


  Fix documentation; re-add test for $(hostname)

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22979?vs=63239=63253

BRANCH
  security_kill_popen

REVISION DETAIL
  https://phabricator.kde.org/D22979

AFFECTED FILES
  autotests/kconfigtest.cpp
  docs/options.md
  src/core/kconfig.cpp

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: fvogt, ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[David Faure]

dfaure marked 2 inline comments as done.
dfaure added inline comments.

INLINE COMMENTS

> mdawson wrote in kconfigtest.cpp:530
> Instead of removing this test, can it instead be switched to verify the 
> command execution does not occur?

Hehe, that's what I did initially, and the value being read was (hostname) 
without the $ because of the way [$e] works. A bit surprising, but in line with 
the fact that $/ $? $@ etc would also remove the $ (because the code just sees 
an empty env var name), and if someone wanted to keep the $ they would have to 
write $$. So I concluded invalid testcase, nobody would write this anymore. But 
OK, it's a test about old files that might have this. I'll re-add the test.

REPOSITORY
  R237 KConfig

BRANCH
  security_kill_popen

REVISION DETAIL
  https://phabricator.kde.org/D22979

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: fvogt, ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[Kai Uwe Broulik]

broulik added a comment.


  +1
  
  I got a valid usecase "Please don't fix this. I use a recursive symlink and a 
shell script to raise my machine load. The extra heat it produces keeps 
children warm in the winter." ;)

REPOSITORY
  R237 KConfig

BRANCH
  security_kill_popen

REVISION DETAIL
  https://phabricator.kde.org/D22979

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[Matthew Dawson]

mdawson added a comment.


  LGTM.  Regarding the test, if we want to get this change in asap due to the 
security focus I can submit a follow up patch re-adding it.

INLINE COMMENTS

> kconfigtest.cpp:530
>  << "URL[$e]=file://${HOME}/foo" << endl
> -<< "hostname[$e]=$(hostname)" << endl
>  << "escapes=aaa,bb/b,ccc\\,ccc" << endl

Instead of removing this test, can it instead be switched to verify the command 
execution does not occur?

> options.md:78
>  
> -Note that the application will replace `$USER` and `$(hostname)` with their
> +Note that the application will replace `$USER` with their
>  respective expanded values after saving. To prevent this combine the `$e` 
> option

Grammar suggestion: Note that the application will replace `$USER` with its 
expanded values after saving.

REPOSITORY
  R237 KConfig

BRANCH
  security_kill_popen

REVISION DETAIL
  https://phabricator.kde.org/D22979

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> options.md:70
>  
>  If an entry is marked with `$e`, environment variables and shell commands 
> will
>  be expanded.

"and shell commands"  to be dropped here no?

REPOSITORY
  R237 KConfig

BRANCH
  security_kill_popen

REVISION DETAIL
  https://phabricator.kde.org/D22979

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[David Edmundson]

davidedmundson accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R237 KConfig

BRANCH
  security_kill_popen

REVISION DETAIL
  https://phabricator.kde.org/D22979

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[Albert Astals Cid]

aacid added a reviewer: security-team.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D22979

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[David Faure]

dfaure updated this revision to Diff 63239.
dfaure added a comment.


  Also update docu, patch by David Edmundson, thanks

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22979?vs=63238=63239

BRANCH
  security_kill_popen

REVISION DETAIL
  https://phabricator.kde.org/D22979

AFFECTED FILES
  autotests/kconfigtest.cpp
  docs/options.md
  src/core/kconfig.cpp

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[David Faure]

dfaure created this revision.
dfaure added reviewers: mdawson, aacid, broulik, davidedmundson, kossebau, 
apol, sitter.
Herald added a project: Frameworks.
Herald edited subscribers, added: kde-frameworks-devel; removed: Frameworks.
dfaure requested review of this revision.

REVISION SUMMARY
  It is very unclear at this point what a valid use case for this feature
  would possibly be. The old documentation only mentions $(hostname) as
  an example, which can be done with $HOSTNAME instead.
  
  Note that $(...) is still supported in Exec lines of desktop files,
  this does not require [$e] anyway (and actually works better without it,
  otherwise the $ signs need to be doubled to obey kconfig $e escaping 
rules...).

TEST PLAN
  ctest passes; various testcases with $(...) in desktop files,
  directory files, and config files, no longer execute commands.

REPOSITORY
  R237 KConfig

BRANCH
  security_kill_popen

REVISION DETAIL
  https://phabricator.kde.org/D22979

AFFECTED FILES
  autotests/kconfigtest.cpp
  src/core/kconfig.cpp

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns