[vim/vim] modelinestrict is too strict and hardcoded (Issue #20028)
Lyderic Landry
lyderic created an issue (vim/vim#20028)The Problem
Today, I found out about the new modelinestrict option introduced in patch 9.2.350. I was opening a file that contains a modeline with a restricted option (namely foldmarker) and couldn't understand why the setting was no longer being applied. Looking around, I eventually stumbled upon issue #19875 which explains why this new option is necessary and why it is enabled by default.
Now, the hardcoded modeline_whitelist[] in option.c is a matter of debate. In my case, I don't understand why foldmarker is not whitelisted while foldmethod=marker is, and I am sure that each vim user will have their own opinion on which options should be added to this list.
Possible Solution
Ideally, I would like to have modeline_whitelist configurable. There would be the default, to which users could add additional options according to their needs and their understanding of the possible security implications.
Would something like this be feasible?
For example:
:set modelinewhitelist+=foldmarker
Alternatives
As it stands currently, the only fix is to let go entirely of modelinestrict: :set nomodelinestrict. It's a shame, but it's not a big deal: just a line to add in .vimrc. However, a configurable whitelist would be better in my opinion.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
MURAOKA Taro
koron left a comment (vim/vim#20028)A user-configurable whitelist can easily become an attack surface, which I believe contradicts the original purpose of the whitelist, which was to provide users with a secure list.
However, I agree with the idea of wanting to give options to users who understand the situation.
Since Vim already has a powerful autocmd, it should be possible to implement a modeline with arbitrary (limited) options using a plugin. Considering this, wouldn't it be easier to create such plugins if the parsing logic for the modeline part were provided as a built-in function in Vim scripts?
I've heard that @mattn has created a prototype function to interpret the contents of such a modeline.
cf. mattn@4557f26
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lyderic Landry
lyderic left a comment (vim/vim#20028)Thank you for you comment on my proposal.
My idea is not to get rid of the secure, hardcoded list, but for people who know what they're doing and accept the risk to be able to add to it.
If an attacker can access and alter a user's .vim configuration to modify the whitelist, then the user is doomed anyway.
Using autocmd, plugins etc. will actually solve the problem. But installing plugins and defining autocmd is complicated compared to simply add set nomodelinestrict to ~/.vimrc so it's highly likely people will take this route, and eventually the security won't be improved.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#20028)I deliberately chose a strict whitelist approach instead of a customizable one to ensure stronger security by default. If a different setup is required, modelinestrict can be disabled, accepting the pre-9.2.0350 security risk.
At this time, I am not aware of general issues with modelines, and we will continue to patch any newly reported vulnerabilities. The goal is simply to make it significantly harder for attackers to exploit potential future issues. This is especially relevant today, as tools like git make it more common to edit files from untrusted sources without being aware of the risks posed by modelines.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lyderic Landry
lyderic left a comment (vim/vim#20028)Thanks, Christian. It makes a lot of sense.
Can we please be consistent and allow foldmarker since foldmethod=marker is already allowed? These two options go hand in hand, don't they? (Unless prohibiting both makes more sense?)
Either way, it's not a big deal as long as :set nomodelinestrict remains available.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.