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

Authored by dfaure on Aug 6 2019, 10:37 PM.

Details

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.

Diff Detail

Repository
R237 KConfig
Branch
security_kill_popen
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14831
Build 14849: arc lint + arc unit
dfaure created this revision.Aug 6 2019, 10:37 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 6 2019, 10:37 PM
Restricted Application edited subscribers, added: kde-frameworks-devel; removed: Frameworks. · View Herald Transcript
dfaure requested review of this revision.Aug 6 2019, 10:37 PM
dfaure updated this revision to Diff 63239.Aug 6 2019, 10:38 PM

Also update docu, patch by David Edmundson, thanks

davidedmundson accepted this revision.Aug 6 2019, 11:07 PM
This revision is now accepted and ready to land.Aug 6 2019, 11:07 PM
kossebau added inline comments.Aug 6 2019, 11:12 PM
docs/options.md
69–70

"and shell commands" to be dropped here no?

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.

autotests/kconfigtest.cpp
530

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

docs/options.md
76–77

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

+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." ;)

fvogt added a subscriber: fvogt.Aug 7 2019, 7:27 AM
dfaure marked 2 inline comments as done.Aug 7 2019, 7:35 AM
dfaure added inline comments.
autotests/kconfigtest.cpp
530

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.

dfaure updated this revision to Diff 63253.Aug 7 2019, 7:36 AM

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

mdawson accepted this revision.Aug 7 2019, 1:13 PM
This revision was automatically updated to reflect the committed changes.

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

kives added a subscriber: kives.Aug 7 2019, 11:15 PM

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

fvogt added a comment.Aug 8 2019, 5:31 AM

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.